Export svg

[sjanssen2]

Addressing #202, this is the very first rudimentary block of code that creates SVGs.
Open issues:

• discuss if GUI element for exporting is at the right location
• the generated SVG "page" is quite big and users will have to scale down if they intend to print the drawing. Can we transform the coords to a regular - say - letter document?
• can we replace the 15 line segments with one arc?
• add tests
• what is a good choice for the size of the circles representing tree nodes?
• transfer line width
• shall we aim to write node names into SVG tags or would that bloat the file too much?
• also export legend
• deactivate node drawing if set off in GUI

[ElDeveloper]

Very cool @sjanssen2!

[kwcantrell]

@sjanssen2 thanks!

Regarding the second issue: We can should be able to normalize the coordinate so that they fit in a standard letter document.

Regarding the third issue: Unfortunately WebGl can only draw lines/triangles and no longer supports the ability to natively draw arcs (hence the approximation with line segments). I'm not familiar with SVG, but Empress stores the information needed to draw arcs so it would be possible to create a custom "getCoords" function for SVG's that would represent the circular layout using arcs instead of line segements. What information would you need to represent arcs in SVG's?

[sjanssen2]

@biocore/empress-dev I'd like to also draw the color legend within the SVG. Does the empress object hold this information or is that only available in the SidePanel? If the later, should I move the exportSvg function from Empress to Side-Panel?

[sjanssen2]

@ElDeveloper @kwcantrell I think this PR is not yet ready to get merged, but a first round of code review would be very helpful. In addition, some opinions regarding the open questions are welcome, too!

[kwcantrell]

@sjanssen2 This looks good so far! I have a couple minor suggestions but nothing to major.

@ElDeveloper I would be interested in what you think about the location of this method. But, I think it might be better to move this to SidePanel. Mainly because the empress class does not have access to the legend, as that is something SidePanel is responsible for. But I also feel like the empress class shouldn't be responsible for creating the svg object as empress's main functionality is manipulating the tree using metadata.

[kwcantrell]

Just a heads up, after #190 is merged, this method will no longer return the proper coordinates since they will be calculated in WebGl. Maybe we can keep this method but rename it to getSvgCoords. @ElDeveloper any thoughts?

[ElDeveloper]

That sounds like a good way to go 👍 . And once we do that, it might be best to draw the arcs with single splines (which are supported in SVG) instead of line segments.

[sjanssen2]

Currently, we can simply draw a line for every two successive coordinates. For circular layout, if #190 is merged, we will than track if we have to draw a "normal" line or an arc. How would this method know? But I figure we should leave this issue for future development.

[ElDeveloper]

@sjanssen2 thanks again for contributing this much needed functionality. This is great!! 🎆

@ElDeveloper I would be interested in what you think about the location of this method. But, I think it might be better to move this to SidePanel. Mainly because the empress class does not have access to the legend, as that is something SidePanel is responsible for. But I also feel like the empress class shouldn't be responsible for creating the svg object as empress's main functionality is manipulating the tree using metadata.

I agree that having this method in SidePanel will likely simplify this and other issues. If this method gets more complex, we can even have a small SVGTreeRenderer(empress, sidepanel) class, but I'll defer to @sjanssen2 on what he thinks makes the most sense.

[sjanssen2]

I would prefer to move the export function from empress.js to side-panel-handler.js, however I am quite of stuck with unit-tests. Can someone show me how to properly initialize a SidePanel object? Especially, how do I best emulate document.getElementById('tip-color-key') in the unit test?

[kwcantrell]

@sjanssen2 I would be more than happy to set up a call to help you to help out with the unit tests! Alternatively, I will be submitting a PR for #142 within the next couple of days that will include tests for side-panel-handler.js you can look at.

[sjanssen2]

Regarding the second issue: We can should be able to normalize the coordinate so that they fit in a standard letter document.

Hi @kwcantrell, could you give me a code example on how to accomplish that?

[sjanssen2]

I am not 100% sure if I understand data flow within Empress correctly, but I was not able to move the export function to side-panel since I could not instantiate a side-panel object for unit testing. Furthermore, I had to create a separate function to export the legend(s), since they are not part of the tree object. I am also no particular fan of this block of code, as I have to count rows and increase indices and so on. Hard to read. @ElDeveloper did you came up with a better solution in Emperor, or is that simply an ugly business, due to explicit computation of x,y coordinates?

