Skip to content

Even better cartesian trace updates and removal #2579

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Apr 30, 2018
Merged

Even better cartesian trace updates and removal #2579

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
400bfe6
ping imagetest after having restarting nw.js
etpinard Apr 24, 2018
284c206
introduce new strategy for cartesian trace module layer updates
etpinard Apr 24, 2018
da63ed4
make heatmap/contour(carpet)/carpet plot method remove its own trace
etpinard Apr 24, 2018
a1c76bc
sub imagelayer->heatmaplayer, maplayer->contourlayer in tests
etpinard Apr 24, 2018
fa38ca5
update `cliponaxis: false`
etpinard Apr 24, 2018
48ee0d8
:lock: contour with heatmap coloring layer order
etpinard Apr 24, 2018
362da1f
:lock: scattercarpet + scatter coexistence
etpinard Apr 24, 2018
3bef9a9
add getUidsFromCalcData helper
etpinard Apr 30, 2018
d47276f
make visible trace module -> layer data a 1D loop w/ + indexOf + sort
etpinard Apr 30, 2018
83422f7
skip gl tags in flaky-tagged jasmine test cmd
etpinard Apr 30, 2018
8c9a7bf
Revert "skip gl tags in flaky-tagged jasmine test cmd"
etpinard Apr 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 3 additions & 24 deletions src/plot_api/subroutines.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,34 +488,13 @@ exports.doCamera = function(gd) {
exports.drawData = function(gd) {
var fullLayout = gd._fullLayout;
var calcdata = gd.calcdata;
var rangesliderContainers = fullLayout._infolayer.selectAll('g.rangeslider-container');
var i;

// in case of traces that were heatmaps or contour maps
// previously, remove them and their colorbars explicitly
// remove old colorbars explicitly
for(i = 0; i < calcdata.length; i++) {
var trace = calcdata[i][0].trace;
var isVisible = (trace.visible === true);
var uid = trace.uid;

if(!isVisible || !Registry.traceIs(trace, '2dMap')) {
var query = (
'.hm' + uid +
',.contour' + uid +
',#clip' + uid
);

fullLayout._paper
.selectAll(query)
.remove();

rangesliderContainers
.selectAll(query)
.remove();
}

if(!isVisible || !trace._module.colorbar) {
fullLayout._infolayer.selectAll('.cb' + uid).remove();
if(trace.visible !== true || !trace._module.colorbar) {
fullLayout._infolayer.select('.cb' + trace.uid).remove();
}
}

Expand Down
25 changes: 4 additions & 21 deletions src/plots/plots.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,8 +672,6 @@ plots.cleanPlot = function(newFullData, newFullLayout, oldFullData, oldFullLayou
}
}

var hasPaper = !!oldFullLayout._paper;
var hasInfoLayer = !!oldFullLayout._infolayer;
var hadGl = oldFullLayout._has && oldFullLayout._has('gl');
var hasGl = newFullLayout._has && newFullLayout._has('gl');

Expand All @@ -684,6 +682,8 @@ plots.cleanPlot = function(newFullData, newFullLayout, oldFullData, oldFullLayou
}
}

var hasInfoLayer = !!oldFullLayout._infolayer;

oldLoop:
for(i = 0; i < oldFullData.length; i++) {
var oldTrace = oldFullData[i],
Expand All @@ -695,26 +695,9 @@ plots.cleanPlot = function(newFullData, newFullLayout, oldFullData, oldFullLayou
if(oldUid === newTrace.uid) continue oldLoop;
}

var query = (
'.hm' + oldUid +
',.contour' + oldUid +
',.carpet' + oldUid +
',#clip' + oldUid +
',.trace' + oldUid
);

// clean old heatmap, contour traces and clip paths
// that rely on uid identifiers
if(hasPaper) {
oldFullLayout._paper.selectAll(query).remove();
}

// clean old colorbars and range slider plot
// clean old colorbars
if(hasInfoLayer) {
oldFullLayout._infolayer.selectAll('.cb' + oldUid).remove();

oldFullLayout._infolayer.selectAll('g.rangeslider-container')
.selectAll(query).remove();
oldFullLayout._infolayer.select('.cb' + oldUid).remove();
}
}

Expand Down
21 changes: 13 additions & 8 deletions src/traces/carpet/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,20 @@ var svgTextUtils = require('../../lib/svg_text_utils');
var Lib = require('../../lib');
var alignmentConstants = require('../../constants/alignment');

module.exports = function plot(gd, plotinfo, cdcarpet) {
if(!cdcarpet.length) {
plotinfo.plot.select('.carpetlayer')
.selectAll('g.trace')
.remove();
}
module.exports = function plot(gd, plotinfo, cdcarpet, carpetLayer) {
var i;

carpetLayer.selectAll('g.trace').each(function() {
var classString = d3.select(this).attr('class');
var oldUid = classString.split('carpet')[1].split(/\s/)[0];
for(i = 0; i < cdcarpet.length; i++) {
if(oldUid === cdcarpet[i][0].trace.uid) return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

🌴 🐎 perhaps make a common helper getUidsFromCalcdata or something, that can be called once up front and returns a hash {uid0: 1, uid1: 1, ...}, then if(!uidHash[oldUid]) d3.select(this).remove();?
Looks like the same helper would work for all modules in this commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in d47276f

d3.select(this).remove();
});

for(var i = 0; i < cdcarpet.length; i++) {
plotOne(gd, plotinfo, cdcarpet[i]);
for(i = 0; i < cdcarpet.length; i++) {
plotOne(gd, plotinfo, cdcarpet[i], carpetLayer);
}
};

Expand Down
15 changes: 12 additions & 3 deletions src/traces/contour/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,18 @@ var constants = require('./constants');
var costConstants = constants.LABELOPTIMIZER;


exports.plot = function plot(gd, plotinfo, cdcontours) {
for(var i = 0; i < cdcontours.length; i++) {
plotOne(gd, plotinfo, cdcontours[i]);
exports.plot = function plot(gd, plotinfo, cdcontours, contourLayer) {
var i;

contourLayer.selectAll('g.contour').each(function(d) {
for(i = 0; i < cdcontours.length; i++) {
if(d.trace.uid === cdcontours[i][0].trace.uid) return;
}
d3.select(this).remove();
});

for(i = 0; i < cdcontours.length; i++) {
plotOne(gd, plotinfo, cdcontours[i], contourLayer);
}
};

Expand Down
21 changes: 12 additions & 9 deletions src/traces/contourcarpet/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,18 @@ var lookupCarpet = require('../carpet/lookup_carpetid');
var closeBoundaries = require('../contour/close_boundaries');


module.exports = function plot(gd, plotinfo, cdcontours) {
for(var i = 0; i < cdcontours.length; i++) {
plotOne(gd, plotinfo, cdcontours[i]);
module.exports = function plot(gd, plotinfo, cdcontours, contourcarpetLayer) {
var i;

contourcarpetLayer.selectAll('g.contour').each(function(d) {
for(i = 0; i < cdcontours.length; i++) {
if(d.trace.uid === cdcontours[i][0].trace.uid) return;
}
d3.select(this).remove();
});

for(i = 0; i < cdcontours.length; i++) {
plotOne(gd, plotinfo, cdcontours[i], contourcarpetLayer);
}
};

Expand All @@ -46,7 +55,6 @@ function plotOne(gd, plotinfo, cd, contourcarpetLayer) {
var uid = trace.uid;
var xa = plotinfo.xaxis;
var ya = plotinfo.yaxis;
var fullLayout = gd._fullLayout;
var id = 'contour' + uid;
var pathinfo = emptyPathinfo(contours, plotinfo, cd[0]);
var isConstraint = contours.type === 'constraint';
Expand All @@ -59,11 +67,6 @@ function plotOne(gd, plotinfo, cd, contourcarpetLayer) {
return [xa.c2p(pt[0]), ya.c2p(pt[1])];
}

if(trace.visible !== true) {
fullLayout._infolayer.selectAll('.cb' + uid).remove();
return;
}

// Define the perimeter in a/b coordinates:
var perimeter = [
[a[0], b[b.length - 1]],
Expand Down
31 changes: 14 additions & 17 deletions src/traces/heatmap/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

'use strict';

var d3 = require('d3');
var tinycolor = require('tinycolor2');

var Registry = require('../../registry');
Expand All @@ -18,32 +19,28 @@ var xmlnsNamespaces = require('../../constants/xmlns_namespaces');

var maxRowLength = require('./max_row_length');

module.exports = function(gd, plotinfo, cdheatmaps, heatmapLayer) {
var i;

module.exports = function(gd, plotinfo, cdheatmaps) {
for(var i = 0; i < cdheatmaps.length; i++) {
plotOne(gd, plotinfo, cdheatmaps[i]);
heatmapLayer.selectAll('.hm > image').each(function(d) {
var oldTrace = d.trace || {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

when is there no d.trace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when a heatmap trace is completely off screen:

plotly.js/src/traces/heatmap/plot.js

Lines 138 to 142 in bab2a5b

var isOffScreen = (imageWidth <= 0 || imageHeight <= 0);
var plotgroup = plotinfo.plot.select('.imagelayer')
.selectAll('g.hm.' + id)
.data(isOffScreen ? [] : [0]);

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not a big deal, but wouldn't an off-screen heatmap just not contribute to the '.hm > image' selection at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removing that || {} makes

plotly.js/test/jasmine/tests/heatmap_test.js

Lines 482 to 506 in c0b38da

it('should not draw traces that are off-screen', function(done) {
var mock = require('@mocks/heatmap_multi-trace.json'),
mockCopy = Lib.extendDeep({}, mock),
gd = createGraphDiv();
function assertImageCnt(cnt) {
var images = d3.selectAll('.hm').select('image');
expect(images.size()).toEqual(cnt);
}
Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(function() {
assertImageCnt(5);
return Plotly.relayout(gd, 'xaxis.range', [2, 3]);
}).then(function() {
assertImageCnt(2);
return Plotly.relayout(gd, 'xaxis.autorange', true);
}).then(function() {
assertImageCnt(5);
done();
});
});

fail with

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... I didn't investigate further.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright, not what my quick glance at the code suggested but you're right, no need to dig in any further.

for(i = 0; i < cdheatmaps.length; i++) {
if(oldTrace.uid === cdheatmaps[i][0].trace.uid) return;
}
d3.select(this.parentNode).remove();
});

for(i = 0; i < cdheatmaps.length; i++) {
plotOne(gd, plotinfo, cdheatmaps[i], heatmapLayer);
}
};

function plotOne(gd, plotinfo, cd, heatmapLayer) {
var cd0 = cd[0];
var trace = cd0.trace;
var uid = trace.uid;
var xa = plotinfo.xaxis;
var ya = plotinfo.yaxis;
var fullLayout = gd._fullLayout;
var id = 'hm' + uid;

// in case this used to be a contour map
fullLayout._paper.selectAll('.contour' + uid).remove();
fullLayout._infolayer.selectAll('g.rangeslider-container')
.selectAll('.contour' + uid).remove();

if(trace.visible !== true) {
fullLayout._paper.selectAll('.' + id).remove();
fullLayout._infolayer.selectAll('.cb' + uid).remove();
return;
}
var id = 'hm' + trace.uid;

var z = cd0.z;
var x = cd0.x;
Expand Down