fix heatmap gap algo: uniform gaps, all bricks centered on their data

Author: alexcjohnson

src/traces/heatmap/plot.js

@@ -226,63 +226,17 @@ function plotOne(gd, plotinfo, cd) {
     // build the pixel map brick-by-brick
     // cruise through z-matrix row-by-row
     // build a brick at each z-matrix value
-    var yi = ypx(0),
-        yb = [yi, yi],
-        xbi = xrev ? 0 : 1,
-        ybi = yrev ? 0 : 1,
-        // for collecting an average luminosity of the heatmap
-        pixcount = 0,
-        rcount = 0,
-        gcount = 0,
-        bcount = 0,
-        brickWithPadding,
-        xb,
-        j,
-        xi,
-        v,
-        row,
-        c;
-
-    function applyBrickPadding(trace, x0, x1, y0, y1, xIndex, xLength, yIndex, yLength) {
-        var padding = {
-                x0: x0,
-                x1: x1,
-                y0: y0,
-                y1: y1
-            },
-            xEdgeGap = trace.xgap * 2 / 3,
-            yEdgeGap = trace.ygap * 2 / 3,
-            xCenterGap = trace.xgap / 3,
-            yCenterGap = trace.ygap / 3;
-
-        if(yIndex === yLength - 1) { // top edge brick
-            padding.y1 = y1 - yEdgeGap;
-        }
-
-        if(xIndex === xLength - 1) { // right edge brick
-            padding.x0 = x0 + xEdgeGap;
-        }
-
-        if(yIndex === 0) { // bottom edge brick
-            padding.y0 = y0 + yEdgeGap;
-        }
-
-        if(xIndex === 0) { // left edge brick
-            padding.x1 = x1 - xEdgeGap;
-        }
-
-        if(xIndex > 0 && xIndex < xLength - 1) { // brick in the center along x
-            padding.x0 = x0 + xCenterGap;
-            padding.x1 = x1 - xCenterGap;
-        }
-
-        if(yIndex > 0 && yIndex < yLength - 1) { // brick in the center along y
-            padding.y0 = y0 + yCenterGap;
-            padding.y1 = y1 - yCenterGap;
-        }
-
-        return padding;
-    }
+    var yi = ypx(0);
+    var yb = [yi, yi];
+    var xbi = xrev ? 0 : 1;
+    var ybi = yrev ? 0 : 1;
+    // for collecting an average luminosity of the heatmap
+    var pixcount = 0;
+    var rcount = 0;
+    var gcount = 0;
+    var bcount = 0;
+
+    var xb, j, xi, v, row, c;
 
     function setColor(v, pixsize) {
         if(v !== undefined) {
@@ -401,6 +355,14 @@ function plotOne(gd, plotinfo, cd) {
 
         context.putImageData(imageData, 0, 0);
     } else { // zsmooth = false -> filling potentially large bricks works fastest with fillRect
+
+        // gaps do not need to be exact integers, but if they *are* we will get
+        // cleaner edges by rounding at least one edge
+        var xGap = trace.xgap;
+        var yGap = trace.ygap;
+        var xGapLeft = Math.floor(xGap / 2);
+        var yGapTop = Math.floor(yGap / 2);
+
         for(j = 0; j < m; j++) {
             row = z[j];
             yb.reverse();
@@ -421,20 +383,8 @@ function plotOne(gd, plotinfo, cd) {
                 c = setColor(v, (xb[1] - xb[0]) * (yb[1] - yb[0]));
                 context.fillStyle = 'rgba(' + c.join(',') + ')';
 
-                brickWithPadding = applyBrickPadding(trace,
-                                                     xb[0],
-                                                     xb[1],
-                                                     yb[0],
-                                                     yb[1],
-                                                     i,
-                                                     n,
-                                                     j,
-                                                     m);
-
-                context.fillRect(brickWithPadding.x0,
-                                 brickWithPadding.y0,
-                                (brickWithPadding.x1 - brickWithPadding.x0),
-                                (brickWithPadding.y1 - brickWithPadding.y0));
+                context.fillRect(xb[0] + xGapLeft, yb[0] + yGapTop,
+                    xb[1] - xb[0] - xGap, yb[1] - yb[0] - yGap);
             }
         }
     }

test/image/baselines/heatmap_brick_padding.png

@@ -10,6 +10,7 @@ var d3 = require('d3');
 var createGraphDiv = require('../assets/create_graph_div');
 var destroyGraphDiv = require('../assets/destroy_graph_div');
 var supplyAllDefaults = require('../assets/supply_defaults');
+var failTest = require('../assets/fail_test');
 
 
 describe('heatmap supplyDefaults', function() {
@@ -542,52 +543,25 @@ describe('heatmap plot', function() {
             argumentsWithoutPadding = getContextStub.fillRect.calls.allArgs().slice(0);
             return Plotly.plot(gd, mockWithPadding.data, mockWithPadding.layout);
         }).then(function() {
-            var centerXGap = mockWithPadding.data[0].xgap / 3;
-            var centerYGap = mockWithPadding.data[0].ygap / 3;
-            var edgeXGap = mockWithPadding.data[0].xgap * 2 / 3;
-            var edgeYGap = mockWithPadding.data[0].ygap * 2 / 3;
-
-            argumentsWithPadding = getContextStub.fillRect.calls.allArgs().slice(getContextStub.fillRect.calls.allArgs().length - 9);
-            expect(argumentsWithPadding).toEqual([
-                [argumentsWithoutPadding[0][0],
-                    argumentsWithoutPadding[0][1] + edgeYGap,
-                    argumentsWithoutPadding[0][2] - edgeXGap,
-                    argumentsWithoutPadding[0][3] - edgeYGap],
-                [argumentsWithoutPadding[1][0] + centerXGap,
-                    argumentsWithoutPadding[1][1] + edgeYGap,
-                    argumentsWithoutPadding[1][2] - edgeXGap,
-                    argumentsWithoutPadding[1][3] - edgeYGap],
-                [argumentsWithoutPadding[2][0] + edgeXGap,
-                    argumentsWithoutPadding[2][1] + edgeYGap,
-                    argumentsWithoutPadding[2][2] - edgeXGap,
-                    argumentsWithoutPadding[2][3] - edgeYGap],
-                [argumentsWithoutPadding[3][0],
-                    argumentsWithoutPadding[3][1] + centerYGap,
-                    argumentsWithoutPadding[3][2] - edgeXGap,
-                    argumentsWithoutPadding[3][3] - edgeYGap],
-                [argumentsWithoutPadding[4][0] + centerXGap,
-                    argumentsWithoutPadding[4][1] + centerYGap,
-                    argumentsWithoutPadding[4][2] - edgeXGap,
-                    argumentsWithoutPadding[4][3] - edgeYGap],
-                [argumentsWithoutPadding[5][0] + edgeXGap,
-                    argumentsWithoutPadding[5][1] + centerYGap,
-                    argumentsWithoutPadding[5][2] - edgeXGap,
-                    argumentsWithoutPadding[5][3] - edgeYGap],
-                [argumentsWithoutPadding[6][0],
-                    argumentsWithoutPadding[6][1],
-                    argumentsWithoutPadding[6][2] - edgeXGap,
-                    argumentsWithoutPadding[6][3] - edgeYGap],
-                [argumentsWithoutPadding[7][0] + centerXGap,
-                    argumentsWithoutPadding[7][1],
-                    argumentsWithoutPadding[7][2] - edgeXGap,
-                    argumentsWithoutPadding[7][3] - edgeYGap],
-                [argumentsWithoutPadding[8][0] + edgeXGap,
-                    argumentsWithoutPadding[8][1],
-                    argumentsWithoutPadding[8][2] - edgeXGap,
-                    argumentsWithoutPadding[8][3] - edgeYGap
-                ]]);
-            done();
-        });
+            var xGap = mockWithPadding.data[0].xgap;
+            var yGap = mockWithPadding.data[0].ygap;
+            var xGapLeft = xGap / 2;
+            var yGapTop = yGap / 2;
+
+            argumentsWithPadding = getContextStub.fillRect.calls.allArgs()
+                .slice(getContextStub.fillRect.calls.allArgs().length - 25);
+
+            expect(argumentsWithPadding.length).toBe(25);
+
+            argumentsWithPadding.forEach(function(args, i) {
+                expect(args[0]).toBe(argumentsWithoutPadding[i][0] + xGapLeft, i);
+                expect(args[1]).toBe(argumentsWithoutPadding[i][1] + yGapTop, i);
+                expect(args[2]).toBe(argumentsWithoutPadding[i][2] - xGap, i);
+                expect(args[3]).toBe(argumentsWithoutPadding[i][3] - yGap, i);
+            });
+        })
+        .catch(failTest)
+        .then(done);
     });
 });