Skip to content
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.

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

Add svg image layer for heatmaps #194

Merged
merged 10 commits into from
Jan 29, 2016
2 changes: 2 additions & 0 deletions devtools/test_dashboard/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
return graphDiv;
}
};

var d3 = Plotly.d3;
</script>

</body>
Expand Down
22 changes: 16 additions & 6 deletions src/plot_api/plot_api.js
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ Plotly.plot = function(gd, data, layout, config) {
subplots = Plots.getSubplotIds(fullLayout, 'cartesian'),
modules = gd._modules;

var i, j, cd, trace, uid, subplot, subplotInfo,
var i, j, trace, subplot, subplotInfo,
cdSubplot, cdError, cdModule, _module;

function getCdSubplot(calcdata, subplot) {
Expand Down Expand Up @@ -293,12 +293,21 @@ Plotly.plot = function(gd, data, layout, config) {
// in case of traces that were heatmaps or contour maps
// previously, remove them and their colorbars explicitly
for (i = 0; i < calcdata.length; i++) {
cd = calcdata[i];
trace = cd[0].trace;
if (trace.visible !== true || !trace._module.colorbar) {
trace = calcdata[i][0].trace;

var isVisible = (trace.visible === true),
uid = trace.uid;
fullLayout._paper.selectAll('.hm'+uid+',.contour'+uid+',.cb'+uid+',#clip'+uid)
.remove();

if(!isVisible || !Plots.traceIs(trace, '2dMap')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @alexcjohnson

I'm hoping to clean up a lot of the selectAll().remove() patterns in subplotly.js, but for now this fixes the heatmap to scatter restyle bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a benefit to setting isVisible vs using trace.visible? It's not a computed value, both ways are nearly the same amount of characters to type, and it avoids adding another piece of information to track in your head while reading.

Copy link
Contributor

Choose a reason for hiding this comment

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

we have to compare trace.visible === true, because visible can also be set to 'legendonly'.

So I thought of using isVisible instead of having two trace.visible === true checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eughhhhhh. Mixed 'types' 👃.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup, it's a band-aid and always has been... but it'll do for now 👍

fullLayout._paper.selectAll(
'.hm' + uid +
',.contour' + uid +
',#clip' + uid
).remove();
}

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

Expand Down Expand Up @@ -2654,6 +2663,7 @@ function makeCartesianPlotFramwork(gd, subplots) {
// 4. scatter
// 5. box plots
function plotLayers(svg) {
svg.append('g').classed('imagelayer', true);
Copy link
Contributor

Choose a reason for hiding this comment

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

@mdtusz the imagelayer stays.

svg.append('g').classed('maplayer', true);
svg.append('g').classed('barlayer', true);
svg.append('g').classed('errorlayer', true);
Expand Down
20 changes: 12 additions & 8 deletions src/traces/contour/plot.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,23 +64,24 @@ function plotOne(gd, plotinfo, cd) {
pathinfo = emptyPathinfo(contours, plotinfo, cd[0]);

if(trace.visible !== true) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we prefer this, or !trace.visible?

Copy link
Contributor

Choose a reason for hiding this comment

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

as above, we need to check again true, not truthy values as visible can be set to 'legendonly'.

fullLayout._paper.selectAll('.'+id+',.cb'+uid+',.hm'+uid).remove();
fullLayout._paper.selectAll('.' + id + ',.hm' + uid).remove();
fullLayout._infolayer.selectAll('.cb' + uid).remove();
return;
}

// use a heatmap to fill - draw it behind the lines
if(contours.coloring==='heatmap') {
if(trace.zauto && trace.autocontour===false) {
if(contours.coloring === 'heatmap') {
if(trace.zauto && (trace.autocontour === false)) {
trace._input.zmin = trace.zmin =
contours.start - contours.size/2;
contours.start - contours.size / 2;
trace._input.zmax = trace.zmax =
trace.zmin + pathinfo.length * contours.size;
}

heatmapPlot(gd, plotinfo, [cd]);
}
// in case this used to be a heatmap (or have heatmap fill)
else fullLayout._paper.selectAll('.hm'+uid).remove();
else fullLayout._paper.selectAll('.hm' + uid).remove();

makeCrossings(pathinfo);
findAllPaths(pathinfo);
Expand Down Expand Up @@ -471,12 +472,15 @@ function getInterpPx(pi, loc, step) {

function makeContourGroup(plotinfo, cd, id) {
var plotgroup = plotinfo.plot.select('.maplayer')
.selectAll('g.contour.'+id)
.selectAll('g.contour.' + id)
.data(cd);

plotgroup.enter().append('g')
.classed('contour',true)
.classed(id,true);
.classed('contour', true)
.classed(id, true);

plotgroup.exit().remove();

return plotgroup;
}

Expand Down
50 changes: 30 additions & 20 deletions src/traces/heatmap/plot.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,13 @@ var xmlnsNamespaces = require('../../constants/xmlns_namespaces');
var maxRowLength = require('./max_row_length');


// From http://www.xarg.org/2010/03/generate-client-side-png-files-using-javascript/
module.exports = function(gd, plotinfo, cdheatmaps) {
cdheatmaps.forEach(function(cd) { plotOne(gd, plotinfo, cd); });
for(var i = 0; i < cdheatmaps.length; i++) {
plotOne(gd, plotinfo, cdheatmaps[i]);
}
};

// From http://www.xarg.org/2010/03/generate-client-side-png-files-using-javascript/
function plotOne(gd, plotinfo, cd) {
Lib.markTime('in Heatmap.plot');

Expand All @@ -33,14 +35,14 @@ function plotOne(gd, plotinfo, cd) {
xa = plotinfo.x(),
ya = plotinfo.y(),
fullLayout = gd._fullLayout,
id = 'hm' + uid,
cbId = 'cb' + uid;
id = 'hm' + uid;

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

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

Expand Down Expand Up @@ -367,20 +369,28 @@ function plotOne(gd, plotinfo, cd) {
gd._hmpixcount = (gd._hmpixcount||0) + pixcount;
gd._hmlumcount = (gd._hmlumcount||0) + pixcount * avgColor.getLuminance();

// put this right before making the new image, to minimize flicker
fullLayout._paper.selectAll('.'+id).remove();
plotinfo.plot.select('.maplayer').append('svg:image')
.classed(id, true)
.datum(cd[0])
.attr({
xmlns: xmlnsNamespaces.svg,
'xlink:href': canvas.toDataURL('image/png'),
height: imageHeight,
width: imageWidth,
x: left,
y: top,
preserveAspectRatio: 'none'
});
var plotgroup = plotinfo.plot.select('.imagelayer')
.selectAll('g.hm.' + id)
.data([0]);
plotgroup.enter().append('g')
.classed('hm', true)
.classed(id, true);
plotgroup.exit().remove();

var image3 = plotgroup.selectAll('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.

Why image3?

Copy link
Contributor

Choose a reason for hiding this comment

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

3 as in a d3 selection.

We used that convention elsewhere in the code (e.g. here), but haven't enforced it.

I personally like a lot, it makes clear what variables are linked to selection vs straight dom nodes.

@mdtusz your thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As long as it's consistent, I'm fine with that.

.data(cd);
image3.enter().append('svg:image');
image3.exit().remove();

image3.attr({
xmlns: xmlnsNamespaces.svg,
'xlink:href': canvas.toDataURL('image/png'),
height: imageHeight,
width: imageWidth,
x: left,
y: top,
preserveAspectRatio: 'none'
});

Lib.markTime('done showing png');
}
96 changes: 96 additions & 0 deletions test/jasmine/tests/plot_interact_test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
var d3 = require('d3');

var Plotly = require('@lib/index');
var Lib = require('@src/lib');

var createGraphDiv = require('../assets/create_graph_div');
var destroyGraphDiv = require('../assets/destroy_graph_div');
Expand Down Expand Up @@ -62,6 +63,101 @@ describe('Test plot structure', function() {
});
});

describe('contour/heatmap traces', function() {
var mock = require('@mocks/connectgaps_2d.json');

function extendMock() {
var mockCopy = Lib.extendDeep(mock);

// add a colorbar for testing
mockCopy.data[0].showscale = true;

return mockCopy;
}

describe('initial structure', function() {
beforeEach(function(done) {
var mockCopy = extendMock();

Plotly.plot(createGraphDiv(), mockCopy.data, mockCopy.layout)
.then(done);
});

it('has four *subplot* nodes', function() {
var nodes = d3.selectAll('g.subplot');
expect(nodes.size()).toEqual(4);
});

// N.B. the contour traces both have a heatmap fill
it('has four heatmap image nodes', function() {
var hmNodes = d3.selectAll('g.hm');
expect(hmNodes.size()).toEqual(4);

var imageNodes = d3.selectAll('image');
expect(imageNodes.size()).toEqual(4);
});

it('has two contour nodes', function() {
var nodes = d3.selectAll('g.contour');
expect(nodes.size()).toEqual(2);
});

it('has one colorbar nodes', function() {
var nodes = d3.selectAll('rect.cbbg');
expect(nodes.size()).toEqual(1);
});
});

describe('structure after restyle', function() {
beforeEach(function(done) {
var mockCopy = extendMock();
var gd = createGraphDiv();

Plotly.plot(gd, mockCopy.data, mockCopy.layout);

Plotly.restyle(gd, {
type: 'scatter',
x: [[1, 2, 3]],
y: [[2, 1, 2]],
z: null
}, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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


Plotly.restyle(gd, 'type', 'contour', 1);

Plotly.restyle(gd, 'type', 'heatmap', 2)
.then(done);
});

it('has four *subplot* nodes', function() {
var nodes = d3.selectAll('g.subplot');
expect(nodes.size()).toEqual(4);
});

it('has two heatmap image nodes', function() {
var hmNodes = d3.selectAll('g.hm');
expect(hmNodes.size()).toEqual(2);

var imageNodes = d3.selectAll('image');
expect(imageNodes.size()).toEqual(2);
});

it('has two contour nodes', function() {
var nodes = d3.selectAll('g.contour');
expect(nodes.size()).toEqual(2);
});

it('has one scatter node', function() {
var nodes = d3.selectAll('g.trace.scatter');
expect(nodes.size()).toEqual(1);
});

it('has no colorbar node', function() {
var nodes = d3.selectAll('rect.cbbg');
expect(nodes.size()).toEqual(0);
});
});
});

describe('pie traces', function() {
var mock = require('@mocks/pie_simple.json');

Expand Down
2 changes: 1 addition & 1 deletion test/jasmine/tests/plot_promise_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ describe('Plotly.___ methods', function() {

Plotly.plot(initialDiv, data, {});

promise = Plotly.restyle(initialDiv, 'title', 'Promise test!');
promise = Plotly.relayout(initialDiv, 'title', 'Promise test!');
Copy link
Contributor

Choose a reason for hiding this comment

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

@mdtusz restyle was getting tested twice.


promise.then(function(gd){
promiseGd = gd;
Expand Down