ENH: Add synchronized animations

[ElDeveloper]

In an Empire plot, users can start an animation in Emperor, and the tree will
update synchronously with the trajectories. Only points in the gradient are
visualized in the tree.

[emperor-helper]

The following artifacts were built for this PR: empire-biplot.qzv, empire.qzv, empress-tree.qzv

[kwcantrell]

Thanks @ElDeveloper! I have a few minor comments. I also can't seem to make the linked.qzv to sync animations. I choose day as gradient and trajectory as trajectory in emperor and hit play in emperor. However, only emperor showed the animation. Empress did nothing.

[ElDeveloper]

Depends on: biocore/emperor#780

[fedarko]

Thanks @ElDeveloper -- a few comments/suggestions.

The main concern I have is that the syncing doesn't seem to be directly tested, but I can see how doing that would be pretty tricky (this PR should be ok without direct tests for now). Maybe worth making a new issue for directly testing the callbacks / Emperor integration functionality?

[fedarko]

It's great that the colors are shared now! I spent some time playing around with this today -- it's super cool. There are a few issues that came up; detailing below. (I installed the master branch of Emperor alongside this PR version of Empress, since I think McHelper's Emperor version is a bit behind?)

Problem 1

When certain fields are selected as the trajectory / gradient in Emperor, I see an Uncaught TypeError: Cannot read property 'value' of undefined error in the console:

With days-since-experiment-start as the gradient field, the following trajectory fields cause this error for the MP dataset:

1. SampleID (this one makes sense, since I don't think Empress considers sample ID to be a metadata field. We could probably add a special case or something that warns the user when they pick this for an animation.)
2. barcode-sequence
3. month (no idea why year is ok but this isn't)
4. days-since-experiment-start

Problem 2

I sometimes get an error in the console that THREE.DirectGeometry: Faceless geometries are not supported. I can only seem to reproduce this with days-since-experiment-start as the gradient when trajectory is the trajectory, bizarrely:

It's strange -- as you can see by the number next to the error message, a lot of these errors are thrown in succession. Each time I restart the animation (using this exact gradient / trajectory / speed) I get exactly 33 of these errors. Changing the speed seems to change the number of errors I get (???). No idea what's going on here.

Problem 3

It's possible to start an Empress animation while an Emperor animation is also running. This results in the tree sort of switching back and forth between the two?

Sort of similarly to #195, we should probably disable the Empress animation panel during synchronized animations. I'm ok with deferring this to a future issue if you'd prefer to get this PR in sooner rather than later.

Question

Not all that important, but Empress animations (when finished) show a toast message saying Animation complete. Do you think synchronized animations should also show this on finishing? I don't really mind either way, but wanted to run it by you.

[ElDeveloper]

Thanks @fedarko!

Problem 1

When certain fields are selected as the trajectory / gradient in Emperor, I see an Uncaught TypeError: Cannot read property 'value' of undefined error in the console:

This is an Emperor feature/bug if you see the traceback this originates from the animations-controller.js. This happens when only single-sample trajectories are selected so there's nothing to animate. This should be better handled on the Emperor side.

Problem 2

I sometimes get an error in the console that THREE.DirectGeometry: Faceless geometries are not supported. I can only seem to reproduce this with days-since-experiment-start as the gradient when trajectory is the trajectory, bizarrely:

This is an issue with THREE.js the most recent build that emperor uses raises this error, with no "visible" consequences. I haven't been able to figure out what's going on here. I think we just need to refactor the way Emperor handles noop cycles for asynchronous trajectories.

Problem 3

It's possible to start an Empress animation while an Emperor animation is also running. This results in the tree sort of switching back and forth between the two?

I've now created a method to disable and reset UI elements in the animation panel. Note that I had to move __startOptions to be a public method startOptions if needed I can do the same thing for other methods, but I think for now we only need this one.

Question

Not all that important, but Empress animations (when finished) show a toast message saying Animation complete. Do you think synchronized animations should also show this on finishing? I don't really mind either way, but wanted to run it by you.

I just added this as well. Luckily there's an animation-ended event so I added this call there.

[fedarko]

Thanks, @ElDeveloper! Looks good to me. For reference I opened two issues about problems 1 and 2 in Emperor's repo, just to keep track of them. (biocore/emperor#784, biocore/emperor#785)

Two remaining points, but I'm ok to merge this code in with or without them:

1. Minor suggestion with the enabling / disabling code (see below, or above I forget which way github orders this stuff)

2. It looks like it's possible to start an animation in Empress and then start an animation in Emperor (and both collide). Sorry for not anticipating this in the earlier review. Would it be possible to extend the disabling stuff so that Empress animations temporarily disable Emperor animations (so it works the other way also)? I can imagine that being more work than it's worth (and it is such a silly corner case that I doubt anyone not actively trying to break things will run into it), so I'm ok deferring that to an issue.

[ElDeveloper]

@fedarko, both notes should be addressed. Let me know what you think.

[fedarko]

Thanks @ElDeveloper. I found a few issues:

1. It looks like synchronized animations don't work any more:

2. Emperor animation UI stuff is are now disabled during Empress animations, but Empress animation UI stuff is still enabled when Emperor animations are running.

Emperor animation is correctly disabled when animating in Empress:

Empress animation is still enabled when animating in Emperor, though:

3. Every Empress animation seems to now start on its second, rather than its first, frame. Guessing there is an off-by-one error somewhere, but not sure what exactly caused this. I verified that this is not a problem on the current master version of Empress, so it seems like this is getting introduced in this PR.

I am not sure where some of these are coming from. Would be free to try to pair-debug this on call later today / this week.

[ElDeveloper]

1. It looks like synchronized animations don't work any more:

Not sure how this happened, but I just resolved conflicts and can't see the problem.

1. Emperor animation UI stuff is are now disabled during Empress animations, but Empress animation UI stuff is still enabled when Emperor animations are running.

I can't reproduce this problem (may have to do with the resolved conflicts).

1. Every Empress animation seems to now start on its second, rather than its first, frame. Guessing there is an off-by-one error somewhere, but not sure what exactly caused this. I verified that this is not a problem on the current master version of Empress, so it seems like this is getting introduced in this PR.

I found the reason for this, should be fixed.

[fedarko]

Thanks @ElDeveloper! Everything looks good now. (For reference, it looks like problems 1 and 2 were due to me using the empire-biplot version on McHelper -- pulling down the changes locally worked.)

I have one small observation; during Empress-started animations the Collapse Clades and Extra Line Width elements are left enabled, while this PR disables them during Emperor-started animations as well. It might be nice to make this consistent (either disabling them during Empress-started animations, or leaving them enabled during Emperor-started animations); however, I'm ok delaying this to later if you'd like. This code should be good to go now 👍

[fedarko]

Thanks @ElDeveloper, looks good to me! Will merge in when the build passes.

[ElDeveloper]

Thanks @fedarko