Skip to content

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

Merged

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Apr 21, 2016

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.

@monfera
Copy link
Contributor Author

monfera commented Apr 21, 2016

@etpinard @bpostlethwaite I believe these changes address FI-29.

);
];
this.glplot.camera.lookAt.apply(this, lookAtInput);
this.graphDiv.emit('plotly_relayout', lookAtInput);
Copy link
Contributor

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 }
  }
});

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

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@etpinard etpinard Apr 21, 2016

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.

@etpinard
Copy link
Contributor

Ok. I'm ok with this.

We should spend some time rethinking how scene.camera interacts with Plotly.relayout so that e.g. the mode bar buttons could simply call Plotly.relayout with the correct update object without having to use an internal Scene method.

That will be for another day (possibly before v2.0.0) though.

@etpinard etpinard added bug something broken status: reviewable labels Apr 21, 2016
expect(relayoutCallback).toHaveBeenCalled(); // initiator: resetCameraLastSave3d
expect(relayoutCallback).toHaveBeenCalledWith({
scene: {
eye: { x: 1.25, y: 1.25, z: 1.25 },
Copy link
Contributor

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.

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 @etpinard, I'll try to capture the multi-scene aspect.

up: { x: 0, y: 0, z: 1 }
}
});
relayoutCallback.calls.reset();
Copy link
Contributor

Choose a reason for hiding this comment

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

nice move.

@etpinard
Copy link
Contributor

@monfera Great way to get introduced with our gl3d code. Nicely done 🍻

@etpinard etpinard merged commit 0f9ea7f into plotly:master Apr 21, 2016
Narghast pushed a commit to GeorgeGhiottone/plotly.js that referenced this pull request May 20, 2016
Narghast pushed a commit to GeorgeGhiottone/plotly.js that referenced this pull request May 20, 2016
Narghast pushed a commit to GeorgeGhiottone/plotly.js that referenced this pull request May 23, 2016
Narghast pushed a commit to GeorgeGhiottone/plotly.js that referenced this pull request May 26, 2016
Narghast pushed a commit to GeorgeGhiottone/plotly.js that referenced this pull request May 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants