Merge pull request #4102 from plotly/constraint-contours-rounded-off-edgepath-bug

Constraint contours with rounded-off edgepath bug fix

Author: etpinard

Diff for: src/traces/carpet/attributes.js

@@ -28,7 +28,7 @@ module.exports = {
         editType: 'calc',
         description: [
             'An identifier for this carpet, so that `scattercarpet` and',
-            '`scattercontour` traces can specify a carpet plot on which',
+            '`contourcarpet` traces can specify a carpet plot on which',
             'they lie'
         ].join(' ')
     },

Diff for: src/traces/carpet/index.js

@@ -19,7 +19,7 @@ module.exports = {
     moduleType: 'trace',
     name: 'carpet',
     basePlotModule: require('../../plots/cartesian'),
-    categories: ['cartesian', 'svg', 'carpet', 'carpetAxis', 'notLegendIsolatable', 'noMultiCategory'],
+    categories: ['cartesian', 'svg', 'carpet', 'carpetAxis', 'notLegendIsolatable', 'noMultiCategory', 'noHover'],
     meta: {
         description: [
             'The data describing carpet axis layout is set in `y` and (optionally)',

Diff for: src/traces/contour/close_boundaries.js

@@ -8,57 +8,80 @@
 
 'use strict';
 
-module.exports = function(pathinfo, operation, perimeter, trace) {
-    // Abandon all hope, ye who enter here.
-    var i, v1, v2;
+module.exports = function(pathinfo, contours) {
     var pi0 = pathinfo[0];
-    var na = pi0.x.length;
-    var nb = pi0.y.length;
     var z = pi0.z;
-    var contours = trace.contours;
+    var i;
 
-    var boundaryMax = -Infinity;
-    var boundaryMin = Infinity;
+    switch(contours.type) {
+        case 'levels':
+            // Why (just) use z[0][0] and z[0][1]?
+            //
+            // N.B. using boundaryMin instead of edgeVal2 here makes the
+            //      `contour_scatter` mock fail
+            var edgeVal2 = Math.min(z[0][0], z[0][1]);
 
-    for(i = 0; i < nb; i++) {
-        boundaryMin = Math.min(boundaryMin, z[i][0]);
-        boundaryMin = Math.min(boundaryMin, z[i][na - 1]);
-        boundaryMax = Math.max(boundaryMax, z[i][0]);
-        boundaryMax = Math.max(boundaryMax, z[i][na - 1]);
-    }
+            for(i = 0; i < pathinfo.length; i++) {
+                var pi = pathinfo[i];
+                pi.prefixBoundary = !pi.edgepaths.length &&
+                    (edgeVal2 > pi.level || pi.starts.length && edgeVal2 === pi.level);
+            }
+            break;
+        case 'constraint':
+            // after convertToConstraints, pathinfo has length=0
+            pi0.prefixBoundary = false;
 
-    for(i = 1; i < na - 1; i++) {
-        boundaryMin = Math.min(boundaryMin, z[0][i]);
-        boundaryMin = Math.min(boundaryMin, z[nb - 1][i]);
-        boundaryMax = Math.max(boundaryMax, z[0][i]);
-        boundaryMax = Math.max(boundaryMax, z[nb - 1][i]);
-    }
+            // joinAllPaths does enough already when edgepaths are present
+            if(pi0.edgepaths.length) return;
 
-    pi0.prefixBoundary = false;
+            var na = pi0.x.length;
+            var nb = pi0.y.length;
+            var boundaryMax = -Infinity;
+            var boundaryMin = Infinity;
 
-    switch(operation) {
-        case '>':
-            if(contours.value > boundaryMax) {
-                pi0.prefixBoundary = true;
-            }
-            break;
-        case '<':
-            if(contours.value < boundaryMin) {
-                pi0.prefixBoundary = true;
+            for(i = 0; i < nb; i++) {
+                boundaryMin = Math.min(boundaryMin, z[i][0]);
+                boundaryMin = Math.min(boundaryMin, z[i][na - 1]);
+                boundaryMax = Math.max(boundaryMax, z[i][0]);
+                boundaryMax = Math.max(boundaryMax, z[i][na - 1]);
             }
-            break;
-        case '[]':
-            v1 = Math.min.apply(null, contours.value);
-            v2 = Math.max.apply(null, contours.value);
-            if(v2 < boundaryMin || v1 > boundaryMax) {
-                pi0.prefixBoundary = true;
+            for(i = 1; i < na - 1; i++) {
+                boundaryMin = Math.min(boundaryMin, z[0][i]);
+                boundaryMin = Math.min(boundaryMin, z[nb - 1][i]);
+                boundaryMax = Math.max(boundaryMax, z[0][i]);
+                boundaryMax = Math.max(boundaryMax, z[nb - 1][i]);
             }
-            break;
-        case '][':
-            v1 = Math.min.apply(null, contours.value);
-            v2 = Math.max.apply(null, contours.value);
-            if(v1 < boundaryMin && v2 > boundaryMax) {
-                pi0.prefixBoundary = true;
+
+            var contoursValue = contours.value;
+            var v1, v2;
+
+            switch(contours._operation) {
+                case '>':
+                    if(contoursValue > boundaryMax) {
+                        pi0.prefixBoundary = true;
+                    }
+                    break;
+                case '<':
+                    if(contoursValue < boundaryMin ||
+                        (pi0.starts.length && contoursValue === boundaryMin)) {
+                        pi0.prefixBoundary = true;
+                    }
+                    break;
+                case '[]':
+                    v1 = Math.min(contoursValue[0], contoursValue[1]);
+                    v2 = Math.max(contoursValue[0], contoursValue[1]);
+                    if(v2 < boundaryMin || v1 > boundaryMax ||
+                        (pi0.starts.length && v2 === boundaryMin)) {
+                        pi0.prefixBoundary = true;
+                    }
+                    break;
+                case '][':
+                    v1 = Math.min(contoursValue[0], contoursValue[1]);
+                    v2 = Math.max(contoursValue[0], contoursValue[1]);
+                    if(v1 < boundaryMin && v2 > boundaryMax) {
+                        pi0.prefixBoundary = true;
+                    }
+                    break;
             }
             break;
     }

Diff for: src/traces/contour/convert_to_constraints.js

@@ -14,6 +14,8 @@ var Lib = require('../../lib');
 // need weird range loops and flipped contours instead of the usual format. This function
 // does some weird manipulation of the extracted pathinfo data such that it magically
 // draws contours correctly *as* constraints.
+//
+// ** I do not know which "weird range loops" the comment above is referring to.
 module.exports = function(pathinfo, operation) {
     var i, pi0, pi1;
 
@@ -29,18 +31,20 @@ module.exports = function(pathinfo, operation) {
                 Lib.warn('Contour data invalid for the specified inequality operation.');
             }
 
-            // In this case there should be exactly two contour levels in pathinfo. We
-            // simply concatenate the info into one pathinfo and flip all of the data
-            // in one. This will draw the contour as closed.
+            // In this case there should be exactly one contour levels in pathinfo.
+            // We flip all of the data. This will draw the contour as closed.
             pi0 = pathinfo[0];
 
             for(i = 0; i < pi0.edgepaths.length; i++) {
                 pi0.edgepaths[i] = op0(pi0.edgepaths[i]);
             }
-
             for(i = 0; i < pi0.paths.length; i++) {
                 pi0.paths[i] = op0(pi0.paths[i]);
             }
+            for(i = 0; i < pi0.starts.length; i++) {
+                pi0.starts[i] = op0(pi0.starts[i]);
+            }
+
             return pathinfo;
         case '][':
             var tmp = op0;
@@ -54,33 +58,41 @@ module.exports = function(pathinfo, operation) {
                 Lib.warn('Contour data invalid for the specified inequality range operation.');
             }
 
-            // In this case there should be exactly two contour levels in pathinfo. We
-            // simply concatenate the info into one pathinfo and flip all of the data
-            // in one. This will draw the contour as closed.
+            // In this case there should be exactly two contour levels in pathinfo.
+            // - We concatenate the info into one pathinfo.
+            // - We must also flip all of the data in the `[]` case.
+            // This will draw the contours as closed.
             pi0 = copyPathinfo(pathinfo[0]);
             pi1 = copyPathinfo(pathinfo[1]);
 
             for(i = 0; i < pi0.edgepaths.length; i++) {
                 pi0.edgepaths[i] = op0(pi0.edgepaths[i]);
             }
-
             for(i = 0; i < pi0.paths.length; i++) {
                 pi0.paths[i] = op0(pi0.paths[i]);
             }
+            for(i = 0; i < pi0.starts.length; i++) {
+                pi0.starts[i] = op0(pi0.starts[i]);
+            }
 
             while(pi1.edgepaths.length) {
                 pi0.edgepaths.push(op1(pi1.edgepaths.shift()));
             }
             while(pi1.paths.length) {
                 pi0.paths.push(op1(pi1.paths.shift()));
             }
+            while(pi1.starts.length) {
+                pi0.starts.push(op1(pi1.starts.shift()));
+            }
+
             return [pi0];
     }
 };
 
 function copyPathinfo(pi) {
     return Lib.extendFlat({}, pi, {
         edgepaths: Lib.extendDeep([], pi.edgepaths),
-        paths: Lib.extendDeep([], pi.paths)
+        paths: Lib.extendDeep([], pi.paths),
+        starts: Lib.extendDeep([], pi.starts)
     });
 }

Diff for: src/traces/contour/find_all_paths.js

@@ -53,15 +53,15 @@ function ptDist(pt1, pt2) {
 }
 
 function makePath(pi, loc, edgeflag, xtol, ytol) {
-    var startLocStr = loc.join(',');
-    var locStr = startLocStr;
+    var locStr = loc.join(',');
     var mi = pi.crossings[locStr];
-    var marchStep = startStep(mi, edgeflag, loc);
+    var marchStep = getStartStep(mi, edgeflag, loc);
     // start by going backward a half step and finding the crossing point
     var pts = [getInterpPx(pi, loc, [-marchStep[0], -marchStep[1]])];
-    var startStepStr = marchStep.join(',');
     var m = pi.z.length;
     var n = pi.z[0].length;
+    var startLoc = loc.slice();
+    var startStep = marchStep.slice();
     var cnt;
 
     // now follow the path
@@ -83,14 +83,16 @@ function makePath(pi, loc, edgeflag, xtol, ytol) {
         pts.push(getInterpPx(pi, loc, marchStep));
         loc[0] += marchStep[0];
         loc[1] += marchStep[1];
+        locStr = loc.join(',');
 
         // don't include the same point multiple times
         if(equalPts(pts[pts.length - 1], pts[pts.length - 2], xtol, ytol)) pts.pop();
-        locStr = loc.join(',');
 
         var atEdge = (marchStep[0] && (loc[0] < 0 || loc[0] > n - 2)) ||
                 (marchStep[1] && (loc[1] < 0 || loc[1] > m - 2));
-        var closedLoop = (locStr === startLocStr) && (marchStep.join(',') === startStepStr);
+
+        var closedLoop = loc[0] === startLoc[0] && loc[1] === startLoc[1] &&
+                marchStep[0] === startStep[0] && marchStep[1] === startStep[1];
 
         // have we completed a loop, or reached an edge?
         if((closedLoop) || (edgeflag && atEdge)) break;
@@ -184,7 +186,7 @@ function makePath(pi, loc, edgeflag, xtol, ytol) {
     } else {
         if(!edgeflag) {
             Lib.log('Unclosed interior contour?',
-                pi.level, startLocStr, pts.join('L'));
+                pi.level, startLoc.join(','), pts.join('L'));
         }
 
         // 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) {
 
 // special function to get the marching step of the
 // first point in the path (leading to loc)
-function startStep(mi, edgeflag, loc) {
+function getStartStep(mi, edgeflag, loc) {
     var dx = 0;
     var dy = 0;
     if(mi > 20 && edgeflag) {

Diff for: src/traces/contour/plot.js

@@ -63,8 +63,8 @@ exports.plot = function plot(gd, plotinfo, cdcontours, contourLayer) {
 
         var fillPathinfo = pathinfo;
         if(contours.type === 'constraint') {
+            // N.B. this also mutates pathinfo
             fillPathinfo = convertToConstraints(pathinfo, contours._operation);
-            closeBoundaries(fillPathinfo, contours._operation, perimeter, trace);
         }
 
         // draw everything
@@ -88,10 +88,17 @@ function makeBackground(plotgroup, perimeter, contours) {
 }
 
 function makeFills(plotgroup, pathinfo, perimeter, contours) {
+    var hasFills = contours.coloring === 'fill' || (contours.type === 'constraint' && contours._operation !== '=');
+    var boundaryPath = 'M' + perimeter.join('L') + 'Z';
+
+    // fills prefixBoundary in pathinfo items
+    if(hasFills) {
+        closeBoundaries(pathinfo, contours);
+    }
+
     var fillgroup = Lib.ensureSingle(plotgroup, 'g', 'contourfill');
 
-    var fillitems = fillgroup.selectAll('path')
-        .data(contours.coloring === 'fill' || (contours.type === 'constraint' && contours._operation !== '=') ? pathinfo : []);
+    var fillitems = fillgroup.selectAll('path').data(hasFills ? pathinfo : []);
     fillitems.enter().append('path');
     fillitems.exit().remove();
     fillitems.each(function(pi) {
@@ -100,30 +107,21 @@ function makeFills(plotgroup, pathinfo, perimeter, contours) {
         // if the whole perimeter is above this level, start with a path
         // enclosing the whole thing. With all that, the parity should mean
         // that we always fill everything above the contour, nothing below
-        var fullpath = joinAllPaths(pi, perimeter);
+        var fullpath = (pi.prefixBoundary ? boundaryPath : '') +
+            joinAllPaths(pi, perimeter);
 
-        if(!fullpath) d3.select(this).remove();
-        else d3.select(this).attr('d', fullpath).style('stroke', 'none');
+        if(!fullpath) {
+            d3.select(this).remove();
+        } else {
+            d3.select(this)
+                .attr('d', fullpath)
+                .style('stroke', 'none');
+        }
     });
 }
 
-function initFullPath(pi, perimeter) {
-    var prefixBoundary = pi.prefixBoundary;
-    if(prefixBoundary === undefined) {
-        var edgeVal2 = Math.min(pi.z[0][0], pi.z[0][1]);
-        prefixBoundary = (!pi.edgepaths.length && edgeVal2 > pi.level);
-    }
-
-    if(prefixBoundary) {
-        // TODO: why does ^^ not work for constraints?
-        // pi.prefixBoundary gets set by closeBoundaries
-        return 'M' + perimeter.join('L') + 'Z';
-    }
-    return '';
-}
-
 function joinAllPaths(pi, perimeter) {
-    var fullpath = initFullPath(pi, perimeter);
+    var fullpath = '';
     var i = 0;
     var startsleft = pi.edgepaths.map(function(v, i) { return i; });
     var newloop = true;
@@ -612,17 +610,18 @@ exports.drawLabels = function(labelGroup, labelData, gd, lineClip, labelClipPath
 };
 
 function clipGaps(plotGroup, plotinfo, gd, cd0, perimeter) {
+    var trace = cd0.trace;
     var clips = gd._fullLayout._clips;
-    var clipId = 'clip' + cd0.trace.uid;
+    var clipId = 'clip' + trace.uid;
 
     var clipPath = clips.selectAll('#' + clipId)
-        .data(cd0.trace.connectgaps ? [] : [0]);
+        .data(trace.connectgaps ? [] : [0]);
     clipPath.enter().append('clipPath')
         .classed('contourclip', true)
         .attr('id', clipId);
     clipPath.exit().remove();
 
-    if(cd0.trace.connectgaps === false) {
+    if(trace.connectgaps === false) {
         var clipPathInfo = {
             // fraction of the way from missing to present point
             // to draw the boundary.
@@ -644,10 +643,13 @@ function clipGaps(plotGroup, plotinfo, gd, cd0, perimeter) {
 
         makeCrossings([clipPathInfo]);
         findAllPaths([clipPathInfo]);
-        var fullpath = joinAllPaths(clipPathInfo, perimeter);
+        closeBoundaries([clipPathInfo], {type: 'levels'});
 
         var path = Lib.ensureSingle(clipPath, 'path', '');
-        path.attr('d', fullpath);
+        path.attr('d',
+            (clipPathInfo.prefixBoundary ? 'M' + perimeter.join('L') + 'Z' : '') +
+            joinAllPaths(clipPathInfo, perimeter)
+        );
     } else clipId = null;
 
     Drawing.setClipUrl(plotGroup, clipId, gd);

Diff for: src/traces/contourcarpet/index.js

@@ -19,7 +19,7 @@ module.exports = {
     moduleType: 'trace',
     name: 'contourcarpet',
     basePlotModule: require('../../plots/cartesian'),
-    categories: ['cartesian', 'svg', 'carpet', 'contour', 'symbols', 'showLegend', 'hasLines', 'carpetDependent'],
+    categories: ['cartesian', 'svg', 'carpet', 'contour', 'symbols', 'showLegend', 'hasLines', 'carpetDependent', 'noHover'],
     meta: {
         hrName: 'contour_carpet',
         description: [