Enable restyle to redraw parcoords #3101 #3125

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

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

src/traces/parcoords/parcoords.js

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

test/jasmine/tests/config_test.js

@@ -7,6 +7,7 @@ var click = require('../assets/click');
 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

src/traces/parcoords/parcoords.js

            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 fa1a436 parcoords 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 
        
                ) { 
        
                   var msg = '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++; 
        
                       return drawFramework(); 
        
                   } 
        
               }

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

test/jasmine/tests/parcoords_test.js

+
+        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

test/jasmine/tests/parcoords_test.js

+        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

test/jasmine/tests/parcoords_test.js

+                    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:

var totalRGB; Plotly.plot(gd, mock3) .then(function() { totalRGB = _getGrayRatio(); mockCopy.data[0].dimensions[1].constraintrange = [0.4, 0.6]; return Plotly.react(gd, mockCopy); }) .then(function() { var totalRGB2 = _getGrayRatio(); expect(totalRGB2).toBe(totalRGB, 'after react'); }) .catch(failTest) .then(done);

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

package-lock.json

@@ -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

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

4bccd3f

updated syntax and comments

dc5c45a

alexcjohnson reviewed Oct 18, 2018

View reviewed changes

src/traces/parcoords/parcoords.js Outdated Show resolved Hide resolved

increased delay in config_test

ae414d3

alexcjohnson reviewed Oct 18, 2018

View reviewed changes

src/traces/parcoords/parcoords.js Show resolved Hide resolved

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 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

archmoj and 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

Enable restyle to redraw parcoords #3101 #3125

Enable restyle to redraw parcoords #3101 #3125

archmoj commented Oct 18, 2018 •

edited by etpinard

Loading

alexcjohnson commented Oct 18, 2018

alexcjohnson Oct 18, 2018

archmoj Oct 26, 2018

alexcjohnson Oct 26, 2018

archmoj commented Oct 18, 2018

alexcjohnson commented Oct 18, 2018

archmoj commented Oct 18, 2018

alexcjohnson commented Oct 19, 2018

archmoj commented Oct 19, 2018

etpinard Oct 23, 2018 •

edited

Loading

etpinard commented Oct 23, 2018

alexcjohnson commented Oct 23, 2018

archmoj commented Oct 24, 2018

etpinard commented Oct 24, 2018

archmoj commented Oct 24, 2018

etpinard commented Oct 29, 2018

archmoj commented Oct 29, 2018

etpinard Oct 29, 2018

etpinard Oct 29, 2018

etpinard Oct 29, 2018

etpinard Oct 29, 2018

etpinard commented Oct 29, 2018


		canvases.each(function(element, index) {

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

Enable restyle to redraw parcoords #3101 #3125

Enable restyle to redraw parcoords #3101 #3125

Conversation

archmoj commented Oct 18, 2018 • edited by etpinard Loading

alexcjohnson commented Oct 18, 2018

alexcjohnson Oct 18, 2018

Choose a reason for hiding this comment

archmoj Oct 26, 2018

Choose a reason for hiding this comment

alexcjohnson Oct 26, 2018

Choose a reason for hiding this comment

archmoj commented Oct 18, 2018

alexcjohnson commented Oct 18, 2018

archmoj commented Oct 18, 2018

alexcjohnson commented Oct 19, 2018

archmoj commented Oct 19, 2018

etpinard Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

etpinard commented Oct 23, 2018

alexcjohnson commented Oct 23, 2018

archmoj commented Oct 24, 2018

etpinard commented Oct 24, 2018

archmoj commented Oct 24, 2018

etpinard commented Oct 29, 2018

archmoj commented Oct 29, 2018

etpinard Oct 29, 2018

Choose a reason for hiding this comment

etpinard Oct 29, 2018

Choose a reason for hiding this comment

etpinard Oct 29, 2018

Choose a reason for hiding this comment

etpinard Oct 29, 2018

Choose a reason for hiding this comment

etpinard commented Oct 29, 2018

archmoj commented Oct 18, 2018 •

edited by etpinard

Loading

etpinard Oct 23, 2018 •

edited

Loading