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

fix reset camera buttons to work after switching projection between perspective and orthographic #3597

Merged
merged 8 commits into from Mar 5, 2019

Conversation

Projects
None yet
3 participants
@archmoj
Copy link
Collaborator

archmoj commented Mar 2, 2019

Fixes the issue described in #3596.
codepen Before.
codepen After.

fixes #3596

@archmoj archmoj added the type: bug label Mar 2, 2019

@@ -347,15 +347,19 @@ function handleCamera3d(gd, ev) {
var key = sceneId + '.camera';
var scene = fullLayout[sceneId]._scene;

if(attr === 'resetDefault') {
if(attr === 'resetDefault' || attr === 'resetLastSave') {
aobj[key] = Lib.extendDeep({}, scene.cameraInitial);

This comment has been minimized.

Copy link
@etpinard

etpinard Mar 4, 2019

Member

Wouldn't it be easier to just save keys up, eye and center in scene.cameraInitial instead of saving the all scene.camera keys and then doing this fancy footwork to get the correct projection.type?

In other words, do we really need a cameraInitial stash? Could we instead just use a "view" stash:

scene.viewInitial = {
  up: {x: /**/, y: /**/, z: /**/},
  eye: {x: /**/, y: /**/, z: /**/},
  center: {x: /**/, y: /**/, z: /**/}
}

This comment has been minimized.

Copy link
@archmoj

archmoj Mar 4, 2019

Author Collaborator

Good call.
Done in 9e1aa7a.

if(attr === 'resetDefault' || attr === 'resetLastSave') {
aobj[key] = Lib.extendDeep({}, scene.viewInitial);

aobj[key].projection = {

This comment has been minimized.

Copy link
@etpinard

etpinard Mar 4, 2019

Member

We shouldn't need this after 9e1aa7a

This comment has been minimized.

Copy link
@etpinard

etpinard Mar 4, 2019

Member

Actually, you'll need to tweak aobj / key logic. In the end, aobj should look like:

aobj = {
  'scene.camera.up': {/**/},
  'scene.camera.eye': {/**/},
  'scene.camera.center': {/**/}
}

as opposed to

aobj = {
  'scene.camera': {
    up: {/**/},
    eye: {/**/},
    center: {/**/}
  }
}
'up': scene.viewInitial.up,
'eye': scene.viewInitial.eye,
'center': scene.viewInitial.center,
'projection': {

This comment has been minimized.

Copy link
@etpinard

etpinard Mar 5, 2019

Member

We shouldn't need to update projection at all, am I right?

This comment has been minimized.

Copy link
@archmoj

archmoj Mar 5, 2019

Author Collaborator

You are right. But in order to that we need to explicitly pass the current projection here.

This comment has been minimized.

Copy link
@etpinard

etpinard Mar 5, 2019

Member

Ok, that's a bug then. Calling

Plotly.relayout('graph', 'scene.camera.up', {/**/})

should not change the projection type.

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Mar 5, 2019

Contributor

don't we just need to flatten it to aobj[key + '.up'] = ... etc?

This comment has been minimized.

Copy link
@archmoj

archmoj Mar 5, 2019

Author Collaborator

Thanks @alexcjohnson & @etpinard.
Flattening was really helpful.

@etpinard

This comment has been minimized.

Copy link
Member

etpinard commented Mar 5, 2019

Nicely done 💃

@archmoj

This comment has been minimized.

Copy link
Collaborator Author

archmoj commented Mar 5, 2019

Nicely done
@etpinard If you don't mind, there is one more commit ready to push which help switching the projection to graph default.

@etpinard

This comment has been minimized.

Copy link
Member

etpinard commented Mar 5, 2019

there is one more commit ready to push

ok.

var oldOrtho = scene.camera._ortho;

if(newOrtho !== oldOrtho) {
aobj[key + '.projection'] = scene.viewInitial.projection;

This comment has been minimized.

Copy link
@etpinard

etpinard Mar 5, 2019

Member

Wait. Why do we need this?

@@ -1490,7 +1490,7 @@ describe('Test gl3d relayout calls', function() {
.then(done);
});

it('@gl should maintain projection type when resetCamera buttons clicked after switching projection type from perspective to orthographic', function(done) {
it('@gl resetCamera buttons should be able to reset projection type after switching projection type from perspective to orthographic', function(done) {

This comment has been minimized.

Copy link
@etpinard

etpinard Mar 5, 2019

Member

Why do you change your mind here?

This comment has been minimized.

Copy link
@archmoj

archmoj Mar 5, 2019

Author Collaborator

Now that we could switch projections to initial projection I thought we may change this.
If the user is not happy with the changes made to the camera and wants to get back to initial/home view this allows that. Codepen.

@etpinard

This comment has been minimized.

Copy link
Member

etpinard commented Mar 5, 2019

Now that we could switch projections to initial projection I thought we may change this.
If the user is not happy with the changes made to the camera and wants to get back to initial/home view this allows that.

I disagree. The reset camera buttons should reset only the attribute that can be changed via user interactions i.e. drag/scroll/clicking on modebar buttons. As we can't change the projection.type via user interactions (we need a relayout call here), it shouldn't be part of viewInitial and shouldn't be handled on modebar button clicks.

I'm voting for the version pre 8444b99

@@ -338,6 +338,8 @@ modeBarButtons.resetCameraLastSave3d = {
function handleCamera3d(gd, ev) {
var button = ev.currentTarget;
var attr = button.getAttribute('data-attr');
if(attr !== 'resetLastSave' && attr !== 'resetDefault') return;

This comment has been minimized.

Copy link
@etpinard

etpinard Mar 5, 2019

Member

When does this happen?

@etpinard

This comment has been minimized.

Copy link
Member

etpinard commented Mar 5, 2019

Well earned 💃

Thanks for following through from my comments.

@archmoj archmoj merged commit cff31df into master Mar 5, 2019

7 of 8 checks passed

ci/circleci: test-jasmine CircleCI is running your tests
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: publish Your tests passed on CircleCI!
Details
ci/circleci: test-image Your tests passed on CircleCI!
Details
ci/circleci: test-image2 Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine2 Your tests passed on CircleCI!
Details
ci/circleci: test-syntax Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details

@archmoj archmoj deleted the fix-reset-camera3d-after-switch-projection branch Mar 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.