Enable restyle to redraw parcoords #3101 by archmoj · Pull Request #3125 · plotly/plotly.js

archmoj · 2018-10-18T17:39:08Z

This PR enables callbacks e.g. restyle to redraw parcoords.
This fixes#3101
@alexcjohnson

alexcjohnson · 2018-10-18T19:18:50Z

Alright, at some point we should dive in and figure out what it will take to make lineLayer.update really handle arbitrary updates so they'll be faster. But in the meantime this looks like a solid fix. Only thing we need now is a test or two showing that this fix does allow us to change line color or colorscale.

alexcjohnson · 2018-10-18T19:50:41Z

 var mouseEvent = require('../assets/mouse_event');
 var failTest = require('../assets/fail_test');
 var delay = require('../assets/delay');
+var RESIZE_DELAY = 300;


cc @antoinerg - bad day for circleci it seems, we were getting repeated test failures here so I encouraged @archmoj to try increasing this delay. At some point we may want to change this so we're waiting on some event (listen for plotly_afterplot perhaps?) rather than a fixed delay, that should be more robust (and usually faster!)

@antoinerg What value you suggest we use here for RESIZE_DELAY now that the CI is back to normal?

Don't bother reducing it (if the goal is essentially to save time) - just leave it until we get around to exploring an event-based delay, rather than time-based.

archmoj · 2018-10-18T20:00:39Z

The restyle function is tested interactively. Do we still need to create a jasmine test?

alexcjohnson · 2018-10-18T20:18:47Z

Do we still need to create a jasmine test?

Yes - especially since at some point we want to improve performance by fixing update, we should write a few tests that we can rely on during that work to ensure nothing has changed.

archmoj · 2018-10-18T21:12:49Z

There was a test available for colorscale; so I added another one for line color.

alexcjohnson · 2018-10-19T01:26:41Z

Hmm, those tests only really verify the mechanics inside restyle and I guess that parcoords doesn't muck something up in gd.data during plotting. But I suspect that all of that worked just as well before your fix.

We need both of those tests to check what's actually on screen after the change, probably by using readPixel - similar to 156a9db I guess but in this case we care about more than just "are there visible pixels", we actually care about the color. Presumably something like the average color over the entire canvas is sufficient.

archmoj · 2018-10-19T01:42:42Z

Yes totally agree on that.

