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

Prototype of SVG gradient for tree branches #897

Closed
wants to merge 3 commits into from

Conversation

jameshadfield
Copy link
Member

See #896 for intention.

image

This requires using SVG rectangles for the branch stem as gradients are overly complex to add to paths. As such, lots of things are broken (e.g. all layouts except rect), and some functionality (branch hover colour changes, branch click to zoom) is disabled here.

I think it looks really good, but wanted to discuss this before we commit to cleaning up the code so that previous functionality is restored. cc @trvrb @rneher

The layout code defined both the stem & tee of a branch in a single SVG path, however the rendering code treated these differently and expected 2 paths. We were therefore rendering both paths via the code only intended for the stem.

This changes the display to draw stems over Ts. This should be improved by SVG gradients in future work.
@jameshadfield jameshadfield temporarily deployed to auspice-svg-gradient-osdh09srk February 17, 2020 01:52 Inactive
@trvrb
Copy link
Member

trvrb commented Feb 17, 2020

Fantastic work @jameshadfield! I'm super happy with this direction. I find the more continuous behavior to be more aesthetically pleasing, but I'd also note that people read a lot into these colors and (perhaps rightfully so) interpret an extended line of a particular color as arriving in that region at that moment, like so. Another good example of this would be the arrival of mumps into BC. The solid red line here makes it look like the arrival was 2013, when that's definitely not the case.

old new

I experimented with this with basically all the live builds and I think it's a consistent improvement. My only concerns at this point are:

  1. Possible performance issues as you mentioned previously (might deserve a small bit of testing to confirm it's not horribly slow)
  2. Loss of pleasing curvature at elbow joints in going from path to rect. (In original nextflu auspice we did polyline for this.) But I bet this can be worked out with thinking about placement and branch thickness.

@jameshadfield
Copy link
Member Author

Great -- I'll find time to restore the functionality so we can use this 👍
Completely agree about the elbow joints & am confident we can find a solution here, I just didn't spend any time looking (e.g. rectangles can have rounded corners).

@tsibley
Copy link
Member

tsibley commented Feb 18, 2020

It seems like it'd be helpful to stick with paths instead of rects if possible? My understanding is that gradients are very non-trivial for curved paths, but that they are relatively straightforward for straight paths at any given angle using a gradient transform. It seems to me that the branches we want to apply gradients to on any of our layouts are straight paths?

@trvrb
Copy link
Member

trvrb commented Feb 18, 2020

I don't know best solution between rect, polyline, path, etc... I would note that the only curved lines we use are in the radial layout. These are equivalent to the vertical lines in the rectangular layout and don't get a gradient assigned.

Inferred transitions from A-B (parent-child) are now represented as colour gradients from the parent to the child (previously the entire branch had the colour of the child, which could be misinterpreted to imply that the time of the transition was known).

Known issues:

1. the linear gradients are _only_ valid for horizontal lines (i.e. rectangular) trees. This will be fixed in a future commit.

2. Branch hover changes the colour & needs to be updated to use the gradient
Previously we changed the stroke colour of branches on hover. Here we extend this to handle linear gradients.

Previously, the on-hover stroke colour was somewhere between the "true" node colour and the "normal" stroke colour, and varied depending on the confidence attached to the colorBy at the node. This didn't look good with gradients, so I now emphasize the previously calculated stroke colour by increasing the saturation and making it slightly darker.
@jameshadfield jameshadfield temporarily deployed to auspice-svg-gradient-osdh09srk February 19, 2020 03:10 Inactive
@jameshadfield
Copy link
Member Author

jameshadfield commented Feb 19, 2020

Update:

  • Rectangular trees now working with paths.
  • Branch hover updated to use gradients & the way I emphasize colours on hover has changed.

To do:

  • Gradient axis needs to be updated depending on branch direction -- currently it is only Left - right, which is why it doesn't look like it should on non-rectangular phylogenies, and (although I haven't tested) will probably be wrong when a second tree is being displayed.
  • Ensure no branches are rendered as vertical or horizontal as you cannot apply linear gradients to such paths (this was why I initially didn't use paths). The jitter can be sub-pixel, so this solution is fine.
  • Test performance differences, esp with regards to animation.
  • Ensure it works for nodes with and without confidence values
  • Update gradients when the colorby changes -- that is, when you change the coloring via the sidebar dropdown, triggering a redux action, the tree needs to update the gradients appropriately.

@tsibley
Copy link
Member

tsibley commented Feb 19, 2020

Ensure no branches are rendered as vertical or horizontal as you cannot apply linear gradients to such paths (this was why I initially didn't use paths).

If the jittering of such paths is problematic, an alternate solution is to switch the coordinate system of the gradients into user space instead of using the path bounding box. I don't think this would be troublesome to specify since I think we already know the coordinates of the branch in user space. Jittering may be simpler though if it doesn't cause other issues.

@enjalot
Copy link

enjalot commented Mar 5, 2020

I don't know if this is helpful, it seems like a complex solution but could be helpful:
https://bl.ocks.org/mbostock/4163057

another idea for the rectangular version would be to simply render a circle under the intersection to smooth things out a little bit

@jameshadfield
Copy link
Member Author

@enjalot all the paths to have gradient applied are straight, so "normal" SVG gradients work (and are working in this PR) as long as the line isn't perfectly vertical / horizontal.

@jameshadfield
Copy link
Member Author

jameshadfield commented Mar 15, 2020

PR superseded by #947

@jameshadfield
Copy link
Member Author

Closed by the merge of #947

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants