Skip to content

Axes ref cleanup #1431

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

Merged
merged 10 commits into from
Mar 2, 2017
3 changes: 1 addition & 2 deletions src/components/colorbar/draw.js
Original file line number Diff line number Diff line change
Expand Up @@ -185,11 +185,10 @@ module.exports = function draw(gd, id) {
}

// Prepare the Plotly axis object
handleAxisDefaults(cbAxisIn, cbAxisOut, coerce, axisOptions);
handleAxisDefaults(cbAxisIn, cbAxisOut, coerce, axisOptions, fullLayout);
handleAxisPositionDefaults(cbAxisIn, cbAxisOut, coerce, axisOptions);

cbAxisOut._id = 'y' + id;
cbAxisOut._gd = gd;

// position can't go in through supplyDefaults
// because that restricts it to [0,1]
Expand Down
9 changes: 5 additions & 4 deletions src/plot_api/plot_api.js
Original file line number Diff line number Diff line change
Expand Up @@ -133,12 +133,14 @@ Plotly.plot = function(gd, data, layout, config) {

Plots.supplyDefaults(gd);

var fullLayout = gd._fullLayout;

// Polar plots
if(data && data[0] && data[0].r) return plotPolar(gd, data, layout);

// so we don't try to re-call Plotly.plot from inside
// legend and colorbar, if margins changed
gd._replotting = true;
fullLayout._replotting = true;

// make or remake the framework if we need to
if(graphWasEmpty) makePlotFramework(gd);
Expand All @@ -152,7 +154,6 @@ Plotly.plot = function(gd, data, layout, config) {
// save initial axis range once per graph
if(graphWasEmpty) Plotly.Axes.saveRangeInitial(gd);

var fullLayout = gd._fullLayout;

// prepare the data and find the autorange

Expand Down Expand Up @@ -321,13 +322,13 @@ Plotly.plot = function(gd, data, layout, config) {
Plots.addLinks(gd);

// Mark the first render as complete
gd._replotting = false;
fullLayout._replotting = false;

return Plots.previousPromises(gd);
}

// An initial paint must be completed before these components can be
// correctly sized and the whole plot re-margined. gd._replotting must
// correctly sized and the whole plot re-margined. fullLayout._replotting must
// be set to false before these will work properly.
function finalDraw() {
Registry.getComponentMethod('shapes', 'draw')(gd);
Expand Down
3 changes: 2 additions & 1 deletion src/plot_api/subroutines.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ exports.lsInner = function(gd) {
xa = Plotly.Axes.getFromId(gd, subplot, 'x'),
ya = Plotly.Axes.getFromId(gd, subplot, 'y');

xa.setScale(); // this may already be done... not sure
// reset scale in case the margins have changed
xa.setScale();
ya.setScale();

if(plotinfo.bg) {
Expand Down
14 changes: 5 additions & 9 deletions src/plots/cartesian/axes.js
Original file line number Diff line number Diff line change
Expand Up @@ -328,14 +328,10 @@ axes.doAutoRange = function(ax) {

// doAutoRange will get called on fullLayout,
// but we want to report its results back to layout
var axIn = ax._gd.layout[ax._name];

if(!axIn) ax._gd.layout[ax._name] = axIn = {};

if(axIn !== ax) {
axIn.range = ax.range.slice();
axIn.autorange = ax.autorange;
}
var axIn = ax._input;
axIn.range = ax.range.slice();
axIn.autorange = ax.autorange;
Copy link
Contributor Author

@etpinard etpinard Mar 1, 2017

Choose a reason for hiding this comment

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

Now, doAutorange writes in ax._input instead of having dig in _gd.layout ... 🎉

}
};

Expand Down Expand Up @@ -1137,7 +1133,7 @@ axes.tickText = function(ax, x, hover) {
};

function tickTextObj(ax, x, text) {
var tf = ax.tickfont || ax._gd._fullLayout.font;
var tf = ax.tickfont || {};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh. I need to double-check this.

Removing the || {} should do the trick - as axis tickfont already default to layout font, but removing || {} makes the fonts.json mock fail. Not sure why.

Copy link
Contributor Author

@etpinard etpinard Mar 2, 2017

Choose a reason for hiding this comment

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

After investigation, I'll keep the || {} fallback, if ok.

Whenever ax.showticklabels: false, ax.tickfont isn't coerced and isn't defined here.

We could make sure that tickTextObj isn't called when ax.showticklabels: false, but this requires adding a few early returns in axes.js, 3d and gl2d convert routines which I believe isn't worth it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍


return {
x: x,
Expand Down Expand Up @@ -1347,7 +1343,7 @@ function numFormat(v, ax, fmtoverride, hover) {
if(dp) v = v.substr(0, dp + tickRound).replace(/\.?0+$/, '');
}
// insert appropriate decimal point and thousands separator
v = Lib.numSeparate(v, ax._gd._fullLayout.separators, separatethousands);
v = Lib.numSeparate(v, ax._separators, separatethousands);
}

// add exponent
Expand Down
4 changes: 2 additions & 2 deletions src/plots/cartesian/axis_defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ var autoType = require('./axis_autotype');
* data: the plot data to use in choosing auto type
* bgColor: the plot background color, to calculate default gridline colors
*/
module.exports = function handleAxisDefaults(containerIn, containerOut, coerce, options) {
module.exports = function handleAxisDefaults(containerIn, containerOut, coerce, options, layoutOut) {
var letter = options.letter,
font = options.font || {},
defaultTitle = 'Click to enter ' +
Expand Down Expand Up @@ -78,7 +78,7 @@ module.exports = function handleAxisDefaults(containerIn, containerOut, coerce,
handleCalendarDefaults(containerIn, containerOut, 'calendar', options.calendar);
}

setConvert(containerOut);
setConvert(containerOut, layoutOut);

var dfltColor = coerce('color');
// if axis.color was provided, use it for fonts too; otherwise,
Expand Down
9 changes: 5 additions & 4 deletions src/plots/cartesian/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@
var d3 = require('d3');
var Lib = require('../../lib');
var Plots = require('../plots');
var Axes = require('./axes');

var axisIds = require('./axis_ids');
var constants = require('./constants');

exports.name = 'cartesian';
Expand Down Expand Up @@ -166,7 +167,7 @@ exports.clean = function(newFullData, newFullLayout, oldFullData, oldFullLayout)

if(hadCartesian && !hasCartesian) {
var subplotLayers = oldFullLayout._cartesianlayer.selectAll('.subplot');
var axIds = Axes.listIds({ _fullLayout: oldFullLayout });
var axIds = axisIds.listIds({ _fullLayout: oldFullLayout });

subplotLayers.call(purgeSubplotLayers, oldFullLayout);
oldFullLayout._defs.selectAll('.axesclip').remove();
Expand Down Expand Up @@ -241,13 +242,13 @@ function makeSubplotData(gd) {
// dimension that this one overlays to be an overlaid subplot,
// the main plot must exist make sure we're not trying to
// overlay on an axis that's already overlaying another
var xa2 = Axes.getFromId(gd, xa.overlaying) || xa;
var xa2 = axisIds.getFromId(gd, xa.overlaying) || xa;
if(xa2 !== xa && xa2.overlaying) {
xa2 = xa;
xa.overlaying = false;
}

var ya2 = Axes.getFromId(gd, ya.overlaying) || ya;
var ya2 = axisIds.getFromId(gd, ya.overlaying) || ya;
if(ya2 !== ya && ya2.overlaying) {
ya2 = ya;
ya.overlaying = false;
Expand Down
63 changes: 41 additions & 22 deletions src/plots/cartesian/layout_defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,17 +115,43 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {

var bgColor = Color.combine(plot_bgcolor, layoutOut.paper_bgcolor);

var axLayoutIn, axLayoutOut;
var axName, axLayoutIn, axLayoutOut;

function coerce(attr, dflt) {
return Lib.coerce(axLayoutIn, axLayoutOut, layoutAttributes, attr, dflt);
}

axesList.forEach(function(axName) {
var axLetter = axName.charAt(0);
function getCounterAxes(axLetter) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as promised in #1261 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are still two maps and a filter 🐎

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 5eb6202

var list = {x: yaList, y: xaList}[axLetter];
return Lib.simpleMap(list, axisIds.name2id);
}

function getOverlayableAxes(axLetter, axName) {
var list = {x: xaList, y: yaList}[axLetter];
var out = [];

axLayoutIn = layoutIn[axName] || {};
axLayoutOut = {};
for(var j = 0; j < list.length; j++) {
var axName2 = list[j];

if(axName2 !== axName && !(layoutIn[axName2] || {}).overlaying) {
out.push(axisIds.name2id(axName2));
}
}

return out;
}

for(i = 0; i < axesList.length; i++) {
axName = axesList[i];

if(!Lib.isPlainObject(layoutIn[axName])) {
layoutIn[axName] = {};
}

axLayoutIn = layoutIn[axName];
axLayoutOut = layoutOut[axName] = {};

var axLetter = axName.charAt(0);

var defaultOptions = {
letter: axLetter,
Expand All @@ -142,29 +168,21 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {

var positioningOptions = {
letter: axLetter,
counterAxes: {x: yaList, y: xaList}[axLetter].map(axisIds.name2id),
overlayableAxes: {x: xaList, y: yaList}[axLetter].filter(function(axName2) {
return axName2 !== axName && !(layoutIn[axName2] || {}).overlaying;
}).map(axisIds.name2id)
counterAxes: getCounterAxes(axLetter),
overlayableAxes: getOverlayableAxes(axLetter, axName)
};

handlePositionDefaults(axLayoutIn, axLayoutOut, coerce, positioningOptions);

layoutOut[axName] = axLayoutOut;

// so we don't have to repeat autotype unnecessarily,
// copy an autotype back to layoutIn
if(!layoutIn[axName] && axLayoutIn.type !== '-') {
layoutIn[axName] = {type: axLayoutIn.type};
}

});
axLayoutOut._input = axLayoutIn;
}

// quick second pass for range slider and selector defaults
var rangeSliderDefaults = Registry.getComponentMethod('rangeslider', 'handleDefaults'),
rangeSelectorDefaults = Registry.getComponentMethod('rangeselector', 'handleDefaults');

xaList.forEach(function(axName) {
for(i = 0; i < xaList.length; i++) {
axName = xaList[i];
axLayoutIn = layoutIn[axName];
axLayoutOut = layoutOut[axName];

Expand All @@ -181,9 +199,10 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
}

coerce('fixedrange');
});
}

yaList.forEach(function(axName) {
for(i = 0; i < yaList.length; i++) {
axName = yaList[i];
axLayoutIn = layoutIn[axName];
axLayoutOut = layoutOut[axName];

Expand All @@ -196,5 +215,5 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
);

coerce('fixedrange', fixedRangeDflt);
});
}
};
13 changes: 9 additions & 4 deletions src/plots/cartesian/set_convert.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ function num(v) {
* also clears the autorange bounds ._min and ._max
* and the autotick constraints ._minDtick, ._forceTick0
*/
module.exports = function setConvert(ax) {
module.exports = function setConvert(ax, fullLayout) {
fullLayout = fullLayout || {};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Second, make setConvert take in fullLayout


// clipMult: how many axis lengths past the edge do we render?
// for panning, 1-2 would suffice, but for zooming more is nice.
Expand Down Expand Up @@ -319,7 +320,7 @@ module.exports = function setConvert(ax) {

// set scaling to pixels
ax.setScale = function(usePrivateRange) {
var gs = ax._gd._fullLayout._size,
var gs = fullLayout._size,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

... so that setScale can look into _size.

axLetter = ax._id.charAt(0);

// TODO cleaner way to handle this case
Expand All @@ -328,7 +329,7 @@ module.exports = function setConvert(ax) {
// make sure we have a domain (pull it in from the axis
// this one is overlaying if necessary)
if(ax.overlaying) {
var ax2 = axisIds.getFromId(ax._gd, ax.overlaying);
var ax2 = axisIds.getFromId({ _fullLayout: fullLayout }, ax.overlaying);
ax.domain = ax2.domain;
}

Expand Down Expand Up @@ -360,7 +361,7 @@ module.exports = function setConvert(ax) {
Lib.notifier(
'Something went wrong with axis scaling',
'long');
ax._gd._replotting = false;
fullLayout._replotting = false;
throw new Error('axis scaling');
}
};
Expand Down Expand Up @@ -417,6 +418,10 @@ module.exports = function setConvert(ax) {
ax._min = [];
ax._max = [];

// copy ref to fullLayout.separators so that
// methods in Axes can use it w/o having to pass fullLayout
ax._separators = fullLayout.separators;

// and for bar charts and box plots: reset forced minimum tick spacing
delete ax._minDtick;
delete ax._forceTick0;
Expand Down
5 changes: 2 additions & 3 deletions src/plots/geo/geo.js
Original file line number Diff line number Diff line change
Expand Up @@ -465,10 +465,9 @@ function createMockAxis(fullLayout) {
var mockAxis = {
type: 'linear',
showexponent: 'all',
exponentformat: Axes.layoutAttributes.exponentformat.dflt,
_gd: { _fullLayout: fullLayout }
exponentformat: Axes.layoutAttributes.exponentformat.dflt
};

Axes.setConvert(mockAxis);
Axes.setConvert(mockAxis, fullLayout);
return mockAxis;
}
15 changes: 0 additions & 15 deletions src/plots/gl3d/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,6 @@ var Plots = require('../plots');
var Lib = require('../../lib');
var xmlnsNamespaces = require('../../constants/xmlns_namespaces');

var axesNames = ['xaxis', 'yaxis', 'zaxis'];


exports.name = 'gl3d';

exports.attr = 'scene';
Expand Down Expand Up @@ -45,8 +42,6 @@ exports.plot = function plotGl3d(gd) {
scene = sceneLayout._scene;

if(!scene) {
initAxes(gd, sceneLayout);

scene = new Scene({
id: sceneId,
graphDiv: gd,
Expand Down Expand Up @@ -118,13 +113,3 @@ exports.cleanId = function cleanId(id) {

return 'scene' + sceneNum;
};

exports.setConvert = require('./set_convert');

function initAxes(gd, sceneLayout) {
for(var j = 0; j < 3; ++j) {
var axisName = axesNames[j];

sceneLayout[axisName]._gd = gd;
}
}
14 changes: 9 additions & 5 deletions src/plots/gl3d/scene.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ var showNoWebGlMsg = require('../../lib/show_no_webgl_msg');

var createCamera = require('./camera');
var project = require('./project');
var setConvert = require('./set_convert');
var createAxesOptions = require('./layout/convert');
var createSpikeOptions = require('./layout/spikes');
var computeTickMarks = require('./layout/tick_marks');
Expand Down Expand Up @@ -339,6 +338,7 @@ proto.plot = function(sceneData, fullLayout, layout) {
this.glplot.snapToData = true;

// Update layout
this.fullLayout = fullLayout;
this.fullSceneLayout = fullSceneLayout;

this.glplotLayout = fullSceneLayout;
Expand All @@ -353,10 +353,7 @@ proto.plot = function(sceneData, fullLayout, layout) {
this.glplot.update({});

// Update axes functions BEFORE updating traces
for(i = 0; i < 3; ++i) {
axis = fullSceneLayout[axisProperties[i]];
setConvert(axis);
}
this.setConvert(axis);

// Convert scene data
if(!sceneData) sceneData = [];
Expand Down Expand Up @@ -708,4 +705,11 @@ proto.toImage = function(format) {
return dataURL;
};

proto.setConvert = function() {
for(var i = 0; i < 3; ++i) {
var ax = this.fullSceneLayout[axisProperties[i]];
Axes.setConvert(ax, this.fullLayout);
}
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice simplification! Why don't we need the containerOut.setScale = Lib.noop; line anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not anymore. 🔪

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be more precise, 3d doesn't use any of the _m and friends fields that setScale computes at the moment. So setting setScale to a noop might be a minor 🐎 boost. But, for 3d annotations, I'm planning on using / mocking those scale fields. To be continued.


module.exports = Scene;
Loading