Introduce global contexts #2159

dy · 2017-11-14T20:33:46Z

Excerpt from #1869.
Covers #1993 and #1989.

dy · 2017-11-15T01:24:07Z

Apparently there are some IE9-related issues with regl.

etpinard · 2017-11-15T14:25:29Z

src/plot_api/plot_api.js

@@ -42,6 +42,8 @@ var enforceAxisConstraints = axisConstraints.enforce;
 var cleanAxisConstraints = axisConstraints.clean;
 var axisIds = require('../plots/cartesian/axis_ids');

+var createRegl = require('regl');


Ha. regl must be invoking one or many globals that IE9 doesn't support at runtime.

As this file here plot_api.js is required in all partial bundles (even that gl-less ones), this makes the IE9 test fail.

So, I think the best to solve this would be use a Registry.getComponentMethod-style pattern so that regl is only required in bundles that expose regl traces.

@etpinard do I get it right that we don't have this safe-require pattern yet? What if I just require regl directly when the regl trace type is detected?

What if I just require regl directly when the regl trace type is detected?

You mean something like:

if(fullLayout._has('regl')) { var createREGL = require('regl') }

in plot_api.js? That could work, but regl would still get bundled up in all partial bundles.

etpinard · 2017-11-16T14:37:24Z

src/plot_api/plot_api.js

@@ -2754,7 +2788,7 @@ function makePlotFramework(gd) {
        .classed('plotly', true);

    // Make the svg container
-    fullLayout._paperdiv = fullLayout._container.selectAll('.svg-container').data([0]);
+    fullLayout._paperdiv = fullLayout._container.selectAll('.svg-container').data([{}]);


Why change [0] to [{}]?

etpinard · 2017-11-16T14:39:22Z

src/plots/plots.js

        var _module = basePlotModules[i];

        if(_module.name === category) return true;
    }

+    // check trace
+    var modules = this._modules || [];


brilliant ✨

etpinard · 2017-11-16T14:39:54Z

src/traces/parcoords/base_plot.js

+            x: 0,
+            y: 0,
+            width: canvas.width,
+            height: canvas.height,


Lovely cleanup ❤️

etpinard · 2017-11-17T15:45:22Z

src/plot_api/plot_api.js

@@ -2765,10 +2799,13 @@ function makePlotFramework(gd) {
    // TODO: sort out all the ordering so we don't have to
    // explicitly delete anything
    fullLayout._glcontainer = fullLayout._paperdiv.selectAll('.gl-container')
-        .data([0]);
+        .data([{}]);


what about this one here?

that is the way parcoords is doing it's thing: https://github.com/plotly/plotly.js/pull/2159/files#diff-2fad23de82bf8c7625ea8e11436233b7R290
I did not really try to figure out why it is done this and not the another way. Before it used own container for storing this data. If that matters, I can try to keep this object within parcoords use.

Does it work ok with data([0])?

@etpinard that is the point that parcoords puts vm property there via extendFlat, that is why it should be an object. Probably it is possible to find a better storage for it, I just did not figure out what.

etpinard · 2017-11-17T15:54:40Z

src/plot_api/plot_api.js

+                key: 'pickLayer',
+                context: false,
+                pick: true
+            }]);


I think you need a key-function here to make sure to not create these canvas nodes on every hard redraw that go through drawFramework.

Sometime like:

fullLayout._glcanvas = fullLayout._glcontainer.selectAll('.gl-canvas').data([{ /* .. as currently ... */ }], function(d) { return d.key; });

This makes me wonder: we probably don't have sufficient test coverage for Plotly.restyle calls that morph a gl-based trace types into a SVG-based. For example off the regl2d branch we should have:

