Some SVG export fixes; add PNG export; other misc. improvements #384
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Declare variables with "var" - exportSvg() -> exportSVG() - exportSVG_legend() -> exportSVGLegend() - other minor things
will be useful when representing arbitrary legends in svg export
should unbreak tests
again, will help svg export
Still gotta, like, test the bulk of this tho. also code needs formatting
Also changed exported legend appearance a bit (no rounded rects, bg is white, color squares have a black border, etc)
legend still isn't properly accounted for in the viewbox; gotta fix. code is a lot prettier tho
the width is a sledgehammer solution; need to make it exactly correct. idk whats wrong
Now legends are fit in to the viewbox, and things seem mostly ok. Turns out the problem was using tree width (and hgt, but the problems i was seeing were just from width b/c legend wasn't fitting in on the x-axis) alongside "max X" stuff. Just storing things in units of "max X", etc. and THEN doing the -> width conversion after computing the SVG of the tree and legends fixed things. Remaining problems: - Push down the legend a bit; it looks like there is like a pixel or two at the top of it (in the border) that isn't shown. - Remove extra blank space on the right edge of the legend. Likely due to (2 * 4) (aka 2 * NODE_RADIUS) stuff being overzealous in how I'm using it. - Remove extra blank space on bottom edge of the legend when it is taller than the tree svg (e.g. for moving pictures f.m. coloring on level 7 taxonomy). Likely for same reason as above. - The tree is still flipped upside down. wat. Also, I need to document the heck out of the new SVG functions (in both empress.js and legend.js), and add tests preferably. Then it's PR time?
I have NO IDEA why this works. Maybe ... is Empress itself flipping the y-coordinates in the first place, and the SVG export was right all along??? argh.
Inclusion options discussed with @ElDeveloper this morning. For an initial PR i'll probably comment out a lot of this stuff (e.g. the barplots) but it should be adaptable as we continue to support more exporting stuff
we still need to include skbio as a dep because _plot needs to reference skbio.OrdinationResult or something (and also some of the python tests load trees as skbio TreeNodes before converting them to bp), but none of the main Empress python code touches TreeNode now at least
even for nodes without a specified name. Closes biocore#349 and closes biocore#348. TODO: improve selection menu interface for no-name nodes
- Add fancy description in selected node menu, rather than saying "Name: null" - Add note to BPTree.getNodesWithName() about dangers of having null as a key in BPTree._nameToNodes. Would be nice to test this case eventuallly ...
|
The following artifacts were built for this PR: empire-biplot.qzv, empire.qzv, empress-tree.qzv |
So, part of my confusion earlier was that rects (i.e. color squares) have their x and y describe their top left, but BY DEFAULT text tags in SVG have their x and y describe their bottom left. And you gotta use dominant-baseline to change that. Not sure what sadist approved that but ok
tbh i'm still kinda lost on why this works as well as it does but OH WELL :D
i warned you about styling bro / i told you dog
removes some work from empress.js, and lets us adjust context based on legend font (shouldn't be dynamic but just in case :O)
more accurate description
|
@fedarko this is great! I am still going over the PR. However, I wanted to ask if you think it would make sense to move the svg and png to an "export" class instead of the empress class? |
|
@kwcantrell Sure, that would work. I think it'd make sense to still have some exporting stuff within Empress (for doing things like tallying up all the active legends), but I just moved the bulk of the exporting code to |
Also rm'd an extra log statement
due to code reorganization in PR
Noticed that double-clicking on a category didn't work with the biplot generated for this PR. turns out that that's b/c I forgot to update the way the legend is cleared in that file. should be good now
Pair-programmed with @kwcantrell
|
Thanks @fedarko, looks good! |
|
Thanks both! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix Add PNG export option #330: Add (initial, basic) PNG export. It's a bit clunky in that what is exported changes as the user zooms around, but it should at least help for creating the paper figures right now (since the PNG export includes barplots/collapsed clades).
Split up SVG exporting somewhat -- now, one button exports the tree (and eventually other stuff drawn on the canvas), and another button exports the legend(s).
Fix Exported SVG images are flipped upside down #334: the exported images are now no longer flipped upside-down (or at the very least the vertical orientation is consistent with Empress' drawer; see that thread for discussion)
Fix Support drawing node circles for all nodes, regardless of if they were named in the Newick file or not #348: node circles are now drawn for all nodes, regardless of if they have a name or not
Tidy up the UI for unnamed nodes: now, rather than showing
Name: nullin the selection menu, these nodes are described as follows:Fix Don't draw node circles by default for large trees? #349: by default, node circles are not drawn.
Always draw a circle for the green "selected node" shape, regardless of whether or not node circles are drawn. (See the comment this PR adds above
c.uniform1i(s.isSingle, 1);indrawer.jsfor details about this. Not sure if the selected node shape turning into a square when node circles were turned off was intentional or not, but it should remain a circle now.)In the SVG export, don't draw a black circle at the root node any more (since Set root marker to a non-circle shape, and allow recoloring based on root node's color #211 was addressed a while back).
The main legend is now contained within
Empress, instead of withinSidePanel. This makes accessing the main legend in the SVG export easier, and makes things a bit more straightforward. (Similarly, nowAnimatordoesn't have direct ownership of the legend.)Legend SVG exporting code is now delegated to
Legendobjects (i.e. each legend now has anexportSVG()method we can call to get its SVG, making combining multiple legends' SVGs not too bad)Exported SVG legends resemble the "new"-styled legends more closely:
Some other minor improvements (split up SVG exporting code, declare variables in advance, use
camelCaseinstead ofsnake_case, make the node radii class attributes of theDrawer, etc.)Barplots and clade collapsing are still not supported in the SVG export -- that is up next. But these changes should make doing that a lot easier.
Things to maybe discuss
... Is the
this.NODE_CIRCLE_RADIUSvariable (formerly justs.pointSize, default4.0) actually a radius, or is it a diameter or something? Not sure what units it's in. (The SVG exporting code assumed it was a radius but IDK if that is true or not.)