-
-
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
Preserve gl3d scene aspectratio after orthographic scroll zoom #4578
Conversation
… for restyle interactions
src/plots/gl3d/scene.js
Outdated
aspectRatio = [1, 1, 1]; | ||
} | ||
} else if(fullSceneLayout.aspectmode === 'cube') { | ||
var aspectmode = scene._aspectmode || fullSceneLayout.aspectmode; |
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.
What's the difference between scene._aspectmode
and fullSceneLayout.aspectmode
?
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.
scene._aspectmode
is an internal variable which is set after aspectratio
values are computed.
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.
Are there situations where scene._aspectmode
and fullSceneLayout.aspectmode
aren't the same?
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 is. Namely after using scroll zoom.
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.
Ok, but shouldn't the changes to aspectmode
during scroll zoom propagate to fullLayout
?
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 initially tried to achieve that on another branch using relayout
updates.
Since tests started failing, I thought that may not be ideal to override fullLayout
in this case.
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 curious. Which tests fail?
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.
Are there situations where
scene._aspectmode
andfullSceneLayout.aspectmode
aren't the same?
OK. This time I was able to make that work without a temporary variable: 1828797
Brilliant. 💃 ! |
Fixes #4514 restyle part
After refactoring parts of the gl3d/scene code related to computing
aspectratio
,this PR fixes the bug in commit e29aceb.
Demo | Please change scales using scroll, then select a point. It shouldn't display initial scales.
@plotly/plotly_js