etpinard · 2018-10-23T19:56:57Z

 if(d.viewModel) {
- if(d.lineLayer) d.lineLayer.update(d);
- else d.lineLayer = lineLayerMaker(this, d);
+ if((!d.lineLayer) ||


That extra ( ) style feels somewhat new to our codebase. Maybe we could add https://eslint.org/docs/rules/no-extra-parens to our lintinting rules.

etpinard · 2018-10-23T19:58:33Z

If I understand correctly, PR #2415 was also the culprit for bug #3099 (fixed in #3123)?

alexcjohnson · 2018-10-23T21:26:05Z

If I understand correctly, PR #2415 was also the culprit for bug #3099 (fixed in #3123)?

Yes, most likely specifically fa1a436parcoords create/update pattern
Another instance of update performance breaking things 🙃

archmoj · 2018-10-24T01:33:52Z

@etpinard
For the case of FireFox, the program actually get into plot_api and Plots.cleanPlot([], {}, gd._fullData, fullLayout);. That explains why newFullLayout was empty on FireFox.
But when we define width & height in the layout the issue goes away.
Do we have any default values for width and height? Or are they declared by the browser/window when not being specified?

etpinard · 2018-10-24T02:44:08Z

Very interesting. FF must not like this block

plotly.js/src/plot_api/plot_api.js

Lines 261 to 279 in 7ae6fd6

if(regl){
// Unfortunately, this can happen when relayouting to large
// width/height on some browsers.
if(fullLayout.width!==regl._gl.drawingBufferWidth||
fullLayout.height!==regl._gl.drawingBufferHeight
){
varmsg='WebGL context buffer and canvas dimensions do not match due to browser/WebGL bug.';
if(drawFrameworkCalls){
Lib.error(msg);
}else{
Lib.log(msg+' Clearing graph and plotting again.');
Plots.cleanPlot([],{},gd._fullData,fullLayout);
Plots.supplyDefaults(gd);
fullLayout=gd._fullLayout;
Plots.doCalcdata(gd);
drawFrameworkCalls++;
returndrawFramework();
}
}

added in #2939

Could you check if regl._gl.drawingBufferWidth and regl._gl.drawingBufferHeight are defined in FF?

archmoj · 2018-10-24T14:06:56Z

On FF when width and height are not declared in the layout we get float values (e.g. fullLayout.width) that we shouldn't get. So they could be rounded somewhere upstream? by either floor function or another round to nearest function. What do you think @etpinard ?
fullLayout.widthXheight= [1082.05, 450]
regl._gl.drawingBufferWidthXheight= [1082, 450]

etpinard · 2018-10-29T14:00:17Z

@archmoj do you need help adding a test for this thing? This is a pretty bad bug that we should merge in before releasing 1.42.0

archmoj · 2018-10-29T14:05:17Z

Thank you! Yes actually I am working on a jasmine test for this. I would let you know.

etpinard · 2018-10-29T17:57:28Z

+
+ canvases.each(function(element, index) {
+
+ if (index == 0) { // FIXME: we assumed here that the context is the first item but may be not.


Yes, the context should always be the first one, if it's not maybe other tests will fail (not just this one). No need to for an additional check.

etpinard · 2018-10-29T17:58:52Z

+ var canvases = d3.selectAll('.gl-canvas');
+ expect(canvases.size()).toBe(3, msg);
+
+ canvases.each(function(element, index) {


... but alternatively, you could select .gl-canvas-context instead of all three <canvas> elements

etpinard · 2018-10-29T18:04:55Z

+ var b = imageArray[i][2];
+ totalRGB += r + g + b;
+ }
+ if(secondPass > 0) {


I don't like this pattern. Let's keep getGrayRatio as just a getter function and move the expect statement to down to it block below:
vartotalRGB;Plotly.plot(gd,mock3).then(function(){totalRGB=_getGrayRatio();mockCopy.data[0].dimensions[1].constraintrange=[0.4,0.6];returnPlotly.react(gd,mockCopy);}).then(function(){vartotalRGB2=_getGrayRatio();expect(totalRGB2).toBe(totalRGB,'after react');}).catch(failTest).then(done);

etpinard · 2018-10-29T18:51:11Z

@@ -17,7 +17,7 @@
 "3d-view-controls": {
 "version": "2.2.2",
 "resolved": "https://registry.npmjs.org/3d-view-controls/-/3d-view-controls-2.2.2.tgz",
- "integrity": "sha512-WL0u3PN41lEx/4qvKqV6bJlweUYoW18FXMshW/qHb41AVdZxDReLoJNGYsI7x6jf9bYelEF62BJPQmO7yEnG2w==",
+ "integrity": "sha1-cXP8GX6efk28YyE0OEZwR9vIT6I=",


Can you undo these package-lock changes? They have nothing to do with this PR.

etpinard · 2018-10-29T20:23:01Z

Now in #3178

archmoj added 2 commits October 18, 2018 13:02

fix to enable callbacks e.g. restyle to redraw
4bccd3f

updated syntax and comments
dc5c45a

alexcjohnson reviewed Oct 18, 2018
View reviewed changes

Comment threadsrc/traces/parcoords/parcoords.js Outdated

increased delay in config_test
ae414d3

alexcjohnson reviewed Oct 18, 2018
View reviewed changes

Comment threadsrc/traces/parcoords/parcoords.js

put render call outside key check
45e1043

alexcjohnson reviewed Oct 18, 2018
View reviewed changes

added test for line color restyle
5413a65

archmoj mentioned this pull request Oct 19, 2018
Parcoords issues with extra update call #3112
Closed

archmoj added bug something broken status: in progress labels Oct 21, 2018

etpinard reviewed Oct 23, 2018
View reviewed changes

archmoj added 3 commits October 29, 2018 10:19

work in progress
0e71641

new mock and test
b581c0d

tests...
b499cfc

etpinard reviewed Oct 29, 2018
View reviewed changes

archmoj added 2 commits October 29, 2018 14:25

updated tests
af231b3

jasmine test rewrite
40dba48

etpinard reviewed Oct 29, 2018
View reviewed changes

archmojand others added 2 commits October 29, 2018 15:12

latest
5f946bc

wip (parcoords channel color test)
dda4bea

etpinard mentioned this pull request Oct 29, 2018
Parcoords line.color restyle fix #3178
Merged

etpinard closed this Oct 29, 2018

etpinard deleted the issue-3101 branch October 29, 2018 20:23


		canvases.each(function(element, index) {

		if (index == 0) { // FIXME: we assumed here that the context is the first item but may be not.

Uh oh!

Uh oh!

Conversation

archmoj commented Oct 18, 2018• edited by etpinard Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexcjohnson commented Oct 18, 2018

Uh oh!

alexcjohnsonOct 18, 2018

Choose a reason for hiding this comment

Uh oh!

archmojOct 26, 2018

Choose a reason for hiding this comment

Uh oh!

alexcjohnsonOct 26, 2018

Choose a reason for hiding this comment

Uh oh!

archmoj commented Oct 18, 2018

Uh oh!

alexcjohnson commented Oct 18, 2018

Uh oh!

archmoj commented Oct 18, 2018

Uh oh!

alexcjohnson commented Oct 19, 2018

Uh oh!

archmoj commented Oct 19, 2018

Uh oh!

etpinardOct 23, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etpinard commented Oct 23, 2018

Uh oh!

alexcjohnson commented Oct 23, 2018

Uh oh!

archmoj commented Oct 24, 2018

Uh oh!

etpinard commented Oct 24, 2018

Uh oh!

archmoj commented Oct 24, 2018

Uh oh!

etpinard commented Oct 29, 2018

Uh oh!

archmoj commented Oct 29, 2018

Uh oh!

etpinardOct 29, 2018

Choose a reason for hiding this comment

Uh oh!

etpinardOct 29, 2018

Choose a reason for hiding this comment

Uh oh!

etpinardOct 29, 2018

Choose a reason for hiding this comment

Uh oh!

etpinardOct 29, 2018

Choose a reason for hiding this comment

Uh oh!

etpinard commented Oct 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

archmoj commented Oct 18, 2018•
edited by etpinard
Loading

etpinardOct 23, 2018•
edited
Loading