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 all commits
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
60 changes: 56 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,51 @@ 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 plotinfo = fullLayout._plots[counterAxis._mainSubplot];
if(!(plotinfo.mainplotinfo || plotinfo).overlays.length) {
return lineNearZero(counterAxis, zeroPosition);
}

var counterLetterAxes = axes.list(gd, counterLetter);
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 +2227,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
142 changes: 142 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,148 @@ 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 coupled 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.mirror': 'allticks', 'yaxis.mirror': 'allticks'});
})
.then(function() {
// allticks works the same as all
assertZeroLines(['x', 'y']);
})
.catch(failTest)
.then(done);
});

it('works with multiple overlaid subplots', function(done) {
Plotly.newPlot(gd, [
{x: [1, 2, 3], y: [1, 2, 3]},
{x: [1, 2, 3], y: [1, 2, 3], xaxis: 'x2', yaxis: 'y2'}
], {
xaxis: {range: [0, 4], showzeroline: true},
yaxis: {range: [0, 4], showzeroline: true},
xaxis2: {range: [0, 4], showzeroline: true, side: 'top', overlaying: 'x'},
yaxis2: {range: [0, 4], showzeroline: true, side: 'right', overlaying: 'y'},
width: 600,
height: 600
})
.then(function() {
assertZeroLines(['x', 'y', 'x2', 'y2']);
return Plotly.relayout(gd, {'xaxis.showline': true, 'yaxis.showline': true});
})
.then(function() {
assertZeroLines([]);
return Plotly.relayout(gd, {
'xaxis.range': [4, 0],
'yaxis.range': [4, 0],
'xaxis2.range': [4, 0],
'yaxis2.range': [4, 0]
});
})
.then(function() {
assertZeroLines(['x', 'y', 'x2', 'y2']);
return Plotly.relayout(gd, {
'xaxis.showline': false,
'yaxis.showline': false,
'xaxis2.showline': true,
'yaxis2.showline': true
});
})
.then(function() {
assertZeroLines([]);
})
.catch(failTest)
.then(done);
});
});
});

function getZoomInButton(gd) {
Expand Down