I suggest we merge the code now and raise new issues for the missing aspects / refactoring to give other developers the chance to use the existing export capabilities and suggest further improvements.

[ElDeveloper]

I am not 100% sure if I understand data flow within Empress correctly, but I was not able to move the export function to side-panel since I could not instantiate a side-panel object for unit testing. Furthermore, I had to create a separate function to export the legend(s), since they are not part of the tree object.

@kwcantrell has been working on improving the "testability" of the classes in the project. @kwcantrell is there a branch that you are working on that would make this easier? If so can we get that merged in sometime soon so that @sjanssen2 can add the needed tests here? I think it's important to have some tests now, specially if we want to improve/optimize this.

@ElDeveloper did you came up with a better solution in Emperor, or is that simply an ugly business, due to explicit computation of x,y coordinates?

In Emperor we re-use a class that's bundled with the GL framework (THREE.js).

---

Is there any chance we could limit the SVG render to what's currently visible on screen or would that be hard to compute. @kwcantrell might have some insight into this.

[ElDeveloper]

@sjanssen2 I think this PR looks like is in good shape to get merged soon, is there any chance you can solve conflicts, and look into limiting the the SVG export to only the edges that are currently visible on screen? Let me know if I can help with any of this! It would be wonderful if we can include this in the upcoming release that's currently scheduled for August 3rd.

🖨️ 🌲

[sjanssen2]

Still on vacation until Tuesday. Will look into this afterwards!

[sjanssen2]

only draw visible edges: this is a pretty big request and I would thus recommend to add it via another PR to keep this one slim and ease future reviewing. I see two problems:

1. clipping the drawing area would result in arithmetic repositioning of many line ends: Imagine an edge from (x1,y1) to (x2,y2). If (x2,y2) lies outside the drawing area, the intersection (x3,y3) of this edge with the drawing area border needs to be computed. Instead of bundling this into the SVG exporting function, I would rather create an additional function in the drawer class that "prunes" the data structures to those edgelists that actually needs to be drawn. Sounds like something webGL does internally. Could be get the coordinates of only the drawn elements back from its API?
2. I can't really image a use case, where someone would only want to export and touch up a tree into which she or he has zoomed interactively via Empress before. What we really need is the ability to collapse sub-trees into some triangular icons.

[sjanssen2]

@ElDeveloper @fedarko @kwcantrell
Conflicts should be gone, but I could need some help with the last few codestyle issues, since I cannot locally install prettier and Travis does unfortunately not report WHAT is not pretty enough yet :-/

[ElDeveloper]

@sjanssen2 Thanks, that makes sense. I agree that we should merge this.

I would rather create an additional function in the drawer class that "prunes" the data structures to those edgelists that actually needs to be drawn. Sounds like something webGL does internally. Could be get the coordinates of only the drawn elements back from its API?

@kwcantrell would you mind commenting on whether something like this could be easily doable right now? It's OK if it isn't, and we can postpone until later (in favor of getting this PR merged).

I can't really image a use case, where someone would only want to export and touch up a tree into which she or he has zoomed interactively via Empress before. What we really need is the ability to collapse sub-trees into some triangular icons.

We actually did something like this for the qemistree paper (see figure 3 panels b-g) where we wanted to highlight some internal details.

As for clade collapsing, @kwcantrell put together a PR here #277. If you have any input and time to provide feedback that would be great. 👍

[kwcantrell]

@ElDeveloper I'm not sure if its possible to retrieve the elements drawn from WebGl. I know in OpenGl you can write back to buffers but I don't believe you can do this in WebGl. I'll look into it through. However, it would be possible to create a special function in drawer.js that "mimics" the vertex shader and we could use that to returns the a list of edges that are drawn withing the bounds of the viewing window. But I think it would probably be better to leave this feature to another PR.

[ElDeveloper]

Thanks so much @kwcantrell. @sjanssen2, this looks great and is all good to merge. I submitted a small PR to your fork, let me know if you have any thoughts. After we get that merged this should be good to go.

[sjanssen2]

many thanks for your good catches in your PR @ElDeveloper. I consider this PR as good to go!

[ElDeveloper]

Thanks so much @sjanssen2!

[kwcantrell]

Thanks @sjanssen2!