fix #2797 - clear full canvas and use redrawReglTraces on selections

- no longer need _module.styleOnSelect! - selections on overlaid subplots now work! - remove clearViewport (for now), we could bring it back again to optimize selections on disjoint subplots.
plotly · Oct 2, 2018 · c02330c · c02330c
1 parent 68dfbc1
commit c02330c
Show file tree

Hide file tree

Showing 6 changed files with 67 additions and 99 deletions.
diff --git a/src/plots/cartesian/select.js b/src/plots/cartesian/select.js
@@ -19,7 +19,8 @@ var polygon = require('../../lib/polygon');
 var throttle = require('../../lib/throttle');
 var makeEventData = require('../../components/fx/helpers').makeEventData;
 var getFromId = require('./axis_ids').getFromId;
-var sortModules = require('../sort_modules').sortModules;
+var clearGlCanvases = require('../../lib/clear_gl_canvases');
+var redrawReglTraces = require('../../plot_api/subroutines').redrawReglTraces;
 
 var constants = require('./constants');
 var MINSELECT = constants.MINSELECT;
@@ -663,7 +664,7 @@ function isOnlyOnePointSelected(searchTraces) {
 }
 
 function updateSelectedState(gd, searchTraces, eventData) {
-    var i, j, searchInfo, trace;
+    var i, searchInfo, cd, trace;
 
     if(eventData) {
         var pts = eventData.points || [];
@@ -696,43 +697,25 @@ function updateSelectedState(gd, searchTraces, eventData) {
         }
     }
 
-    // group searchInfo traces by trace modules
-    var lookup = {};
+    var hasRegl = false;
 
     for(i = 0; i < searchTraces.length; i++) {
         searchInfo = searchTraces[i];
+        cd = searchInfo.cd;
+        trace = cd[0].trace;
 
-        var name = searchInfo._module.name;
-        if(lookup[name]) {
-            lookup[name].push(searchInfo);
-        } else {
-            lookup[name] = [searchInfo];
+        if(Registry.traceIs(trace, 'regl')) {
+            hasRegl = true;
         }
+
+        var _module = searchInfo._module;
+        var fn = _module.styleOnSelect || _module.style;
+        if(fn) fn(gd, cd);
     }
 
-    var keys = Object.keys(lookup).sort(sortModules);
-
-    for(i = 0; i < keys.length; i++) {
-        var items = lookup[keys[i]];
-        var len = items.length;
-        var item0 = items[0];
-        var trace0 = item0.cd[0].trace;
-        var _module = item0._module;
-        var styleSelection = _module.styleOnSelect || _module.style;
-
-        if(Registry.traceIs(trace0, 'regl')) {
-            // plot regl traces per module
-            var cds = new Array(len);
-            for(j = 0; j < len; j++) {
-                cds[j] = items[j].cd;
-            }
-            styleSelection(gd, cds);
-        } else {
-            // plot svg trace per trace
-            for(j = 0; j < len; j++) {
-                styleSelection(gd, items[j].cd);
-            }
-        }
+    if(hasRegl) {
+        clearGlCanvases(gd);
+        redrawReglTraces(gd);
     }
 }
 

diff --git a/src/traces/scattergl/index.js b/src/traces/scattergl/index.js
@@ -294,18 +294,6 @@ function sceneUpdate(gd, subplot) {
             scene.dirty = false;
         };
 
-        scene.clear = function clear() {
-            var vp = getViewport(gd._fullLayout, subplot.xaxis, subplot.yaxis);
-
-            if(scene.select2d) {
-                clearViewport(scene.select2d, vp);
-            }
-
-            var anyComponent = scene.scatter2d || scene.line2d ||
-                (scene.glText || [])[0] || scene.fill2d || scene.error2d;
-            if(anyComponent) clearViewport(anyComponent, vp);
-        };
-
         // remove scene resources
         scene.destroy = function destroy() {
             if(scene.fill2d) scene.fill2d.destroy();
@@ -357,14 +345,6 @@ function getViewport(fullLayout, xaxis, yaxis) {
     ];
 }
 
-function clearViewport(comp, vp) {
-    var gl = comp.regl._gl;
-    gl.enable(gl.SCISSOR_TEST);
-    gl.scissor(vp[0], vp[1], vp[2] - vp[0], vp[3] - vp[1]);
-    gl.clearColor(0, 0, 0, 0);
-    gl.clear(gl.COLOR_BUFFER_BIT);
-}
-
 function plot(gd, subplot, cdata) {
     if(!cdata.length) return;
 
@@ -889,19 +869,6 @@ function selectPoints(searchInfo, selectionTester) {
     return selection;
 }
 
-function styleOnSelect(gd, cds) {
-    var stash = cds[0][0].t;
-    var scene = stash._scene;
-
-    // don't clear the subplot if there are splom traces
-    // on the graph
-    if(!gd._fullLayout._has('splom')) {
-        scene.clear();
-    }
-
-    scene.draw();
-}
-
 function styleTextSelection(cd) {
     var cd0 = cd[0];
     var stash = cd0.t;
@@ -951,7 +918,6 @@ module.exports = {
     calc: calc,
     plot: plot,
     hoverPoints: hoverPoints,
-    styleOnSelect: styleOnSelect,
     selectPoints: selectPoints,
 
     sceneOptions: sceneOptions,

diff --git a/src/traces/scatterpolargl/index.js b/src/traces/scatterpolargl/index.js
@@ -183,7 +183,6 @@ module.exports = {
     calc: calc,
     plot: plot,
     hoverPoints: hoverPoints,
-    styleOnSelect: ScatterGl.styleOnSelect,
     selectPoints: ScatterGl.selectPoints,
 
     meta: {

diff --git a/src/traces/splom/index.js b/src/traces/splom/index.js
@@ -438,31 +438,6 @@ function selectPoints(searchInfo, selectionTester) {
     return selection;
 }
 
-function styleOnSelect(gd, cds) {
-    var fullLayout = gd._fullLayout;
-    var cd0 = cds[0];
-    var scene0 = fullLayout._splomScenes[cd0[0].trace.uid];
-    scene0.matrix.regl.clear({color: true, depth: true});
-
-    if(fullLayout._splomGrid) {
-        fullLayout._splomGrid.draw();
-    }
-
-    for(var i = 0; i < cds.length; i++) {
-        var scene = fullLayout._splomScenes[cds[i][0].trace.uid];
-        scene.draw();
-    }
-
-    // redraw all subplot with scattergl traces,
-    // as we cleared the whole canvas above
-    if(fullLayout._has('cartesian')) {
-        for(var k in fullLayout._plots) {
-            var sp = fullLayout._plots[k];
-            if(sp._scene) sp._scene.draw();
-        }
-    }
-}
-
 function getDimIndex(trace, ax) {
     var axId = ax._id;
     var axLetter = axId.charAt(0);
@@ -491,7 +466,6 @@ module.exports = {
     plot: plot,
     hoverPoints: hoverPoints,
     selectPoints: selectPoints,
-    styleOnSelect: styleOnSelect,
     editStyle: editStyle,
 
     meta: {

diff --git a/test/jasmine/tests/gl2d_click_test.js b/test/jasmine/tests/gl2d_click_test.js
@@ -1135,4 +1135,47 @@ describe('@noCI Test gl2d lasso/select:', function() {
         .catch(failTest)
         .then(done);
     });
+
+    it('should work on overlaid subplots', function(done) {
+        gd = createGraphDiv();
+
+        var scene, scene2;
+
+        Plotly.plot(gd, [{
+            x: [1, 2, 3],
+            y: [40, 50, 60],
+            type: 'scattergl',
+            mode: 'markers'
+        }, {
+            x: [2, 3, 4],
+            y: [4, 5, 6],
+            yaxis: 'y2',
+            type: 'scattergl',
+            mode: 'markers'
+        }], {
+            xaxis: {domain: [0.2, 1]},
+            yaxis2: {overlaying: 'y', side: 'left', position: 0},
+            showlegend: false,
+            margin: {l: 0, t: 0, b: 0, r: 0},
+            width: 400,
+            height: 400,
+            dragmode: 'select'
+        })
+        .then(delay(100))
+        .then(function() {
+            scene = gd._fullLayout._plots.xy._scene;
+            scene2 = gd._fullLayout._plots.xy2._scene;
+
+            spyOn(scene.scatter2d, 'draw');
+            spyOn(scene2.scatter2d, 'draw');
+        })
+        .then(function() { return select([[20, 20], [380, 250]]); })
+        .then(function() {
+            expect(scene.scatter2d.draw).toHaveBeenCalledTimes(1);
+            expect(scene2.scatter2d.draw).toHaveBeenCalledTimes(1);
+        })
+        .catch(failTest)
+        .then(done);
+
+    });
 });
diff --git a/test/jasmine/tests/splom_test.js b/test/jasmine/tests/splom_test.js
@@ -1298,23 +1298,26 @@ describe('Test splom select:', function() {
         var cnt = 0;
         var scatterGlCnt = 0;
         var splomCnt = 0;
+        var scatterglScene, splomScene;
 
         Plotly.newPlot(gd, fig).then(function() {
-            // 'scattergl' trace module
-            spyOn(gd._fullLayout._modules[0], 'styleOnSelect').and.callFake(function() {
+            var fullLayout = gd._fullLayout;
+            scatterglScene = fullLayout._plots.xy._scene;
+            splomScene = fullLayout._splomScenes[gd._fullData[1].uid];
+
+            spyOn(scatterglScene, 'draw').and.callFake(function() {
                 cnt++;
                 scatterGlCnt = cnt;
             });
-            // 'splom' trace module
-            spyOn(gd._fullLayout._modules[1], 'styleOnSelect').and.callFake(function() {
+            spyOn(splomScene, 'draw').and.callFake(function() {
                 cnt++;
                 splomCnt = cnt;
             });
         })
         .then(function() { return _select([[20, 395], [195, 205]]); })
         .then(function() {
-            expect(gd._fullLayout._modules[0].styleOnSelect).toHaveBeenCalledTimes(1);
-            expect(gd._fullLayout._modules[1].styleOnSelect).toHaveBeenCalledTimes(1);
+            expect(scatterglScene.draw).toHaveBeenCalledTimes(1);
+            expect(splomScene.draw).toHaveBeenCalledTimes(1);
 
             expect(cnt).toBe(2);
             expect(splomCnt).toBe(1, 'splom redraw before scattergl');