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

Refactor parcoords - bug fixes - reuse axes ticktext - support pseudo html labels - features for label angle & position #3966

Merged
merged 70 commits into from Jun 27, 2019

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Jun 15, 2019

Refactoring parcoords code helped fixing:
#3794
#3946

Reusing Cartesian axes tickText function for linear axes (parcoords has linear and ordinal axis types)
#3930

Support for pseudo-html and MathJax formatting in parcoords dimension labels
#3206

And label line breaks useful for:
https://github.com/plotly/phoenix-integration/issues/242
Also two new attributes are added at trace data level to help control the angle and position of labels

The 3 vertex shader programs as well as 2 helper functions are all combined in one vertex shader & optimised.
Also to mention low precision flag in fragment shader is replaced by high precision flag for better output on different devices.

To investigate performance benchmarks please visit the examples below and find the console.logs after refresh!
1050ms codepen 1 before
0700ms codepen 1 after

codepen 2 before
codepen 2 after

@plotly/plotly_js

…p - avoid casting int to float - reduce argument passing - etc
remove parcoords high & low limit tweakings
@@ -348,6 +350,17 @@ function parcoordsInteractionState() {
};
}

function calcTilt(angle) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this look like for multi-line labels?

Cartesian labels have this:

// how much to shift a multi-line label to center it vertically.
var nLines = svgTextUtils.lineCount(thisLabel);
var lineHeight = LINE_SPACING * d.fontSize;
var anchorHeight = labelFns.heightFn(d, isNumeric(angle) ? +angle : 0, (nLines - 1) * lineHeight);
if(anchorHeight) {
transform += ' translate(0, ' + anchorHeight + ')';
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the moment the labels are positioned using the first line.
This is pretty cool! I may give that a try on this PR.

@@ -19,7 +19,37 @@
},
"dimensions": [
{
"label": "$\\varphi ={\\frac {1+{\\sqrt {5}}}{2}}$",
"label": "<i>Rien ne se perd,<br>rien ne se crée,<br>tout se <b>transforme</b>.</i><sub>",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. So parcoords trace with MathJax text don't render ok on CI?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually they do render on the CI. But I noticed the mock often fails locally when testing multiple parcoords. By the way there could be an issue with a callback required for MathJax.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. That's weird. Looks like you're using convertToTspans correctly - which should be filling in gd._promises.

I suspect the problem is in here:

exports.toSVG = function(gd) {
var imageRoot = gd._fullLayout._glimages;
var root = d3.select(gd).selectAll('.svg-container');
var canvases = root.filter(function(d, i) {return i === root.size() - 1;})
.selectAll('.gl-canvas-context, .gl-canvas-focus');
function canvasToImage() {
var canvas = this;
var imageData = canvas.toDataURL('image/png');
var image = imageRoot.append('svg:image');
image.attr({
xmlns: xmlnsNamespaces.svg,
'xlink:href': imageData,
preserveAspectRatio: 'none',
x: 0,
y: 0,
width: canvas.width,
height: canvas.height
});
}
canvases.each(canvasToImage);
// Chrome / Safari bug workaround - browser apparently loses connection to the defined pattern
// Without the workaround, these browsers 'lose' the filter brush styling (color etc.) after a snapshot
// on a subsequent interaction.
// Firefox works fine without this workaround
window.setTimeout(function() {
d3.selectAll('#filterBarPattern')
.attr('id', 'filterBarPattern');
}, 60);
};

@@ -579,7 +583,9 @@ module.exports = function(gd, svg, parcoordsLineLayers, cdModule, layout, callba
.tickValues(d.ordinal ? // and this works for ordinal scales
sdom :
null)
.tickFormat(d.ordinal ? function(d) { return d; } : function(d) { return linearFormat(d); })
.tickFormat(function(v) {
return d.ordinal ? v : linearFormat(v, d.tickFormat);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@archmoj I'm a little concerned with the image diff for gl2d_parcoords_rgba_colorscale

Peek 2019-06-17 10-31

I don't understand why those m suffixes are now showing up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revised in 11b9434.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. This now shows up as:

image

this seems odd as the values are:

image

is that expected with the parcoords .3s tickformat default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default is 3s not .3s

tickformat: {
valType: 'string',
dflt: '3s',

Does that make sense?

Copy link
Contributor

@etpinard etpinard Jun 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, well there's nothing wrong with your code:

image

(using http://bl.ocks.org/zanarmstrong/05c1e95bf7aa16c4768e)

I'm not sure what the m stands for here though.


Maybe it's time to ditch the (bad) tickformat default altogether.

@archmoj can you generate try re-generating all the parcoords baselines w/o that tickformat default on a separate branch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m==milli - but yeah, we shouldn't use that by default - 0.003 is much better than 3m. If we use the Axes.tickText default formatting it skips milli but does use all the other SI prefixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revised in a80bf49.

@@ -1762,7 +1762,7 @@ describe('Plotly.react and uirevision attributes', function() {
_run(fig, editEditable, checkAttrs(true), checkAttrs).then(done);
});

it('@gl preserves editable: true name, colorbar title and parcoords constraint range via trace.uirevision', function(done) {
it('@noCI @gl preserves editable: true name, colorbar title and parcoords constraint range via trace.uirevision', function(done) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one bothers me. Let's spend some time to try find out why it fails now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@archmoj I was able to make this test pass again in f15c622

Feel free to cherry-pick my commit onto your branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etpinard thanks very much for the help.
Done in 2228339.

@etpinard
Copy link
Contributor

Awesome work @archmoj

The only blocking item keeping this PR from getting merged in that new @noCI tag in plot_api_react_test.js.


I'll see if I can figure that mystery out.

@etpinard
Copy link
Contributor

💃 -- a new parcoords era has begun.

@archmoj archmoj merged commit 3d383b7 into master Jun 27, 2019
@archmoj archmoj deleted the refactor-parcoords-bug-fixes-labelangle-side branch June 27, 2019 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants