Improve draw

[kwcantrell]

@antgonza if you have some spare time, can you test this pr on one of your large trees? thanks

[emperor-helper]

The following artifacts were built for this PR: empire-biplot.qzv, empire.qzv, empress-tree.qzv, just-fm.qzv, plain.qzv

[fedarko]

This looks cool, thanks for getting this ready!

Read over things, and I have a few suggestions. I think the most important are:

• The structure of the getTreeCoords / getTreeColor functions in empress.js -- would it be possible to merge these back into a single function, maybe by adding a parameter that controls whether it returns colors or coordinates? I think duplicating this logic will cause some problems (although I understand that it's already kinda duplicated between getCoords and thickenColoredNodes now, though).

• Use of 256 as the "base" for the compressed colors -- since hex colors range from [0, 255], I think we should use 255 instead.

Also, I am not sure what is causing it, but it looks like there is a strange bug when turning off collapsed clades where the colors become broken and some nodes remain hidden. My guess is that this might be a problem with getTreeCoords and getTreeColor getting out of sync, but I'm not confident about that.

[kwcantrell]

Thanks @fedarko for the feedback. I believe all of your comments have been addressed.

A couple of things:

1. I fixed the collapse clades issue you pointed out (good catch)
2. I also fixed the export svg function

The structure of the getTreeCoords / getTreeColor functions in empress.js -- would it be possible to merge these back into a single function, maybe by adding a parameter that controls whether it returns colors or coordinates? I think duplicating this logic will cause some problems (although I understand that it's already kinda duplicated between getCoords and thickenColoredNodes now, though).

I think this should get moved to a different PR. We already have multiple functions that use the same pattern so, creating a single function will require a bit of restructuring that I is a bit outside of the scope of this PR.

[fedarko]

Going through now, changes look great. It looks like the clade collapsing is mostly fixed, but I think there's still some issues with collapsing and the rectangular / circular layouts. It looks like turning off collapsing doesn't re-draw the arcs in the circular layout or the vertical lines in the rectangular layout, and causes some coloring problems:

Circular layout:

Rectangular layout:

It also looks like turning off coloring in general doesn't fix this, either (the arcs/lines only seem to come back when I uncheck "collapse clades"):

I think the issue may have to do with resetTree(), and that getTreeCoords() is called before this._collapsedClades is reset (which causes the vertical lines / arcs to not be drawn since they're still included in the now-removed collapsed clades). But I'm not 100% sure -- something similar also might be what's messing with the colors not being correct.

[kwcantrell]

good catch @fedarko! It should be fixed now.

[fedarko]

I haven't gone through all of the export util / empress stuff yet, but have gone through about everything else. Have a few mostly small comments. This is looking great, thanks so much!!!

[fedarko]

I agree that the var is needed :), but I think we should still divide this by 2 right?

... I guess it doesn't matter too much since the "units" are kind of arbitrary, but idk.

[kwcantrell]

The circles were not really visible when we divide by 2 (at least on my monitor with the moving pictures data set)

Linked are two svg. One that divides the diameter by 2 and one that does not. Let me know which one looks better - it could just be my monitor :).

examples.zip

[fedarko]

Thanks for the examples! It's subjective, but I think keeping the division by 2 looks slightly better, so I think we should stick with that for right now. It does make it harder to see node circles when zoomed out, but things look less cluttered when the SVGs are zoomed in (... at least on my computer :) --

with division by 2:

with no division by 2:

Neither solution is great, honestly, since the size of node circles isn't proportional to the tree size or anything. We should ideally try to scale node circles based on the tree dimensions, similarly to the stuff for edge thicknesses described in #276.

[fedarko]

Thanks, this looks good! (And sorry for taking a few days to get back to you on this.) The bulk of this is basically ready as is, but there are some documentation / internal code things I think we should sort out first.

Besides the comments I made here, there are a few places with old documentation saying "RGB arrays" instead of RGB floats, including:

Part of Empress.colorSampleGroups():

empress/empress/support_files/js/empress.js

Lines 1926 to 1927 in aaac6e3

	// convert hex string to rgb array
	var rgb = Colorer.hex2RGB(group);

Collapsed clade data:

empress/empress/support_files/js/empress.js

Line 310 in aaac6e3

* color: <[r,g,b]>

Part of Empress._colorTree():

empress/empress/support_files/js/empress.js

Lines 2206 to 2211 in aaac6e3

	* @param{Object} cm Maps categories to the colors to color their nodes
	* with. Colors should be represented as RGB arrays, for
	* example as is done in the color values of the output
	* of Colorer.getMapRGB().
	*/
	Empress.prototype._colorTree = function (obs, cm) {

Part of BarplotLayer.initFMDiv():

empress/empress/support_files/js/barplot-layer.js

Lines 246 to 250 in aaac6e3

	// To my knowledge, there isn't a straightforward way of
	// getting an RGB array out of the "TinyColor" values passed in
	// by Spectrum: see
	// https://bgrins.github.io/spectrum#details-acceptedColorInputs
	scope.defaultColor = Colorer.hex2RGB(newColor.toHexString());

The border color stuff in the barplot panel handler:

empress/empress/support_files/js/barplot-panel-handler.js

Lines 139 to 140 in aaac6e3

	// Borders (when enabled) default to white. (This is an RGB array.)
	this.borderColor = [1, 1, 1];

empress/empress/support_files/js/barplot-panel-handler.js

Lines 267 to 284 in aaac6e3

	// this.borderColor is always a RGB array, for the sake of everyone's
	// sanity. However, spectrum requires that the specified color is a hex
	// string: so we have to convert it to hex first here (only to later
	// convert it back to RGB on change events). Eesh!
	// A SILLY NOTE: Apparently chroma.gl() modifies the input RGB array if
	// you pass it in directly, converting it into a weird thing that
	// is represented in the browser console as "(4) [255, 255, 255,
	// undefined, _clipped: false, _unclipped: Array(3)]". Unpacking the
	// input array using ... (as done here with this.borderColor) seems to
	// avoid this problem.
	var startingColor = chroma.gl(...this.borderColor).hex();
	$(this.borderColorPicker).spectrum({
	color: startingColor,
	change: function (newColor) {
	scope.borderColor = Colorer.hex2RGB(newColor.toHexString());
	},
	});

empress/empress/support_files/js/empress.js

Lines 1840 to 1855 in aaac6e3

	var borderColor = this._barplotPanel.borderColor;
	var borderLength = this._barplotPanel.borderLength;
	var maxD = prevLayerMaxD + borderLength;
	// TODO: Should be changed when the ability to change the background
	// color is added. Basically, we get a "freebie" if the border color
	// matches the background color, and we don't need to draw anything --
	// we can just increase the displacement and leave it at that.
	// (This works out very well if this is the "outermost" border -- then
	// we really don't need to do anything.)
	if (
	borderColor[0] === 1 &&
	borderColor[1] === 1 &&
	borderColor[2] === 1
	) {
	return maxD;
	}

I haven't gone extremely thoroughly through things yet, so there may be additional places throughout the code/docs which exhibit this problem.

None of these are really urgent issues, since only one of them impacts the user directly, but I would like to try to fix as many of them as we can in advance (to avoid the master codebase having confusing docs).

For what it's worth the barplot panel handler stuff does very slightly impact the program behavior -- the "freebie" checking stuff should be changed to check if the border color is equal to exactly white (as a float), rather than the current behavior which still assumes that the color is stored as an RGB array. The consequence of this right now is that "freebies" are detected whenever the border color is unchanged (since it defaults to [1,1,1]), but even if the user changes the color and then changes it back to exactly white (16777215), no "freebie" is given since the checks still assume this is an array. This makes drawing barplot borders slower... albeit in this one, extremely tiny corner case :P

Anyway, hope this clarifies things. Happy to zoom sometime this week and go through these, or to just defer some of this work to a new issue if you'd prefer (since we have a lot of open PRs to get through). Thanks so much!

[fedarko]

Suggested change

	// So, if coords[node+2] == DEFAULT_COLOR then coords[node+2+5] will
	// also be equal to DEFAULT_COLOR. Thus, we can save checking three
	// array elements here.

I think we'd need to update this comment about the colors array -- honestly I'd be ok just removing this comment since I'm not sure if it's really applicable here.

[fedarko]

I can't comment on it directly (??? github is weird), but the docs comment for getMapRGB should be updated to mention that the colors are RGB floats not arrays