Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nCoV animation should be smooth #918

Closed
jameshadfield opened this issue Mar 5, 2020 · 3 comments · Fixed by #920
Closed

nCoV animation should be smooth #918

jameshadfield opened this issue Mar 5, 2020 · 3 comments · Fixed by #920
Labels
bug Something isn't working good first issue A relatively isolated issue appropriate for first-time contributors please take this issue

Comments

@jameshadfield
Copy link
Member

This is due to us hardcoding some minimum amount of time to be shifted each tick. The tree is small enough that it should be animating super smoothly.

@jameshadfield jameshadfield added bug Something isn't working high priority please take this issue good first issue A relatively isolated issue appropriate for first-time contributors labels Mar 5, 2020
@trvrb
Copy link
Member

trvrb commented Mar 5, 2020

@dnprock
Copy link
Contributor

dnprock commented Mar 5, 2020

I'm not able to test with ncov dataset to see the behavior. I looked at this animation code. I think the choppy behavior is caused by remove/add of the temporal windows. When the code removes/adds svg rect, we see the choppy animation.

https://github.com/nextstrain/auspice/blob/master/src/components/tree/phyloTree/grid.js#L338

I suggest instead of using remove/add, you use d3.transition. That can help smooth out the animation.

I can play around with it when I have some time.

@hydrosquall
Copy link
Contributor

+1 to @dnprock 's observation that modifying the animationTick didn't have a noticeable impact on the observed smoothness of the animation (I had been looking into this issue prior to seeing their comment).

The issue seemed to me that the jumps between adjacent "rectangle" positions are fundamentally much larger when the totalDatasetRange is small, as is the case with the ncov sample (which spans months), whereas the Zika example linked above spans years. This is because the smallest granularity that can be encoded in the URL is at the "days" level of resolution. Changing that behavior (representing sub-day timestamps) seems out of scope for the PR, so representing those intermediary x coordinates with d3 transition instead of more granular dates seems reasonable.

const params = query.animate.split(',');
// console.log("start animation!", params);
window.NEXTSTRAIN.animationStartPoint = calendarToNumeric(params[0]);
window.NEXTSTRAIN.animationEndPoint = calendarToNumeric(params[1]);
state.dateMin = params[0];
state.dateMax = params[1];
state.dateMinNumeric = calendarToNumeric(params[0]);
state.dateMaxNumeric = calendarToNumeric(params[1]);

After figuring this out, I took a pass at the d3.transition based approach. It's working with a local copy of the ncov dataset using the following commands, but I'm not sure how to test a "right hand tree", so some tweaks may be needed to support that usecase. Here's a video clip (and the steps to reproduce this setup locally once you've cloned my branch): https://a.cl.ly/jkuKeJn7

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue A relatively isolated issue appropriate for first-time contributors please take this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants