[feature] Improve smoothness of temporalWindow animation over short time frames #920

hydrosquall · 2020-03-05T07:44:03Z

Motivation

Fixes nCoV animation should be smooth #918, which described a bug with temporal animations in the tree view. Users would get choppy results when the time interval spanned months or shorter instead of years.

Changes

Video with updated performance using d3.transition instead of destroying/creating the rectangles on every iteration of addTemporalSlice: https://a.cl.ly/jkuKeJn7
Rewrote rectangle movement to use CSS transforms instead of setting the x and y attributes
Adjusted the "transitioned" set of properties to the minimal set (x position), since it's simpler if height/width don't need to be modified on every frame.

Testing

mkdir data
curl http://data.nextstrain.org/ncov.json --compressed -o data/ncov.json
curl http://data.nextstrain.org/zika.json --compressed -o data/zika.json
node auspice view --datasetDir data

Feel free to test with other layouts or datasets, I tested exclusively with zika and nkov using the links provided by @trvrb here.

jameshadfield

Thanks @hydrosquall - the animation is much smoother here 😄

See line comments below as well as:

Two trees don't work appropriately (the x-axis coords can get confusing here) -- e.g. load two ncov trees (see localhost link in line-comment) and modulate the date sliders (regression).
Sliding the max-date slider to the end doesn't clear the grey shading (regression).
Clearing the max date filter (via the "x" in the info box in the header of the page) doesn't clear the grey shading (regression).
Sometimes, when the animation is "reset" (i.e. stopped), the grey regions remain, when they should disappear (regression).
Behavior at the start of /zika animation is weird when the left grey area comes into view. Didn't happen with the ncov dataset.
Sometimes (happens for me with /zika, but not /ncov) the grey areas remain at the end of the animation (regression).
Performance will need to be tested (but wouldn't expect this to add much overhead)

The animation behavior isn't easy in auspice, so don't be put off by all these regressions!

jameshadfield · 2020-03-09T03:14:48Z

src/components/tree/phyloTree/grid.js

@@ -334,17 +342,20 @@ export const addGrid = function addGrid() {
  timerEnd("addGrid");
 };

-
-export const removeTemporalSlice = function removeTemporalSlice() {


This removed function is still set as a prototype in phyloTree.js and called by the function modifySVGInStages. This will result in a complete app crash when (e.g.) the tree layout it changed

Ah, I didn't realize this was used on the outside, I should have checked given that it was exported. I'll bring this back.

I renamed these to hideTemporalSlice to make it clear that no DOM is being removed, just that visibility is toggled.

jameshadfield · 2020-03-09T03:15:18Z

src/components/tree/phyloTree/grid.js

@@ -188,7 +196,7 @@ const computeYGridPoints = (ymin, ymax) => {
 */
 export const addGrid = function addGrid() {
  const layout = this.layout;
-  addSVGGroupsIfNeeded(this.groups, this.svg);
+  addSVGGroupsIfNeeded(this.groups, this.svg, this.temporalWindow);


addSVGGroupsIfNeeded still takes only 2 arguments, but 3 are provided here

I had tried an alternative solution which had involved modifying the signature, and accidentally left this in - good catch.

jameshadfield · 2020-03-09T03:20:57Z

src/components/tree/phyloTree/grid.js

+      .attr("width", totalWidth)
+      .attr("fill", fill)
+      .transition('temporalWindowTransition')
+      .attr("transform", `translate(${translateX_start},0)`);


Cool! Am i right in understanding that on each update, we modify the rect's position via the transform attr, and in-between transforms the transition smooths things?

Correct, this is in contrast to the current behavior of creating a new rectangle on every frame.

Alternately, we have the option of fixing the position, and instead modifying the width on each frame. However, modifying position is cheaper to animate than modifying the width.

jameshadfield · 2020-03-09T03:22:39Z

src/components/tree/phyloTree/grid.js

-      .attr("x", rightHandTree ? xWindow[0] : 0)
-      .attr("width", rightHandTree ? totalWidth-xWindow[0]: xWindow[0])
-      .attr("y", 0)
+    const xEnd_start = rightHandTree ? totalWidth - xWindow[0] : xWindow[0]; // ending X coordinate of the "start" rectangle


It's important to address the case of two-trees, where the x-axis is flipped. Currently (e.g.) localhost:4000/ncov/2020-02-11:ncov/2020-02-09?dmax=2020-01-26&m=num_date is broken as somewhere in this PR (probably not this line) we are assuming x-values are left-to-right

… coordinate system

…en appearing and disappearing, replace visibility with opacity

hydrosquall · 2020-03-11T03:27:47Z

Thanks for the thorough feedback @jameshadfield ! I've moved your comments into a checklist for you or another reviewer to cross-validate my initial results. I believe the latest changes solve all the regressions identified, but please let me know if you end up finding a different result in your local testing.

Two trees don't work appropriately (the x-axis coords can get confusing here) -- e.g. load two ncov trees (see localhost link in line-comment) and modulate the date sliders (regression).

I didn't know how to test this earlier, but I opened this up side-by-side with nextstrain.org with the help of your link, and think I have something matching now. Please let me know if you notice more differences.

Sliding the max-date slider to the end doesn't clear the grey shading (regression).

This should be fixed as of the latest version, I added code to hide the regions if they don't pass the "minimum width" test.

Clearing the max date filter (via the "x" in the info box in the header of the page) doesn't clear the grey shading (regression).

I think this should be fixed now.

Sometimes, when the animation is "reset" (i.e. stopped), the grey regions remain, when they should disappear (regression).

I wasn't able to reproduce this, can you let me know if you still observe this one?

Behavior at the start of /zika animation is weird when the left grey area comes into view. Didn't happen with the ncov dataset.

I suspect this was one of the animations was animating into its initial position instead of appearing instantly, I've added some custom logic to avoid doing the interpolation when the left gray area first appears. Let me know if this reoccurs.

Sometimes (happens for me with /zika, but not /ncov) the grey areas remain at the end of the animation (regression).

I think the issue was related to a change in the visibility toggling logic, I think it should be better now (I've been testing in both datasets).

Performance will need to be tested (but wouldn't expect this to add much overhead)

To handle the edge cases at the start/end of the animation, I had to animate width for the endRegion panel instead of just transforming the x position as I had originally hoped. After I did this, performance is slightly worse than when I first opened this PR, and you can see some animation slowdown in the busiest parts of the /zika tree.

If it's too much of a slowdown, there's another technique we can try (applying a clip path and using a fixed width panel). The problem is that we would need to figure out when to regenerate the clip path, so didn't want to pursue that approach until it becomes absolutely necessary. I didn't notice a performance degradation on the ncov animation.

jameshadfield · 2020-03-23T06:01:45Z

Quick note to say thanks for this & that I'll try to test / merge this as soon as possible!

hydrosquall · 2020-03-23T22:19:55Z

No worries/rush @jameshadfield ! Let me know if there are issues and I'm happy to go through whatever rounds of iteration are necessary.

jameshadfield

Have tested this (reasonably) thoroughly and think it's great -- thankyou! I'll try to release this in a new version of auspice ASAP, which we'll release to dev.nextstrain.org for ~24hours of further testing before going live. Thanks again!

hydrosquall force-pushed the cameron.yick/bug/improve-timeline-animation branch from 3c0ae14 to a95b7ae Compare March 5, 2020 07:44

jameshadfield requested changes Mar 9, 2020

View reviewed changes

hydrosquall added 2 commits March 10, 2020 19:54

[feature] Improve smoothness of animation for temporalWindow movement

099b625

[feature] Adapt transition-based animation to work with rightHandTree…

6ea60d0

… coordinate system

hydrosquall force-pushed the cameron.yick/bug/improve-timeline-animation branch from a95b7ae to 6ea60d0 Compare March 11, 2020 00:57

hydrosquall added 2 commits March 10, 2020 21:11

[animation] Clarify names for show/hiding temporal slices

2c0752e

[animation] Remove unnecessary animation when frame transitions betwe…

eb2629f

…en appearing and disappearing, replace visibility with opacity

hydrosquall requested a review from jameshadfield March 11, 2020 03:27

Polish jumpy-ness in the endRegion animation by using CSS scaling

27bbba1

hydrosquall force-pushed the cameron.yick/bug/improve-timeline-animation branch from 0b10c33 to 27bbba1 Compare March 11, 2020 03:29

jameshadfield temporarily deployed to auspice-cameron-yick-bu-mdjnug March 13, 2020 05:08 Inactive

jameshadfield approved these changes Apr 8, 2020

View reviewed changes

jameshadfield merged commit 9234f06 into nextstrain:master Apr 8, 2020

hydrosquall deleted the cameron.yick/bug/improve-timeline-animation branch April 11, 2020 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] Improve smoothness of temporalWindow animation over short time frames #920

[feature] Improve smoothness of temporalWindow animation over short time frames #920

hydrosquall commented Mar 5, 2020

jameshadfield left a comment

jameshadfield Mar 9, 2020

hydrosquall Mar 10, 2020 •

edited

Loading

hydrosquall Mar 11, 2020

jameshadfield Mar 9, 2020

hydrosquall Mar 10, 2020

jameshadfield Mar 9, 2020

hydrosquall Mar 11, 2020

jameshadfield Mar 9, 2020

hydrosquall commented Mar 11, 2020 •

edited by jameshadfield

Loading

jameshadfield commented Mar 23, 2020

hydrosquall commented Mar 23, 2020

jameshadfield left a comment

[feature] Improve smoothness of temporalWindow animation over short time frames #920

[feature] Improve smoothness of temporalWindow animation over short time frames #920

Conversation

hydrosquall commented Mar 5, 2020

Motivation

Changes

Testing

jameshadfield left a comment

Choose a reason for hiding this comment

jameshadfield Mar 9, 2020

Choose a reason for hiding this comment

hydrosquall Mar 10, 2020 • edited Loading

Choose a reason for hiding this comment

hydrosquall Mar 11, 2020

Choose a reason for hiding this comment

jameshadfield Mar 9, 2020

Choose a reason for hiding this comment

hydrosquall Mar 10, 2020

Choose a reason for hiding this comment

jameshadfield Mar 9, 2020

Choose a reason for hiding this comment

hydrosquall Mar 11, 2020

Choose a reason for hiding this comment

jameshadfield Mar 9, 2020

Choose a reason for hiding this comment

hydrosquall commented Mar 11, 2020 • edited by jameshadfield Loading

jameshadfield commented Mar 23, 2020

hydrosquall commented Mar 23, 2020

jameshadfield left a comment

Choose a reason for hiding this comment

hydrosquall Mar 10, 2020 •

edited

Loading

hydrosquall commented Mar 11, 2020 •

edited by jameshadfield

Loading