Skip to content

fix logic for hiding zero lines when they conflict with axis lines #2936

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 4 commits into from
Aug 22, 2018
Merged

fix logic for hiding zero lines when they conflict with axis lines #2936

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ef53e31
fix logic for hiding zero lines when they conflict with axis lines
alexcjohnson Aug 22, 2018
545ad47
lock down equivalence of all/allticks for zeroline visibility
alexcjohnson Aug 22, 2018
bcd7c06
add zeroline visibility test for overlaying subplots
alexcjohnson Aug 22, 2018
0f59ca2
speed up zero line logic - remove `axes.list` when possible
alexcjohnson Aug 22, 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
11 changes: 6 additions & 5 deletions src/plot_api/subroutines.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,9 @@ function lsInner(gd) {

// figure out the main axis line and main mirror line position.
// it's easier to follow the logic if we handle these separately from
// ax._linepositions, which are really only used by mirror=allticks
// for the non-main-subplot ticks.
// ax._linepositions, which are only used by mirror=allticks
// for non-main-subplot ticks, and mirror=all(ticks)? for zero line
// hiding logic
ax._mainLinePosition = getLinePosition(ax, counterAx, ax.side);
ax._mainMirrorPosition = (ax.mirror && counterAx) ?
getLinePosition(ax, counterAx,
Expand Down Expand Up @@ -270,7 +271,7 @@ function lsInner(gd) {
// each subplot that gets ticks from "allticks" gets an entry:
// [left or bottom, right or top]
extraSubplot = (!xa._anchorAxis || subplot !== xa._mainSubplot);
if(extraSubplot && xa.ticks && xa.mirror === 'allticks') {
if(extraSubplot && (xa.mirror === 'allticks' || xa.mirror === 'all')) {
xa._linepositions[subplot] = [xLinesYBottom, xLinesYTop];
}

Expand Down Expand Up @@ -306,8 +307,8 @@ function lsInner(gd) {
yLinesXLeft = getLinePosition(ya, xa, 'left');
yLinesXRight = getLinePosition(ya, xa, 'right');

extraSubplot = (!ya._anchorAxis || subplot !== xa._mainSubplot);
if(extraSubplot && ya.ticks && ya.mirror === 'allticks') {
extraSubplot = (!ya._anchorAxis || subplot !== ya._mainSubplot);
if(extraSubplot && (ya.mirror === 'allticks' || ya.mirror === 'all')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice find! Is there a test 🔒 ing this down?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ha, there was a test (the subplot case) locking down the changes but not the parts that remained -> added these in 545ad47

ya._linepositions[subplot] = [yLinesXLeft, yLinesXRight];
}

Expand Down
55 changes: 51 additions & 4 deletions src/plots/cartesian/axes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2146,6 +2146,46 @@ axes.doTicksSingle = function(gd, arg, skipTitle) {
return trace.fill && trace.fill.charAt(trace.fill.length - 1) === axLetter;
}

function lineNearZero(ax2, position) {
if(!ax2.showline || !ax2.linewidth) return false;
var tolerance = Math.max((ax2.linewidth + ax.zerolinewidth) / 2, 1);

function closeEnough(pos2) {
return typeof pos2 === 'number' && Math.abs(pos2 - position) < tolerance;
}

if(closeEnough(ax2._mainLinePosition) || closeEnough(ax2._mainMirrorPosition)) {
return true;
}
var linePositions = ax2._linepositions || {};
for(var k in linePositions) {
if(closeEnough(linePositions[k][0]) || closeEnough(linePositions[k][1])) {
return true;
}
}
}

function anyCounterAxLineAtZero(counterAxis, rng) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice function name.

var mainCounterAxis = counterAxis._mainAxis;
if(!mainCounterAxis) return;

var zeroPosition = ax._offset + (
((Math.abs(rng[0]) < Math.abs(rng[1])) === (axLetter === 'x')) ?
0 : ax._length
);

var counterLetterAxes = axes.list(gd, counterLetter);
Copy link
Contributor

Choose a reason for hiding this comment

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

It would nice to avoid looping over all axes here.

Can we skip this loop when fullLayout._plots[counterAxis._mainSubplot].overlays is empty, and only check lineNearZero(counterAxis, zeroPosition)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good call - it was almost that simple. one more relevant test in bcd7c06 and 🐎 in 0f59ca2

Copy link
Contributor

Choose a reason for hiding this comment

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

Good to hear it was that easy. 💯

for(var i = 0; i < counterLetterAxes.length; i++) {
var counterAxis2 = counterLetterAxes[i];
if(
counterAxis2._mainAxis === mainCounterAxis &&
lineNearZero(counterAxis2, zeroPosition)
) {
return true;
}
}
}

function drawGrid(plotinfo, counteraxis, subplot) {
if(fullLayout._hasOnlyLargeSploms) return;

Expand Down Expand Up @@ -2182,12 +2222,19 @@ axes.doTicksSingle = function(gd, arg, skipTitle) {
break;
}
}
var rng = Lib.simpleMap(ax.range, ax.r2l),
showZl = (rng[0] * rng[1] <= 0) && ax.zeroline &&
var rng = Lib.simpleMap(ax.range, ax.r2l);
var zlData = {x: 0, id: axid};
Copy link
Contributor

Choose a reason for hiding this comment

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

No need this do in this PR (opening a new issue would be fine), but we'll need to propagate these changes to large-sploms' regl-based grid line logic:

plotly.js/src/traces/splom/base_plot.js

Lines 171 to 183 in b5f8b23

// just like in Axes.doTicks but without the loop over traces
function showZeroLine(ax) {
var rng = Lib.simpleMap(ax.range, ax.r2l);
var p0 = ax.l2p(0);
return (
ax.zeroline &&
ax._vals && ax._vals.length &&
(rng[0] * rng[1] <= 0) &&
(ax.type === 'linear' || ax.type === '-') &&
((p0 > 1 && p0 < ax._length - 1) || !ax.showline)
);
}

where I didn't just reuse the axes.js logic because I wanted to avoid looping over the traces (to compute that hasBarsOrFill boolean). Thinking about this again, looping over traces isn't really a performance hit for sploms (I can't really imagine a user wanting to plot more 10 splom traces on the same graph).

So it would be worth trying to expose and ♻️ this logic here for large-sploms.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, thanks for pointing that out! #2938


var showZl = (rng[0] * rng[1] <= 0) && ax.zeroline &&
(ax.type === 'linear' || ax.type === '-') && gridvals.length &&
(hasBarsOrFill || clipEnds({x: 0}) || !ax.showline);
(
hasBarsOrFill ||
clipEnds(zlData) ||
!anyCounterAxLineAtZero(counteraxis, rng)
);

var zl = zlcontainer.selectAll('path.' + zcls)
.data(showZl ? [{x: 0, id: axid}] : []);
.data(showZl ? [zlData] : []);
zl.enter().append('path').classed(zcls, 1).classed('zl', 1)
.classed('crisp', 1)
.attr('d', gridpath)
Expand Down
2 changes: 1 addition & 1 deletion src/plots/cartesian/layout_attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,7 +573,7 @@ module.exports = {
valType: 'boolean',
dflt: false,
role: 'style',
editType: 'layoutstyle',
editType: 'ticks+layoutstyle',
description: [
'Determines whether or not a line bounding this axis is drawn.'
].join(' ')
Expand Down
97 changes: 97 additions & 0 deletions test/jasmine/tests/axes_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2783,6 +2783,103 @@ describe('Test axes', function() {

});
});

describe('zeroline visibility logic', function() {
var gd;
beforeEach(function() {
gd = createGraphDiv();
});
afterEach(destroyGraphDiv);

function assertZeroLines(expectedIDs) {
var sortedIDs = expectedIDs.slice().sort();
var zlIDs = [];
d3.select(gd).selectAll('.zl').each(function() {
var cls = d3.select(this).attr('class');
var clsMatch = cls.match(/[xy]\d*(?=zl)/g)[0];
zlIDs.push(clsMatch);
});
zlIDs.sort();
expect(zlIDs).toEqual(sortedIDs);
}

it('works with a single subplot', function(done) {
Plotly.newPlot(gd, [{x: [1, 2, 3], y: [1, 2, 3]}], {
xaxis: {range: [0, 4], showzeroline: true, showline: true},
yaxis: {range: [0, 4], showzeroline: true, showline: true},
width: 600,
height: 600
})
.then(function() {
assertZeroLines([]);
return Plotly.relayout(gd, {'xaxis.showline': false});
})
.then(function() {
assertZeroLines(['y']);
return Plotly.relayout(gd, {'xaxis.showline': true, 'yaxis.showline': false});
})
.then(function() {
assertZeroLines(['x']);
return Plotly.relayout(gd, {'yaxis.showline': true, 'yaxis.range': [4, 0]});
})
.then(function() {
assertZeroLines(['y']);
return Plotly.relayout(gd, {'xaxis.range': [4, 0], 'xaxis.side': 'top'});
})
.then(function() {
assertZeroLines(['x']);
return Plotly.relayout(gd, {'yaxis.side': 'right', 'xaxis.anchor': 'free', 'xaxis.position': 1});
})
.then(function() {
assertZeroLines([]);
return Plotly.relayout(gd, {'xaxis.range': [0, 4], 'yaxis.range': [0, 4]});
})
.then(function() {
assertZeroLines(['x', 'y']);
return Plotly.relayout(gd, {'xaxis.mirror': 'all', 'yaxis.mirror': true});
})
.then(function() {
assertZeroLines([]);
return Plotly.relayout(gd, {'xaxis.range': [-0.1, 4], 'yaxis.range': [-0.1, 4]});
})
.then(function() {
assertZeroLines(['x', 'y']);
})
.catch(failTest)
.then(done);
});

it('works with multiple subplots', function(done) {
Plotly.newPlot(gd, [
{x: [1, 2, 3], y: [1, 2, 3]},
{x: [1, 2, 3], y: [1, 2, 3], xaxis: 'x2'},
{x: [1, 2, 3], y: [1, 2, 3], yaxis: 'y2'}
], {
xaxis: {range: [0, 4], showzeroline: true, domain: [0, 0.4]},
yaxis: {range: [0, 4], showzeroline: true, domain: [0, 0.4]},
xaxis2: {range: [0, 4], showzeroline: true, domain: [0.6, 1]},
yaxis2: {range: [0, 4], showzeroline: true, domain: [0.6, 1]},
width: 600,
height: 600
})
.then(function() {
assertZeroLines(['x', 'x', 'y', 'y', 'x2', 'y2']);
return Plotly.relayout(gd, {'xaxis.showline': true, 'xaxis.mirror': 'all'});
})
.then(function() {
assertZeroLines(['x', 'x', 'y', 'x2']);
return Plotly.relayout(gd, {'yaxis.showline': true, 'yaxis.mirror': 'all'});
})
.then(function() {
// x axis still has a zero line on xy2, and y on x2y
// all the others have disappeared now
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for the 📚

assertZeroLines(['x', 'y']);
return Plotly.relayout(gd, {'xaxis.showline': true, 'xaxis.mirror': 'all'});
})
.catch(failTest)
.then(done);
});
});
});

function getZoomInButton(gd) {
Expand Down