-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Constrain axes by domain #1767
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.
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
Constrain axes by domain #1767
Changes from 11 commits
d0af01b
cc705c3
b3a3bb7
27fc2a9
d937732
1570274
72896ec
ee3211a
f12ff73
5f5214e
9df6903
d0b15c8
d67829e
b1c6b24
59a092a
51fc2c6
3931973
1271382
6d3a48a
3b0d829
4002fc1
fa78376
67ea3a8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
/** | ||
* Copyright 2012-2017, Plotly, Inc. | ||
* All rights reserved. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
'use strict'; | ||
|
||
// fraction of some size to get to a named position | ||
module.exports = { | ||
// from bottom left: this is the origin of our paper-reference | ||
// positioning system | ||
FROM_BL: { | ||
left: 0, | ||
center: 0.5, | ||
right: 1, | ||
bottom: 0, | ||
middle: 0.5, | ||
top: 1 | ||
}, | ||
// from top left: this is the screen pixel positioning origin | ||
FROM_TL: { | ||
left: 0, | ||
center: 0.5, | ||
right: 1, | ||
bottom: 1, | ||
middle: 0.5, | ||
top: 0 | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -190,8 +190,7 @@ Plotly.plot = function(gd, data, layout, config) { | |
|
||
return Lib.syncOrAsync([ | ||
subroutines.layoutStyles, | ||
drawAxes, | ||
initInteractions | ||
drawAxes | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I guess initialising interactions after pushing the margins makes sense 👍 |
||
], gd); | ||
} | ||
|
||
|
@@ -220,19 +219,19 @@ Plotly.plot = function(gd, data, layout, config) { | |
|
||
// in case the margins changed, draw margin pushers again | ||
function marginPushersAgain() { | ||
var seq = JSON.stringify(fullLayout._size) === oldmargins ? | ||
[] : | ||
[marginPushers, subroutines.layoutStyles]; | ||
if(JSON.stringify(fullLayout._size) === oldmargins) return; | ||
|
||
// re-initialize cartesian interaction, | ||
// which are sometimes cleared during marginPushers | ||
seq = seq.concat(initInteractions); | ||
|
||
return Lib.syncOrAsync(seq, gd); | ||
return Lib.syncOrAsync([ | ||
marginPushers, | ||
subroutines.layoutStyles | ||
], gd); | ||
} | ||
|
||
function positionAndAutorange() { | ||
if(!recalc) return; | ||
if(!recalc) { | ||
enforceAxisConstraints(gd); | ||
return; | ||
} | ||
|
||
var subplots = Plots.getSubplotIds(fullLayout, 'cartesian'), | ||
modules = fullLayout._modules; | ||
|
@@ -270,7 +269,26 @@ Plotly.plot = function(gd, data, layout, config) { | |
|
||
var axList = Plotly.Axes.list(gd, '', true); | ||
for(var i = 0; i < axList.length; i++) { | ||
Plotly.Axes.doAutoRange(axList[i]); | ||
// before autoranging, check if this axis was previously constrained | ||
// by domain but no longer is | ||
var ax = axList[i]; | ||
if(ax._inputDomain) { | ||
var isConstrained = false; | ||
var axId = ax._id; | ||
var constraintGroups = gd._fullLayout._axisConstraintGroups; | ||
for(var j = 0; j < constraintGroups.length; j++) { | ||
if(constraintGroups[j][axId]) { | ||
isConstrained = true; | ||
break; | ||
} | ||
} | ||
if(!isConstrained || ax.constrain !== 'domain') { | ||
ax._input.domain = ax.domain = ax._inputDomain; | ||
delete ax._inputDomain; | ||
} | ||
} | ||
|
||
Plotly.Axes.doAutoRange(ax); | ||
} | ||
|
||
enforceAxisConstraints(gd); | ||
|
@@ -365,11 +383,13 @@ Plotly.plot = function(gd, data, layout, config) { | |
drawFramework, | ||
marginPushers, | ||
marginPushersAgain, | ||
initInteractions, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just want to call out a note I put in a commit message:
I'll have a perf PR coming after this merges and will look into that in more detail. |
||
positionAndAutorange, | ||
subroutines.layoutStyles, | ||
drawAxes, | ||
drawData, | ||
finalDraw, | ||
initInteractions, | ||
Plots.rehover | ||
]; | ||
|
||
|
@@ -1917,10 +1937,12 @@ function _relayout(gd, aobj) { | |
// we're editing the (auto)range of, so we can tell the others constrained | ||
// to scale with them that it's OK for them to shrink | ||
var rangesAltered = {}; | ||
var axId; | ||
|
||
function recordAlteredAxis(pleafPlus) { | ||
var axId = axisIds.name2id(pleafPlus.split('.')[0]); | ||
rangesAltered[axId] = 1; | ||
return axId; | ||
} | ||
|
||
// alter gd.layout | ||
|
@@ -1963,11 +1985,26 @@ function _relayout(gd, aobj) { | |
else if(pleafPlus.match(/^[xyz]axis[0-9]*\.range(\[[0|1]\])?$/)) { | ||
doextra(ptrunk + '.autorange', false); | ||
recordAlteredAxis(pleafPlus); | ||
Lib.nestedProperty(fullLayout, ptrunk + '._inputRange').set(null); | ||
} | ||
else if(pleafPlus.match(/^[xyz]axis[0-9]*\.autorange$/)) { | ||
doextra([ptrunk + '.range[0]', ptrunk + '.range[1]'], | ||
undefined); | ||
recordAlteredAxis(pleafPlus); | ||
Lib.nestedProperty(fullLayout, ptrunk + '._inputRange').set(null); | ||
var axFull = Lib.nestedProperty(fullLayout, ptrunk).get(); | ||
if(axFull._inputDomain) { | ||
// if we're autoranging and this axis has a constrained domain, | ||
// reset it so we don't get locked into a shrunken size | ||
axFull._input.domain = axFull._inputDomain.slice(); | ||
} | ||
} | ||
else if(pleafPlus.match(/^[xyz]axis[0-9]*\.domain(\[[0|1]\])?$/)) { | ||
axId = recordAlteredAxis(pleafPlus); | ||
Lib.nestedProperty(fullLayout, ptrunk + '._inputDomain').set(null); | ||
} | ||
else if(pleafPlus.match(/^[xyz]axis[0-9]*\.constrain.*$/)) { | ||
flags.docalc = true; | ||
} | ||
else if(pleafPlus.match(/^aspectratio\.[xyz]$/)) { | ||
doextra(proot + '.aspectmode', 'manual'); | ||
|
@@ -2047,6 +2084,7 @@ function _relayout(gd, aobj) { | |
// will not make sense, so autorange it. | ||
doextra(ptrunk + '.autorange', true); | ||
} | ||
Lib.nestedProperty(fullLayout, ptrunk + '._inputRange').set(null); | ||
} | ||
else if(pleaf.match(cartesianConstants.AX_NAME_PATTERN)) { | ||
var fullProp = Lib.nestedProperty(fullLayout, ai).get(), | ||
|
@@ -2193,7 +2231,7 @@ function _relayout(gd, aobj) { | |
|
||
// figure out if we need to recalculate axis constraints | ||
var constraints = fullLayout._axisConstraintGroups; | ||
for(var axId in rangesAltered) { | ||
for(axId in rangesAltered) { | ||
for(i = 0; i < constraints.length; i++) { | ||
var group = constraints[i]; | ||
if(group[axId]) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -455,6 +455,13 @@ axes.expand = function(ax, data, options) { | |
i, j, v, di, dmin, dmax, | ||
ppadiplus, ppadiminus, includeThis, vmin, vmax; | ||
|
||
// domain-constrained axes: base extrappad on the unconstrained | ||
// domain so it's consistent as the domain changes | ||
if(extrappad && ax._inputDomain) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting choice for having a condition based on Wouldn't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah possibly... this might get called before There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. -> tighter condition in 1271382 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW I think there's a good argument that basing this padding on But if anyone has a strong opinion that padding based on the constrained domain is truly the correct behavior we can certainly discuss. The biggest argument in that direction that I can see is that if your data make a perfect square but it's padded on constrained axes, the subplot itself will not end up precisely square. But if you've gotten to that point, perhaps you could be bothered to explicitly set up the domains and dimensions so the axes have equal length :) |
||
extrappad *= (ax._inputDomain[1] - ax._inputDomain[0]) / | ||
(ax.domain[1] - ax.domain[0]); | ||
} | ||
|
||
function getPad(item) { | ||
if(Array.isArray(item)) { | ||
return function(i) { return Math.max(Number(item[i]||0), 0); }; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,10 +15,25 @@ var id2name = require('./axis_ids').id2name; | |
|
||
module.exports = function handleConstraintDefaults(containerIn, containerOut, coerce, allAxisIds, layoutOut) { | ||
var constraintGroups = layoutOut._axisConstraintGroups; | ||
var thisID = containerOut._id; | ||
var letter = thisID.charAt(0); | ||
|
||
if(containerOut.fixedrange || !containerIn.scaleanchor) return; | ||
if(containerOut.fixedrange) return; | ||
|
||
var constraintOpts = getConstraintOpts(constraintGroups, containerOut._id, allAxisIds, layoutOut); | ||
// coerce the constraint mechanics even if this axis has no scaleanchor | ||
// because it may be the anchor of another axis. | ||
coerce('constrain'); | ||
Lib.coerce(containerIn, containerOut, { | ||
constraintoward: { | ||
valType: 'enumerated', | ||
values: letter === 'x' ? ['left', 'center', 'right'] : ['bottom', 'middle', 'top'], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it might time to consider having separate attribute declarations for x axes and y axes (similarly for x,y and z error bars #621). The reference page https://plot.ly/javascript/reference/ would benefit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious, does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. heh, no,
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found a way to fix this. Can I push a commit to this branch? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, push it! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixing |
||
dflt: letter === 'x' ? 'center' : 'middle' | ||
} | ||
}, 'constraintoward'); | ||
|
||
if(!containerIn.scaleanchor) return; | ||
|
||
var constraintOpts = getConstraintOpts(constraintGroups, thisID, allAxisIds, layoutOut); | ||
|
||
var scaleanchor = Lib.coerce(containerIn, containerOut, { | ||
scaleanchor: { | ||
|
@@ -37,7 +52,7 @@ module.exports = function handleConstraintDefaults(containerIn, containerOut, co | |
if(!scaleratio) scaleratio = containerOut.scaleratio = 1; | ||
|
||
updateConstraintGroups(constraintGroups, constraintOpts.thisGroup, | ||
containerOut._id, scaleanchor, scaleratio); | ||
thisID, scaleanchor, scaleratio); | ||
} | ||
else if(allAxisIds.indexOf(containerIn.scaleanchor) !== -1) { | ||
Lib.warn('ignored ' + containerOut._name + '.scaleanchor: "' + | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,12 +14,14 @@ var scaleZoom = require('./scale_zoom'); | |
|
||
var ALMOST_EQUAL = require('../../constants/numerical').ALMOST_EQUAL; | ||
|
||
var FROM_BL = require('../../constants/alignment').FROM_BL; | ||
|
||
|
||
module.exports = function enforceAxisConstraints(gd) { | ||
var fullLayout = gd._fullLayout; | ||
var constraintGroups = fullLayout._axisConstraintGroups; | ||
|
||
var i, j, axisID, ax, normScale; | ||
var i, j, axisID, ax, normScale, mode, factor; | ||
|
||
for(i = 0; i < constraintGroups.length; i++) { | ||
var group = constraintGroups[i]; | ||
|
@@ -41,6 +43,9 @@ module.exports = function enforceAxisConstraints(gd) { | |
axisID = axisIDs[j]; | ||
axes[axisID] = ax = fullLayout[id2name(axisID)]; | ||
|
||
if(!ax._inputDomain) ax._inputDomain = ax.domain.slice(); | ||
if(!ax._inputRange) ax._inputRange = ax.range.slice(); | ||
|
||
// set axis scale here so we can use _m rather than | ||
// having to calculate it from length and range | ||
ax.setScale(); | ||
|
@@ -65,10 +70,115 @@ module.exports = function enforceAxisConstraints(gd) { | |
for(j = 0; j < axisIDs.length; j++) { | ||
axisID = axisIDs[j]; | ||
normScale = normScales[axisID]; | ||
ax = axes[axisID]; | ||
mode = ax.constrain; | ||
|
||
// even if the scale didn't change, if we're shrinking domain | ||
// we need to recalculate in case `constraintoward` changed | ||
if(normScale !== matchScale || mode === 'domain') { | ||
factor = normScale / matchScale; | ||
|
||
if(mode === 'range') { | ||
scaleZoom(ax, factor); | ||
} | ||
else { | ||
// mode === 'domain' | ||
|
||
var inputDomain = ax._inputDomain; | ||
var domainShrunk = (ax.domain[1] - ax.domain[0]) / | ||
(inputDomain[1] - inputDomain[0]); | ||
var rangeShrunk = (ax.r2l(ax.range[1]) - ax.r2l(ax.range[0])) / | ||
(ax.r2l(ax._inputRange[1]) - ax.r2l(ax._inputRange[0])); | ||
|
||
factor /= domainShrunk; | ||
|
||
if(factor * rangeShrunk < 1) { | ||
// we've asked to magnify the axis more than we can just by | ||
// enlarging the domain - so we need to constrict range | ||
ax.domain = ax._input.domain = inputDomain.slice(); | ||
scaleZoom(ax, factor); | ||
continue; | ||
} | ||
|
||
if(rangeShrunk < 1) { | ||
// the range has previously been constricted by ^^, but we've | ||
// switched to the domain-constricted regime, so reset range | ||
ax.range = ax._input.range = ax._inputRange.slice(); | ||
factor *= rangeShrunk; | ||
} | ||
|
||
if(normScale !== matchScale) { | ||
scaleZoom(axes[axisID], normScale / matchScale); | ||
// TODO | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. still There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ignore. my bad There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed in a later commit I think :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah sorry - TODO removed here |
||
if(ax.autorange) { | ||
/* | ||
* range & factor may need to change because range was | ||
* calculated for the larger scaling, so some pixel | ||
* paddings may get cut off when we reduce the domain. | ||
* | ||
* This is easier than the regular autorange calculation | ||
* because we already know the scaling `m`, but we still | ||
* need to cut out impossible constraints (like | ||
* annotations with super-long arrows). That's what | ||
* outerMin/Max are for - if the expansion was going to | ||
* go beyond the original domain, it must be impossible | ||
*/ | ||
var rangeMin = Math.min(ax.range[0], ax.range[1]); | ||
var rangeMax = Math.max(ax.range[0], ax.range[1]); | ||
var rangeCenter = (rangeMin + rangeMax) / 2; | ||
var halfRange = rangeMax - rangeCenter; | ||
var outerMin = rangeCenter - halfRange * factor; | ||
var outerMax = rangeCenter + halfRange * factor; | ||
|
||
updateDomain(ax, factor); | ||
ax.setScale(); | ||
var m = Math.abs(ax._m); | ||
var newVal; | ||
var k; | ||
|
||
for(k = 0; k < ax._min.length; k++) { | ||
newVal = ax._min[i].val - ax._min[i].pad / m; | ||
if(newVal > outerMin && newVal < rangeMin) { | ||
rangeMin = newVal; | ||
} | ||
} | ||
|
||
for(k = 0; k < ax._max.length; k++) { | ||
newVal = ax._max[i].val + ax._max[i].pad / m; | ||
if(newVal < outerMax && newVal > rangeMax) { | ||
rangeMax = newVal; | ||
} | ||
} | ||
|
||
ax.range = ax._input.range = (ax.range[0] < ax.range[1]) ? | ||
[rangeMin, rangeMax] : [rangeMax, rangeMin]; | ||
|
||
/* | ||
* In principle this new range can be shifted vs. what | ||
* you saw at the end of a zoom operation, like if you | ||
* have a big bubble on one side and a small bubble on | ||
* the other. | ||
* To fix this we'd have to be doing this calculation | ||
* continuously during the zoom, but it's enough of an | ||
* edge case and a subtle enough effect that I'm going | ||
* to ignore it for now. | ||
*/ | ||
var domainExpand = (rangeMax - rangeMin) / (2 * halfRange); | ||
factor /= domainExpand; | ||
} | ||
|
||
updateDomain(ax, factor); | ||
} | ||
} | ||
} | ||
} | ||
}; | ||
|
||
function updateDomain(ax, factor) { | ||
var inputDomain = ax._inputDomain; | ||
var centerFraction = FROM_BL[ax.constraintoward]; | ||
var center = inputDomain[0] + (inputDomain[1] - inputDomain[0]) * centerFraction; | ||
|
||
ax.domain = ax._input.domain = [ | ||
center + (inputDomain[0] - center) / factor, | ||
center + (inputDomain[1] - center) / factor | ||
]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First steps towards #1577
Nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, wasn't even thinking of that but good point. There are tons of other places we could make use of these (potentially adding
FROM_BR
andFROM_TR
...) to make things clearer and more efficient. see egag middle:
in the src directory. I guess perhaps all of these could also allow[0, 1]
.