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.

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

Merged
merged 23 commits into from
Jun 9, 2017
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
d0af01b
axis.constrain and axis.constraintoward
alexcjohnson Jun 1, 2017
cc705c3
robustify plot_test
alexcjohnson Jun 1, 2017
b3a3bb7
simplify plot_test
alexcjohnson Jun 1, 2017
27fc2a9
robustify plot_api_test
alexcjohnson Jun 1, 2017
d937732
simplify hover_label_test
alexcjohnson Jun 2, 2017
1570274
no really, initInteractions! fix #1044... again
alexcjohnson Jun 2, 2017
72896ec
robustify drawing_test
alexcjohnson Jun 2, 2017
ee3211a
robustify gl_plot_interact_test
alexcjohnson Jun 2, 2017
f12ff73
streamline gl2d_click_test
alexcjohnson Jun 2, 2017
5f5214e
fix bug with unhover in gl2d
alexcjohnson Jun 2, 2017
9df6903
robustify mapbox_test
alexcjohnson Jun 2, 2017
d0b15c8
tests of axis domain constraints
alexcjohnson Jun 2, 2017
d67829e
create constraints.clean to get it out of the main Plotly.plot code
alexcjohnson Jun 8, 2017
b1c6b24
more robust way to generate relayout args during axis drag
alexcjohnson Jun 8, 2017
59a092a
update constraints.js to handle some more editing edge cases
alexcjohnson Jun 8, 2017
51fc2c6
put range 0,1 parts separately into GUI relayout calls
alexcjohnson Jun 8, 2017
3931973
rm unused `attrs` variable
alexcjohnson Jun 8, 2017
1271382
axes.expand: ensure domain constraint before adjusting extrappad
alexcjohnson Jun 8, 2017
6d3a48a
add special validateFunction for enumerated valType
etpinard Jun 8, 2017
3b0d829
add special handler in Plotly.validate for enumerated w/ dynamic values
etpinard Jun 8, 2017
4002fc1
Merge pull request #1769 from plotly/validate-dynamic-enumerated
etpinard Jun 8, 2017
fa78376
:hocho: debug cruft
alexcjohnson Jun 9, 2017
67ea3a8
test axis constraints (range & domain) with log and category axes
alexcjohnson Jun 9, 2017
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
32 changes: 32 additions & 0 deletions src/constants/alignment.js
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: {
Copy link
Contributor

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.

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, 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 and FROM_TR...) to make things clearer and more efficient. see eg ag middle: in the src directory. I guess perhaps all of these could also allow [0, 1].

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
}
};
64 changes: 51 additions & 13 deletions src/plot_api/plot_api.js
Original file line number Diff line number Diff line change
Expand Up @@ -190,8 +190,7 @@ Plotly.plot = function(gd, data, layout, config) {

return Lib.syncOrAsync([
subroutines.layoutStyles,
drawAxes,
initInteractions
drawAxes
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I guess initialising interactions after pushing the margins makes sense 👍

], gd);
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -365,11 +383,13 @@ Plotly.plot = function(gd, data, layout, config) {
drawFramework,
marginPushers,
marginPushersAgain,
initInteractions,
Copy link
Contributor

Choose a reason for hiding this comment

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

for reference (because this diff isn't great), we now have:

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 don't understand why we need to initInteractions twice,
but it seems necessary to do both before and after finalDraw

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
];

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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]) {
Expand Down
7 changes: 7 additions & 0 deletions src/plots/cartesian/axes.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting choice for having a condition based on ax._inputDomain.

Wouldn't ax.constrain === 'domain' be more robust?

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 possibly... this might get called before enforceAxisConstraints though, so I guess I should filter on both.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

-> tighter condition in 1271382

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 _inputDomain is the right thing to do (rather than having the padding be 5% of the constrained domain), but a big part of MY motivation here was that trying to do it the other way means you'd have to make a totally separate attribute in the ax._min/_max items in addition to val and pad because you'd no longer be able to express the padding either in data (val) or pixel (pad) units, then both the auto-range and the auto-domain routines get more complicated... also it makes the conditions more complicated for which points might possibly be the extremes so must be kept in ax._min/max. I didn't want to open that can of worms.

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); };
Expand Down
21 changes: 18 additions & 3 deletions src/plots/cartesian/constraint_defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'],
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious, does Plotly.validate gives the correct answer here e.g. where constraintoward: 'bottom' in an xaxis container?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

heh, no, Plotly.validate doesn't catch constraintoward: 'bottom' on an x axis, but it also incorrectly complains about anchor and scaleanchor:

LOG: In layout, key xaxis.scaleanchor is set to an invalid value (y)
LOG: In layout, key xaxis.anchor is set to an invalid value (y)

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, push it!

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixing Plotly.validate for scaleanchor, anchor and overlaying was a little harder than I thought actually, so I made a separate PR -> #1769

dflt: letter === 'x' ? 'center' : 'middle'
}
}, 'constraintoward');

if(!containerIn.scaleanchor) return;

var constraintOpts = getConstraintOpts(constraintGroups, thisID, allAxisIds, layoutOut);

var scaleanchor = Lib.coerce(containerIn, containerOut, {
scaleanchor: {
Expand All @@ -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: "' +
Expand Down
116 changes: 113 additions & 3 deletions src/plots/cartesian/constraints.js
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand All @@ -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();
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

still TODO ?

Copy link
Contributor

Choose a reason for hiding this comment

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

ignore. my bad

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed in a later commit I think :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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
];
}
Loading