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

Bug fix issue 3224 - avoid NaN values for axis dticks #3233

Merged
merged 7 commits into from
Nov 9, 2018
Merged

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Nov 8, 2018

Fixes #3224 by checking ax._length values not to be NaN
@etpinard

@archmoj archmoj changed the title Issue 3224 Bug fix issue 3224 - check for NaN values for axis dticks Nov 8, 2018
@@ -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;
Copy link
Contributor

@etpinard etpinard Nov 8, 2018

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?

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Collaborator

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 whenever ditck===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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etpinard here is a demo

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@etpinard
Copy link
Contributor

etpinard commented Nov 8, 2018

@etpinard Could you please let me know where to put a jasmine test for this bug?

Somewhere at the end of this describe block

describe('Test gl3d plots', function() {

@archmoj archmoj changed the title Bug fix issue 3224 - check for NaN values for axis dticks Bug fix issue 3224 - avoid NaN values for axis dticks Nov 8, 2018
@etpinard
Copy link
Contributor

etpinard commented Nov 9, 2018

Great fix 💃

@@ -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 ||
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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!

💃

@archmoj archmoj merged commit 889087d into master Nov 9, 2018
@archmoj archmoj deleted the issue-3224 branch November 9, 2018 03:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants