-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Trigger plotly_relayout event when camera is reset to default or saved (Solves #FI29) #458
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
Trigger plotly_relayout event when camera is reset to default or saved (Solves #FI29) #458
Conversation
…default or saved
…ss fine locally with fdescribe)
@etpinard @bpostlethwaite I believe these changes address FI-29. |
); | ||
]; | ||
this.glplot.camera.lookAt.apply(this, lookAtInput); | ||
this.graphDiv.emit('plotly_relayout', lookAtInput); |
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.
We could do even better passing the corresponding camera attribute update object e.g:
this.graphDiv.emit('plotly_relayout', {
scene: {
eye: { x: 1.25, y: 1.25, z: 1.25 },
center: { x: 0, y: 0, z: 0 },
up: { x: 0, y: 0, z: 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.
@etpinard I'm glad to refactor for it; I'll extract the object-to-array logic into a separate function so we can be DRY among these two emit
places.
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.
Oh. To handle the mulit-scene case ⏫ should in fact be:
var update = {};
update[this.id] = {
eye: { x: 1.25, y: 1.25, z: 1.25 },
center: { x: 0, y: 0, z: 0 },
up: { x: 0, y: 0, z: 1 }
};
this.graphDiv.emit('plotly_relayout', update);
where this.id
can be scene
, scene2
, scene3
, etc.
Ok. I'm ok with this. We should spend some time rethinking how That will be for another day (possibly before |
expect(relayoutCallback).toHaveBeenCalled(); // initiator: resetCameraLastSave3d | ||
expect(relayoutCallback).toHaveBeenCalledWith({ | ||
scene: { | ||
eye: { x: 1.25, y: 1.25, z: 1.25 }, |
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.
This assertion is misleading. Adding
console.log(getOrbitCamera(cameraData))
above this line in Scene.prototype.setCamera
logs
[[0.1, 0.1, 1], [0, 0, 0], [0, 0, 1]]
for the first scene, and
[[2.5, 2.5, 2.5], [0, 0, 0], [0, 0, 1]]
for the second scene.
I think the jasmine spy, holds on to the arguments of the first relayoutCallback
call and isn't updated for subsequent calls.
Moreover, we should make it clear that plotly_relayout
is triggered twice (one per each scene) when clicking on these buttons.
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 @etpinard, I'll try to capture the multi-scene aspect.
…k argument values
up: { x: 0, y: 0, z: 1 } | ||
} | ||
}); | ||
relayoutCallback.calls.reset(); |
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.
nice move.
@monfera Great way to get introduced with our gl3d code. Nicely done 🍻 |
Fixes issue #FI29. The added test cases aren't part of CircleCI as the test file is excluded, but they pass when run locally with fdescribe.