-
Notifications
You must be signed in to change notification settings - Fork 31
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
Set a maximum value for line width; add more granularity; normalize to tree dimensions #144
Comments
Great suggestion @fedarko, increments of 0.1 sound like a good idea. |
I think this issue mainly stems from how the line width are calculated. Currently, the line width are created by creating a rectangle of width 2*line_width (in tree space). So, a line width of 2 will appear smaller or larger depending on how much space is spanned by the tree. see https://github.com/biocore/empress/pull/145/files#r401141659 |
From @kwcantrell, emphasis mine:
|
From discussion with @kwcantrell and @ElDeveloper, it sounds like one way to do this is computing the area of the tree display for a given layout and then scaling the line thicknesses to some proportion of that area -- say, 1/1000th of the area. It would be ideal to make this clear to users in a tooltip next to the line width selectors (#138), so that they'd have a clear understanding of what a unit of "1" or "2" means. (This would be a good time to adjust the line width stuff so that Things to look into:
The ideal thing would be using a more straightforward "unit" than A/1000 (e.g. pixels), ... but it seems like doing that in WebGL would be more trouble than it's worth. Maybe? |
Closes biocore#135! It would be nice to have some Empress / side-panel / animator tests that verify things work properly, but i think this is ok for now. Also of note: now, the default line width is 0. This will be important for biocore#144.
These, plus tests, should be sufficient for us to close that issue.
Not suuper satisifed with how the scaled line widths look, or the fact that I had to make the coefficient 1 / ten million to get things to not look janky. wondering if there's a better way to do this... but this at least works. Still need to add tests, but once that's done biocore#144 should be closeable
I think there is still more work that should be done on biocore#144, but this is probably sufficient for a PR for now.
Closing in favor of #276 since many of the issues here were already addressed. |
In practice, increasing the line width by 1 is a really big increase. In my opinion, it would be desirable to cap the line width at ~3, and then make the interface support smaller increases (e.g. by 0.1, or even keeping things as integers but scaling them by 1/10 or something), so that increasing the line width doesn't feel so sudden. (When you get up to line width = 3, it becomes really hard to see what's going on in the tree, especially when you zoom in.)
Below are screenshots of line widths = 1, 2, 3 for the two layouts. For each of the layouts' sections, I kept the camera at the same place for all 3 screenshots.
unrooted
Line width = 1:
Line width = 2:
Line width = 3:
rectangular
Line width = 1:
Line width = 2:
Line width = 3:
The text was updated successfully, but these errors were encountered: