-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Bug fix issue 3224 - avoid NaN values for axis dticks #3233
Conversation
src/plots/cartesian/axes.js
Outdated
@@ -755,6 +755,9 @@ axes.autoTicks = function(ax, roughDTick) { | |||
// prevent infinite loops | |||
if(ax.dtick === 0) ax.dtick = 1; | |||
|
|||
// prevent issue https://github.com/plotly/plotly.js/issues/3224 | |||
if(Number.isNaN(ax.dtick)) ax.dtick = 1; |
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.
Do we know under which circumstances ax.dtick
comes through as NaN
here?
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.
It could simply be triggered for example using gl3d_ibm-plot by panning the graph to outside.
I am wondering now that we may need to replace the condition checks in few other places there too? Noting that isNumeric(NaN)
returns false; it skips the adjustment blocks starting with if(!isNumeric(...
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.
It could simply be triggered for example using gl3d_ibm-plot by panning the graph to outside.
Hmm. I can't replicate. Can you take a screenshot of
gd._fullLayout.scene.camera.eye
in the console once after that error appears.
I am wondering now that we may need to replace the condition checks in few other places there too?
I'm actually thinking the opposite: we shouldn't be calling Axes.autoTicks
in the first place whenever ditck===NaN
. But first, I'll like to know how this happens.
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.
I'm actually thinking the opposite: we shouldn't be calling
Axes.autoTicks
in the first place wheneverditck===NaN
. But first, I'll like to know how this happens.
Agreed. For one thing, dtick=1
would draw 1000 ticks on date or otherwise large-range axes which would bog things down and look ugly. What's roughDTick
when we get NaN
here, and why?
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.
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.
And this is another example example now without date and logs.
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.
Right, this is consistent with what I was thinking: we get those Uncaught ax.dtick error: NaN
errors in the console in situations where ticks can't be seen.
Somewhere at the end of this
|
Great fix 💃 |
src/plots/gl3d/layout/tick_marks.js
Outdated
@@ -47,7 +47,9 @@ function computeTickMarks(scene) { | |||
axes._length = (glRange[i].hi - glRange[i].lo) * | |||
glRange[i].pixelsPerDataUnit / scene.dataScale[i]; | |||
|
|||
if(Math.abs(axes._length) === Infinity) { | |||
if(Math.abs(axes._length) === Infinity || | |||
Math.abs(axes._length) === -Infinity || |
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.
wait, how can Math.abs(...) === -Infinity
?
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.
Thanks for the note. Just removed that line.
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.
Thanks. Anyway much better to fix the issue here than in axes.js
!
💃
Fixes #3224 by checking
ax._length
values not to be NaN@etpinard