8000 Merge pull request #4102 from plotly/constraint-contours-rounded-off-… · plotly/plotly.js@060369a · GitHub
[go: up one dir, main page]
Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 060369a

Browse files
etpinardetpinard
authored
Merge pull request #4102 from plotly/constraint-contours-rounded-off-edgepath-bug
Constraint contours with rounded-off edgepath bug fix
2 parents 3561cab + 6f9a59d commit 060369a

21 files changed

+1257
-269
lines changed

‎src/traces/carpet/attributes.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ module.exports = {
2828
editType: 'calc',
2929
description: [
3030
'An identifier for this carpet, so that `scattercarpet` and',
31-
'`scattercontour` traces can specify a carpet plot on which',
31+
'`contourcarpet` traces can specify a carpet plot on which',
3232
'they lie'
3333
].join(' ')
3434
},

‎src/traces/carpet/index.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ module.exports = {
1919
moduleType: 'trace',
2020
name: 'carpet',
2121
basePlotModule: require('../../plots/cartesian'),
22-
categories: ['cartesian', 'svg', 'carpet', 'carpetAxis', 'notLegendIsolatable', 'noMultiCategory'],
22+
categories: ['cartesian', 'svg', 'carpet', 'carpetAxis', 'notLegendIsolatable', 'noMultiCategory', 'noHover'],
2323
meta: {
2424
description: [
2525
'The data describing carpet axis layout is set in `y` and (optionally)',

‎src/traces/contour/close_boundaries.js

Lines changed: 65 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -8,57 +8,80 @@
88

99
'use strict';
1010

11-
module.exports = function(pathinfo, operation, perimeter, trace) {
12-
// Abandon all hope, ye who enter here.
13-
var i, v1, v2;
11+
module.exports = function(pathinfo, contours) {
1412
var pi0 = pathinfo[0];
15-
var na = pi0.x.length;
16-
var nb = pi0.y.length;
1713
var z = pi0.z;
18-
var contours = trace.contours;
14+
var i;
1915

20-
var boundaryMax = -Infinity;
21-
var boundaryMin = Infinity;
16+
switch(contours.type) {
17+
case 'levels':
18+
// Why (just) use z[0][0] and z[0][1]?
19+
//
20+
// N.B. using boundaryMin instead of edgeVal2 here makes the
21+
// `contour_scatter` mock fail
22+
var edgeVal2 = Math.min(z[0][0], z[0][1]);
2223

23-
for(i = 0; i < nb; i++) {
24-
boundaryMin = Math.min(boundaryMin, z[i][0]);
25-
boundaryMin = Math.min(boundaryMin, z[i][na - 1]);
26-
boundaryMax = Math.max(boundaryMax, z[i][0]);
27-
boundaryMax = Math.max(boundaryMax, z[i][na - 1]);
28-
}
24+
for(i = 0; i < pathinfo.length; i++) {
25+
var pi = pathinfo[i];
26+
pi.prefixBoundary = !pi.edgepaths.length &&
27+
(edgeVal2 > pi.level || pi.starts.length && edgeVal2 === pi.level);
28+
}
29+
break;
30+
case 'constraint':
31+
// after convertToConstraints, pathinfo has length=0
32+
pi0.prefixBoundary = false;
2933

30-
for(i = 1; i < na - 1; i++) {
31-
boundaryMin = Math.min(boundaryMin, z[0][i]);
32-
boundaryMin = Math.min(boundaryMin, z[nb - 1][i]);
33-
boundaryMax = Math.max(boundaryMax, z[0][i]);
34-
boundaryMax = Math.max(boundaryMax, z[nb - 1][i]);
35-
}
34+
// joinAllPaths does enough already when edgepaths are present
35+
if(pi0.edgepaths.length) return;
3636

37-
pi0.prefixBoundary = false;
37+
var na = pi0.x.length;
38+
var nb = pi0.y.length;
39+
var boundaryMax = -Infinity;
40+
var boundaryMin = Infinity;
3841

39-
switch(operation) {
40-
case '>':
41-
if(contours.value > boundaryMax) {
42-
pi0.prefixBoundary = true;
43-
}
44-
break;
45-
case '<':
46-
if(contours.value < boundaryMin) {
47-
pi0.prefixBoundary = true;
42+
for(i = 0; i < nb; i++) {
43+
boundaryMin = Math.min(boundaryMin, z[i][0]);
44+
boundaryMin = Math.min(boundaryMin, z[i][na - 1]);
45+
boundaryMax = Math.max(boundaryMax, z[i][0]);
46+
boundaryMax = Math.max(boundaryMax, z[i][na - 1]);
4847
}
49-
break;
50-
case '[]':
51-
v1 = Math.min.apply(null, contours.value);
52-
v2 = Math.max.apply(null, contours.value);
53-
if(v2 < boundaryMin || v1 > boundaryMax) {
54-
pi0.prefixBoundary = true;
48+
for(i = 1; i < na - 1; i++) {
49+
boundaryMin = Math.min(boundaryMin, z[0][i]);
50+
boundaryMin = Math.min(boundaryMin, z[nb - 1][i]);
51+
boundaryMax = Math.max(boundaryMax, z[0][i]);
52+
boundaryMax = Math.max(boundaryMax, z[nb - 1][i]);
5553
}
56-
break;
57-
case '][':
58-
v1 = Math.min.apply(null, contours.value);
59-
v2 = Math.max.apply(null, contours.value);
60-
if(v1 < boundaryMin && v2 > boundaryMax) {
61-
pi0.prefixBoundary = true;
54+
55+
var contoursValue = contours.value;
56+
var v1, v2;
57+
58+
switch(contours._operation) {
59+
case '>':
60+
if(contoursValue > boundaryMax) {
61+
pi0.prefixBoundary = true;
62+
}
63+
break;
64+
case '<':
65+
if(contoursValue < boundaryMin ||
66+
(pi0.starts.length && contoursValue === boundaryMin)) {
67+
pi0.prefixBoundary = true;
68+
}
69+
break;
70+
case '[]':
71+
v1 = Math.min(contoursValue[0], contoursValue[1]);
72+
v2 = Math.max(contoursValue[0], contoursValue[1]);
73+
if(v2 < boundaryMin || v1 > boundaryMax ||
74+
(pi0.starts.length && v2 === boundaryMin)) {
75+
pi0.prefixBoundary = true;
76+
}
77+
break;
78+
case '][':
79+
v1 = Math.min(contoursValue[0], contoursValue[1]);
80+
v2 = Math.max(contoursValue[0], contoursValue[1]);
81+
if(v1 < boundaryMin && v2 > boundaryMax) {
82+
pi0.prefixBoundary = true;
83+
}
84+
break;
6285
}
6386
break;
6487
}

‎src/traces/contour/convert_to_constraints.js

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ var Lib = require('../../lib');
1414
// need weird range loops and flipped contours instead of the usual format. This function
1515
// does some weird manipulation of the extracted pathinfo data such that it magically
1616
// draws contours correctly *as* constraints.
17+
//
18+
// ** I do not know which "weird range loops" the comment above is referring to.
1719
module.exports = function(pathinfo, operation) {
1820
var i, pi0, pi1;
1921

@@ -29,18 +31,20 @@ module.exports = function(pathinfo, operation) {
2931
Lib.warn('Contour data invalid for the specified inequality operation.');
3032
}
3133

32-
// In this case there should be exactly two contour levels in pathinfo. We
33-
// simply concatenate the info into one pathinfo and flip all of the data
34-
// in one. This will draw the contour as closed.
34+
// In this case there should be exactly one contour levels in pathinfo.
35+
// We flip all of the data. This will draw the contour as closed.
3536
pi0 = pathinfo[0];
3637

3738
for(i = 0; i < pi0.edgepaths.length; i++) {
3839
pi0.edgepaths[i] = op0(pi0.edgepaths[i]);
3940
}
40-
4141
for(i = 0; i < pi0.paths.length; i++) {
4242
pi0.paths[i] = op0(pi0.paths[i]);
4343
}
44+
for(i = 0; i < pi0.starts.length; i++) {
45+
pi0.starts[i] = op0(pi0.starts[i]);
46+
}
47+
4448
return pathinfo;
4549
case '][':
4650
var tmp = op0;
@@ -54,33 +58,41 @@ module.exports = function(pathinfo, operation) {
5458
Lib.warn('Contour data invalid for the specified inequality range operation.');
5559
}
5660

57-
// In this case there should be exactly two contour levels in pathinfo. We
58-
// simply concatenate the info into one pathinfo and flip all of the data
59-
// in one. This will draw the contour as closed.
61+
// In this case there should be exactly two contour levels in pathinfo.
62+
// - We concatenate the info into one pathinfo.
63+
// - We must also flip all of the data in the `[]` case.
64+
// This will draw the contours as closed.
6065
pi0 = copyPathinfo(pathinfo[0]);
6166
pi1 = copyPathinfo(pathinfo[1]);
6267

6368
for(i = 0; i < pi0.edgepaths.length; i++) {
6469
pi0.edgepaths[i] = op0(pi0.edgepaths[i]);
6570
}
66-
6771
for(i = 0; i < pi0.paths.length; i++) {
6872
pi0.paths[i] = op0(pi0.paths[i]);
6973
}
74+
for(i = 0; i < pi0.starts.length; i++) {
75+
pi0.starts[i] = op0(pi0.starts[i]);
76+
}
7077

7178
while(pi1.edgepaths.length) {
7279
pi0.edgepaths.push(op1(pi1.edgepaths.shift()));
7380
}
7481
while(pi1.paths.length) {
7582
pi0.paths.push(op1(pi1.paths.shift()));
7683
}
84+
while(pi1.starts.length) {
85+
pi0.starts.push(op1(pi1.starts.shift()));
86+
}
87+
7788
return [pi0];
7889
}
7990
};
8091

8192
function copyPathinfo(pi) {
8293
return Lib.extendFlat({}, pi, {
8394
edgepaths: Lib.extendDeep([], pi.edgepaths),
84-
paths: Lib.extendDeep([], pi.paths)
95+
paths: Lib.extendDeep([], pi.paths),
96+
starts: Lib.extendDeep([], pi.starts)
8597
});
8698
}

‎src/traces/contour/find_all_paths.js

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -53,15 +53,15 @@ function ptDist(pt1, pt2) {
5353
}
5454

5555
function makePath(pi, loc, edgeflag, xtol, ytol) {
56-
var startLocStr = loc.join(',');
57-
var locStr = startLocStr;
56+
var locStr = loc.join(',');
5857
var mi = pi.crossings[locStr];
59-
var marchStep = startStep(mi, edgeflag, loc);
58+
var marchStep = getStartStep(mi, edgeflag, loc);
6059
// start by going backward a half step and finding the crossing point
6160
var pts = [getInterpPx(pi, loc, [-marchStep[0], -marchStep[1]])];
62-
var startStepStr = marchStep.join(',');
6361
var m = pi.z.length;
6462
var n = pi.z[0].length;
63+
var startLoc = loc.slice();
64+
var startStep = marchStep.slice();
6565
var cnt;
6666

6767
// now follow the path
@@ -83,14 +83,16 @@ function makePath(pi, loc, edgeflag, xtol, ytol) {
8383
pts.push(getInterpPx(pi, loc, marchStep));
8484
loc[0] += marchStep[0];
8585
loc[1] += marchStep[1];
86+
locStr = loc.join(',');
8687

8788
// don't include the same point multiple times
8889
if(equalPts(pts[pts.length - 1], pts[pts.length - 2], xtol, ytol)) pts.pop();
89-
locStr = loc.join(',');
9090

9191
var atEdge = (marchStep[0] && (loc[0] < 0 || loc[0] > n - 2)) ||
9292
(marchStep[1] && (loc[1] < 0 || loc[1] > m - 2));
93-
var closedLoop = (locStr === startLocStr) && (marchStep.join(',') === startStepStr);
93+
94+
var closedLoop = loc[0] === startLoc[0] && loc[1] === startLoc[1] &&
95+
marchStep[0] === startStep[0] && marchStep[1] === startStep[1];
9496

9597
// have we completed a loop, or reached an edge?
9698
if((closedLoop) || (edgeflag && atEdge)) break;
@@ -184,7 +186,7 @@ function makePath(pi, loc, edgeflag, xtol, ytol) {
184186
} else {
185187
if(!edgeflag) {
186188
Lib.log('Unclosed interior contour?',
187-
pi.level, startLocStr, pts.join('L'));
189+
pi.level, startLoc.join(','), pts.join('L'));
188190
}
189191

190192
// edge path - does it start where an existing edge path ends, or vice versa?
@@ -234,7 +236,7 @@ function makePath(pi, loc, edgeflag, xtol, ytol) {
234236

235237
// special function to get the marching step of the
236238
// first point in the path (leading to loc)
237-
function startStep(mi, edgeflag, loc) {
239+
function getStartStep(mi, edgeflag, loc) {
238240
var dx = 0;
239241
var dy = 0;
240242
if(mi > 20 && edgeflag) {

‎src/traces/contour/plot.js

Lines changed: 29 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -63,8 +63,8 @@ exports.plot = function plot(gd, plotinfo, cdcontours, contourLayer) {
6363

6464
var fillPathinfo = pathinfo;
6565
if(contours.type === 'constraint') {
66+
// N.B. this also mutates pathinfo
6667
fillPathinfo = convertToConstraints(pathinfo, contours._operation);
67-
closeBoundaries(fillPathinfo, contours._operation, perimeter, trace);
6868
}
6969

7070
// draw everything
@@ -88,10 +88,17 @@ function makeBackground(plotgroup, perimeter, contours) {
8888
}
8989

9090
function makeFills(plotgroup, pathinfo, perimeter, contours) {
91+
var hasFills = contours.coloring === 'fill' || (contours.type === 'constraint' && contours._operation !== '=');
92+
var boundaryPath = 'M' + perimeter.join('L') + 'Z';
93+
94+
// fills prefixBoundary in pathinfo items
95+
if(hasFills) {
96+
closeBoundaries(pathinfo, contours);
97+
}
98+
9199
var fillgroup = Lib.ensureSingle(plotgroup, 'g', 'contourfill');
92100

93-
var fillitems = fillgroup.selectAll('path')
94-
.data(contours.coloring === 'fill' || (contours.type === 'constraint' && contours._operation !== '=') ? pathinfo : []);
101+
var fillitems = fillgroup.selectAll('path').data(hasFills ? pathinfo : []);
95102
fillitems.enter().append('path');
96103
fillitems.exit().remove();
97104
fillitems.each(function(pi) {
@@ -100,30 +107,21 @@ function makeFills(plotgroup, pathinfo, perimeter, contours) {
100107
// if the whole perimeter is above this level, start with a path
101108
// enclosing the whole thing. With all that, the parity should mean
102109
// that we always fill everything above the contour, nothing below
103-
var fullpath = joinAllPaths(pi, perimeter);
110+
var fullpath = (pi.prefixBoundary ? boundaryPath : '') +
111+
joinAllPaths(pi, perimeter);
104112

105-
if(!fullpath) d3.select(this).remove();
106-
else d3.select(this).attr('d', fullpath).style('stroke', 'none');
113+
if(!fullpath) {
114+
d3.select(this).remove();
115+
} else {
116+
d3.select(this)
117+
.attr('d', fullpath)
118+
.style('stroke', 'none');
119+
}
107120
});
108121
}
109122

110-
function initFullPath(pi, perimeter) {
111-
var prefixBoundary = pi.prefixBoundary;
112-
if(prefixBoundary === undefined) {
113-
var edgeVal2 = Math.min(pi.z[0][0], pi.z[0][1]);
114-
prefixBoundary = (!pi.edgepaths.length && edgeVal2 > pi.level);
115-
}
116-
117-
if(prefixBoundary) {
118-
// TODO: why does ^^ not work for constraints?
119-
// pi.prefixBoundary gets set by closeBoundaries
120-
return 'M' + perimeter.join('L') + 'Z';
121-
}
122-
return '';
123-
}
124-
125123
function joinAllPaths(pi, perimeter) {
126-
var fullpath = initFullPath(pi, perimeter);
124+
var fullpath = '';
127125
var i = 0;
128126
var startsleft = pi.edgepaths.map(function(v, i) { return i; });
129127
var newloop = true;
@@ -612,17 +610,18 @@ exports.drawLabels = function(labelGroup, labelData, gd, lineClip, labelClipPath
612610
};
613611

614612
function clipGaps(plotGroup, plotinfo, gd, cd0, perimeter) {
613+
var trace = cd0.trace;
615614
var clips = gd._fullLayout._clips;
616-
var clipId = 'clip' + cd0.trace.uid;
615+
var clipId = 'clip' + trace.uid;
617616

618617
var clipPath = clips.selectAll('#' + clipId)
619-
.data(cd0.trace.connectgaps ? [] : [0]);
618+
.data(trace.connectgaps ? [] : [0]);
620619
clipPath.enter().append('clipPath')
621620
.classed('contourclip', true)
622621
.attr('id', clipId);
623622
clipPath.exit().remove();
624623

625-
if(cd0.trace.connectgaps === false) {
624+
if(trace.connectgaps === false) {
626625
var clipPathInfo = {
627626
// fraction of the way from missing to present point
628627
// to draw the boundary.
@@ -644,10 +643,13 @@ function clipGaps(plotGroup, plotinfo, gd, cd0, perimeter) {
644643

645644
makeCrossings([clipPathInfo]);
646645
findAllPaths([clipPathInfo]);
647-
var fullpath = joinAllPaths(clipPathInfo, perimeter);
646+
closeBoundaries([clipPathInfo], {type: 'levels'});
648647

649648
var path = Lib.ensureSingle(clipPath, 'path', '');
650-
path.attr('d', fullpath);
649+
path.attr('d',
650+
(clipPathInfo.prefixBoundary ? 'M' + perimeter.join('L') + 'Z' : '') +
651+
joinAllPaths(clipPathInfo, perimeter)
652+
);
651653
} else clipId = null;
652654

653655
Drawing.setClipUrl(plotGroup, clipId, gd);

‎src/traces/contourcarpet/index.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ module.exports = {
1919
moduleType: 'trace',
2020
name: 'contourcarpet',
2121
basePlotModule: require('../../plots/cartesian'),
22-
categories: ['cartesian', 'svg', 'carpet', 'contour', 'symbols', 'showLegend', 'hasLines', 'carpetDependent'],
22+
categories: ['cartesian', 'svg', 'carpet', 'contour', 'symbols', 'showLegend', 'hasLines', 'carpetDependent', 'noHover'],
2323
meta: {
2424
hrName: 'contour_carpet',
2525
description: [

0 commit comments

Comments
 (0)
0