-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Axes ref cleanup #1431
Changes from all commits
1700908
11073f2
2289657
6a271ed
704596a
045da99
50d1288
5eb6202
6e25640
9612549
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 |
---|---|---|
|
@@ -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; | ||
} | ||
}; | ||
|
||
|
@@ -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 || {}; | ||
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. Oh. I need to double-check this. Removing the 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. After investigation, I'll keep the Whenever We could make sure that 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. 👍 |
||
|
||
return { | ||
x: x, | ||
|
@@ -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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
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. as promised in #1261 (comment) 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. 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, | ||
|
@@ -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]; | ||
|
||
|
@@ -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]; | ||
|
||
|
@@ -196,5 +215,5 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) { | |
); | ||
|
||
coerce('fixedrange', fixedRangeDflt); | ||
}); | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 || {}; | ||
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. Second, make |
||
|
||
// clipMult: how many axis lengths past the edge do we render? | ||
// for panning, 1-2 would suffice, but for zooming more is nice. | ||
|
@@ -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, | ||
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. ... so that |
||
axLetter = ax._id.charAt(0); | ||
|
||
// TODO cleaner way to handle this case | ||
|
@@ -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; | ||
} | ||
|
||
|
@@ -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'); | ||
} | ||
}; | ||
|
@@ -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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'); | ||
|
@@ -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; | ||
|
@@ -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 = []; | ||
|
@@ -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); | ||
} | ||
}; | ||
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. Nice simplification! Why don't we need the 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. Not anymore. 🔪 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. 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; |
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.
Now,
doAutorange
writes inax._input
instead of having dig in_gd.layout ...
🎉