Plotly.newPlot(gd, [{ type: 'scattergl', // ... }]) .then(function() { d3.selectAll('canvas').size() // => 3 return Plotly.restyle(gd, 'type', 'scatter') }) .then(function() { d3.selectAll('canvas').size() // => 0 })

As restyling a parcoords traces into any other trace types doesn't make much sense at the moment. You could instead try something like:

Plotly.newPlot(gd, [{ type: 'parcoords', // ... }]) .then(function() { d3.selectAll('canvas').size() // => 3 return Plotly.deleteTraces(gd, [0]) }) .then(function() { d3.selectAll('canvas').size() // => 0 })

etpinard · 2017-11-24T01:12:27Z

src/plots/plots.js

+
+    if(hadGl && !hasGl) {
+        if(oldFullLayout._glcontainer !== undefined) {
+            oldFullLayout._glcontainer.selectAll('.gl-canvas').data([]).exit().remove();


oldFullLayout._glcontainer.selectAll('.gl-canvas').remove()

should suffice.

etpinard · 2017-11-27T18:20:48Z

@dfcreative Good news the parcoords tests are passing again

Bad news other @noCI tests are failing.

Can you try making npm run test-jasmine -- --tags=noCI pass?

dy · 2017-11-28T00:12:49Z

@etpinard fixed these four noCI tests, seems that others (mapbox ones) are not related to this PR but are specific to my machine.

etpinard · 2017-11-28T15:39:40Z

seems that others (mapbox ones) are not related to this PR but are specific to my machine.

Yeah, those mapbox tests are pretty resource-intensive; that doesn't surprise me. Hopefully #2029 will fix those issues.

Great PR. Merge away 💃

Introduce global contexts

e6fcbf5

dy requested a review from etpinard November 14, 2017 20:34

Fix canvas counts

f711cf1

etpinard reviewed Nov 15, 2017

View reviewed changes

dy added 3 commits November 15, 2017 16:40

Scope regl creation by trace

0838c31

Fix interaction/image tests

348e170

Lintify

07ea384

etpinard reviewed Nov 16, 2017

View reviewed changes

src/traces/parcoords/base_plot.js

x: 0,

y: 0,

width: canvas.width,

height: canvas.height,

Copy link

Contributor

etpinard Nov 16, 2017

Choose a reason for hiding this comment

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

Lovely cleanup ❤️

Remove data obj

1517ece

etpinard reviewed Nov 17, 2017

View reviewed changes

This was referenced Nov 17, 2017

Shared canvases #1993

Closed

Cross-chart context management #1989

Closed

etpinard added status: in progress labels Nov 17, 2017

finger563 mentioned this pull request Nov 22, 2017

When using WebGL backend for results-viz only a limited number of plots can be shown rosmod/webgme-rosmod#196

Closed

dy added 3 commits November 22, 2017 17:39

Merge branch 'master' into global-context

f7fc8cb

Recycle global canvases properly

384b577

Fix container test

8346ce4

etpinard reviewed Nov 24, 2017

View reviewed changes

dy added 3 commits November 24, 2017 14:03

Simplify canvas removal

b8b8951

Minor fixes

4efe21e

Fix parcoords tests

83b7589

etpinard added status: reviewable and removed status: in progress labels Nov 27, 2017

Fix noCI tests

de9b1aa

dy merged commit accae1a into master Nov 28, 2017

dy deleted the global-context branch November 28, 2017 16:02

alexcjohnson mentioned this pull request Dec 22, 2017

multiple gl2d subplots: only one is drawn at a time #2220

Closed

alexcjohnson mentioned this pull request Mar 4, 2018

gl2d zoom turns background black #2436

Closed

archmoj mentioned this pull request May 30, 2019

data missing when multiple parcoords traces #3361

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce global contexts #2159

Introduce global contexts #2159

dy commented Nov 14, 2017

dy commented Nov 15, 2017

etpinard Nov 15, 2017 •

edited

Loading

dy Nov 15, 2017

etpinard Nov 15, 2017 •

edited

Loading

etpinard Nov 16, 2017

etpinard Nov 16, 2017

etpinard Nov 16, 2017

etpinard Nov 17, 2017

dy Nov 22, 2017 •

edited

Loading

etpinard Nov 22, 2017

dy Nov 22, 2017 •

edited

Loading

etpinard Nov 17, 2017

etpinard Nov 17, 2017

etpinard Nov 24, 2017

etpinard commented Nov 27, 2017

dy commented Nov 28, 2017

etpinard commented Nov 28, 2017

Introduce global contexts #2159

Introduce global contexts #2159

Conversation

dy commented Nov 14, 2017

dy commented Nov 15, 2017

etpinard Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dy Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dy Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Nov 27, 2017

dy commented Nov 28, 2017

etpinard commented Nov 28, 2017

etpinard Nov 15, 2017 •

edited

Loading

etpinard Nov 15, 2017 •

edited

Loading

dy Nov 22, 2017 •

edited

Loading

dy Nov 22, 2017 •

edited

Loading