Skip to content
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

Jump to bottom

Fix reverse autorange on cartesian axes with breaks #4639

Merged
merged 3 commits into from
Mar 13, 2020
Merged

Fix reverse autorange on cartesian axes with breaks #4639

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9c0d92b
simplify logic for axis breaks in zoomAxRanges
archmoj Mar 12, 2020
422bd72
add a failing test for autorange reversed
archmoj Mar 13, 2020
aa1143e
Fix autorange on reversed axes with breaks
archmoj Mar 13, 2020
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
5 changes: 3 additions & 2 deletions src/plots/cartesian/autorange.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,10 +101,11 @@ function getAutoRange(gd, ax) {
if(ax.breaks) {
var breaksOut = ax.locateBreaks(v0, v1);
for(var i = 0; i < breaksOut.length; i++) {
lBreaks += (breaksOut[i].max - breaksOut[i].min);
var brk = breaksOut[i];
lBreaks += brk.max - brk.min;
}
}
return lBreaks;
return (axReverse ? -1 : 1) * lBreaks;
};

var mbest = 0;
Expand Down
4 changes: 2 additions & 2 deletions src/plots/cartesian/axes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,7 +527,7 @@ axes.prepTicks = function(ax) {
if(ax.tickmode === 'array') nt *= 100;


ax._roughDTick = (Math.abs(rng[1] - rng[0]) - (ax._lBreaks || 0)) / nt;
ax._roughDTick = (Math.abs(rng[1] - rng[0]) - Math.abs(ax._lBreaks || 0)) / nt;
axes.autoTicks(ax, ax._roughDTick);

// check for a forced minimum dtick
Expand Down Expand Up @@ -1009,7 +1009,7 @@ axes.tickText = function(ax, x, hover, noSuffixPrefix) {

if(arrayMode && Array.isArray(ax.ticktext)) {
var rng = Lib.simpleMap(ax.range, ax.r2l);
var minDiff = (Math.abs(rng[1] - rng[0]) - (ax._lBreaks || 0)) / 10000;
var minDiff = (Math.abs(rng[1] - rng[0]) - Math.abs(ax._lBreaks || 0)) / 10000;

for(i = 0; i < ax.ticktext.length; i++) {
if(Math.abs(x - tickVal2l(ax.tickvals[i])) < minDiff) break;
Expand Down
13 changes: 6 additions & 7 deletions src/plots/cartesian/dragbox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -987,13 +987,12 @@ function zoomAxRanges(axList, r0Fraction, r1Fraction, updates, linkedAxes) {
if(axi.fixedrange) continue;

if(axi.breaks) {
if(axi._id.charAt(0) === 'y') {
updates[axi._name + '.range[0]'] = axi.l2r(axi.p2l((1 - r0Fraction) * axi._length));
updates[axi._name + '.range[1]'] = axi.l2r(axi.p2l((1 - r1Fraction) * axi._length));
} else {
updates[axi._name + '.range[0]'] = axi.l2r(axi.p2l(r0Fraction * axi._length));
updates[axi._name + '.range[1]'] = axi.l2r(axi.p2l(r1Fraction * axi._length));
}
var isY = axi._id.charAt(0) === 'y';
var r0F = isY ? (1 - r0Fraction) : r0Fraction;
var r1F = isY ? (1 - r1Fraction) : r1Fraction;

updates[axi._name + '.range[0]'] = axi.l2r(axi.p2l(r0F * axi._length));
updates[axi._name + '.range[1]'] = axi.l2r(axi.p2l(r1F * axi._length));
} else {
var axRangeLinear0 = axi._rl[0];
var axRangeLinearSpan = axi._rl[1] - axRangeLinear0;
Expand Down
123 changes: 57 additions & 66 deletions src/plots/cartesian/set_convert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,67 +189,58 @@ module.exports = function setConvert(ax, fullLayout) {
};

if(ax.breaks) {
if(axLetter === 'y') {
l2p = function(v) {
if(!isNumeric(v)) return BADNUM;
if(!ax._breaks.length) return _l2p(v, ax._m, ax._b);

var b = ax._B[0];
for(var i = 0; i < ax._breaks.length; i++) {
var brk = ax._breaks[i];
if(v <= brk.min) b = ax._B[i + 1];
else if(v > brk.min && v < brk.max) {
// when v falls into break, pick offset 'closest' to it
if(v - brk.min <= brk.max - v) b = ax._B[i + 1];
else b = ax._B[i];
break;
} else if(v > brk.max) break;
}
return _l2p(v, -ax._m2, b);
};
p2l = function(px) {
if(!isNumeric(px)) return BADNUM;
if(!ax._breaks.length) return _p2l(px, ax._m, ax._b);

var b = ax._B[0];
for(var i = 0; i < ax._breaks.length; i++) {
var brk = ax._breaks[i];
if(px >= brk.pmin) b = ax._B[i + 1];
else if(px < brk.pmax) break;
}
return _p2l(px, -ax._m2, b);
};
} else {
l2p = function(v) {
if(!isNumeric(v)) return BADNUM;
if(!ax._breaks.length) return _l2p(v, ax._m, ax._b);

var b = ax._B[0];
for(var i = 0; i < ax._breaks.length; i++) {
var brk = ax._breaks[i];
if(v >= brk.max) b = ax._B[i + 1];
else if(v > brk.min && v < brk.max) {
// when v falls into break, pick offset 'closest' to it
if(v - brk.min <= brk.max - v) b = ax._B[i];
else b = ax._B[i + 1];
break;
} else if(v < brk.min) break;
l2p = function(v) {
if(!isNumeric(v)) return BADNUM;
var len = ax._breaks.length;
if(!len) return _l2p(v, ax._m, ax._b);

var isY = axLetter === 'y';
var pos = isY ? -v : v;

var q = 0;
for(var i = 0; i < len; i++) {
var nextI = i + 1;
var brk = ax._breaks[i];

var min = isY ? -brk.max : brk.min;
var max = isY ? -brk.min : brk.max;

if(pos < min) break;
if(pos > max) q = nextI;
else {
// when falls into break, pick 'closest' offset
q = pos > (min + max) / 2 ? nextI : i;
break;
}
return _l2p(v, ax._m2, b);
};
p2l = function(px) {
if(!isNumeric(px)) return BADNUM;
if(!ax._breaks.length) return _p2l(px, ax._m, ax._b);

var b = ax._B[0];
for(var i = 0; i < ax._breaks.length; i++) {
var brk = ax._breaks[i];
if(px >= brk.pmax) b = ax._B[i + 1];
else if(px < brk.pmin) break;
}
return _l2p(v, (isY ? -1 : 1) * ax._m2, ax._B[q]);
};

p2l = function(px) {
if(!isNumeric(px)) return BADNUM;
var len = ax._breaks.length;
if(!len) return _p2l(px, ax._m, ax._b);

var isY = axLetter === 'y';
var pos = isY ? -px : px;

var q = 0;
for(var i = 0; i < len; i++) {
var nextI = i + 1;
var brk = ax._breaks[i];

var min = isY ? -brk.pmax : brk.pmin;
var max = isY ? -brk.pmin : brk.pmax;

if(pos < min) break;
if(pos > max) q = nextI;
else {
q = i;
break;
}
return _p2l(px, ax._m2, b);
};
}
}
return _p2l(px, (isY ? -1 : 1) * ax._m2, ax._B[q]);
};
}

// conversions among c/l/p are fairly simple - do them together for all axis types
Expand Down Expand Up @@ -561,7 +552,7 @@ module.exports = function setConvert(ax, fullLayout) {

// set of "N" disjoint breaks inside the range
ax._breaks = [];
// length of these breaks in value space
// length of these breaks in value space - negative on reversed axes
ax._lBreaks = 0;
// l2p slope (same for all intervals)
ax._m2 = 0;
Expand All @@ -575,12 +566,13 @@ module.exports = function setConvert(ax, fullLayout) {
Math.min(rl0, rl1),
Math.max(rl0, rl1)
);
var signAx = rl0 > rl1 ? -1 : 1;
var axReverse = rl0 > rl1;
var signAx = axReverse ? -1 : 1;

if(ax._breaks.length) {
for(i = 0; i < ax._breaks.length; i++) {
brk = ax._breaks[i];
ax._lBreaks += (brk.max - brk.min);
ax._lBreaks += brk.max - brk.min;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

One small recommendation: I think it would be best to keep ax._lBreaks positive-definite. So many below this loop we could have

ax._lBreaks = Math.abs(ax._lBreaks)

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that at one point. However, allowing negative helped handling reversed cases much easier. For now I added a comment in front of _lBreaks description.

Copy link
Contributor

Choose a reason for hiding this comment

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

For now I added a comment in front of _lBreaks description.

That's great!


ax._m2 = ax._length / (rl1 - rl0 - ax._lBreaks * signAx);
Expand All @@ -597,17 +589,16 @@ module.exports = function setConvert(ax, fullLayout) {
brk = ax._breaks[i];
ax._B.push(ax._B[ax._B.length - 1] - ax._m2 * (brk.max - brk.min) * signAx);
}

if(signAx === -1) {
if(axReverse) {
ax._B.reverse();
}

// fill pixel (i.e. 'p') min/max here,
// to not have to loop through the _breaks twice during `p2l`
for(i = 0; i < ax._breaks.length; i++) {
brk = ax._breaks[i];
brk.pmin = l2p(brk.min);
brk.pmax = l2p(brk.max);
brk.pmin = l2p(axReverse ? brk.max : brk.min);
brk.pmax = l2p(axReverse ? brk.min : brk.max);
}
}
}
Expand Down
Binary file added test/image/baselines/axes_breaks-night_autorange-reversed.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading