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

improve phylotree - react interface #481

Closed
jameshadfield opened this issue Dec 14, 2017 · 3 comments
Closed

improve phylotree - react interface #481

jameshadfield opened this issue Dec 14, 2017 · 3 comments
Labels
bug Something isn't working enhancement New feature or request

Comments

@jameshadfield
Copy link
Member

jameshadfield commented Dec 14, 2017

The Narrative component has highlighted issues in how we call phylotree methods in order to update the tree. Currently we have a system where we compare old vs new props (react-like pattern) in TreeView, identify the salient changes and call the appropriate PhyloTree methods. (Phylotree also stores it's own state.) This works ok for current user interactions, but when too many changes are present the D3 methods don't perform as expected (my guess is that there are too many being called and they are called too quickly after one another and the transitions are colliding).

Proposal:

  • Store copies of relevant state (react.props) PhyloTree. The PhyloTree state dictates what is drawn. TreeView can therefore compare (new) redux state with D3 state and call the appropriate D3 methods (each methods updating the relevent PhyloTree state).
  • This removes the reliance on comparing old props to new props and allows batching of updates (i.e. update the tree on a "tick" cycle instead of every time the reducers have changed). This should have positive performance implications for the animation.
  • The tick-like cycle of D3 updates should prevent bugs where a D3 function fires before the previous one has completed.
  • This should remove cases of react & phylotree being "out of sync" as we are comparing "current D3 state (what is drawn)" to "current redux state (what should be drawn)", rather than having to capture every incremental change to props.
  • When there are too many differences, simply re-render the tree (like bf3bc4a) rather than trying to update certain aspects.

@colinmegill can you please advise us whether this is a good idea.

@jameshadfield jameshadfield added bug Something isn't working enhancement New feature or request labels Dec 14, 2017
@rneher
Copy link
Member

rneher commented Dec 21, 2017

not sure how to best do this. A while back we discussed whether we should make PhyloTree stateless and trigger all updates from upstream. But this has been dormant for a while

https://github.com/nextstrain/phyloTree

Not sure what the best way forward is, but we certainly need to deal with colliding transitions. And the zooming was also causing issues, right?

@jameshadfield
Copy link
Member Author

jameshadfield commented Dec 21, 2017

I am in favor of keeping phylotree as stand-alone as possible, which is why I am trying to go the route of maintaining "state" in phylotree, comparing react state to that, and firing the necessary changes. I can't see how we avoid maintaining some state in phylotree (e.g. divergence or temporal?). I forgot that repo existed - i'll have a browse.

Re: transition collisions, how about maintaining a workInProgress flag and toggling it off like so:

   .transition()
   .on('end', () => workInProgress = false)

@jameshadfield
Copy link
Member Author

closed in #501

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants