Skip to content

Bug fix: applying camera up.z vector at scene init #3256

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.

Sign up for GitHub

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
merged 23 commits into from
Nov 22, 2018
Merged

Bug fix: applying camera up.z vector at scene init #3256

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
f82007b
fix for issue 2425
archmoj Nov 17, 2018
d8ef9eb
revised camera positions in 3 mocks
archmoj Nov 17, 2018
556268b
updated mocks and baseline images
archmoj Nov 17, 2018
eb72856
moved overwrites to layout defaults
archmoj Nov 17, 2018
789cfd9
fix syntax
archmoj Nov 17, 2018
4014c35
revised snowden mock and baseline
archmoj Nov 17, 2018
1ae12b3
revert snowden mock
archmoj Nov 17, 2018
cb6dab7
revert gl3d_surface-lighting mock to better display the old bug
archmoj Nov 18, 2018
67ea429
improved logic
archmoj Nov 18, 2018
5c3742c
minimal fix
archmoj Nov 18, 2018
3ab1b1d
changed defaults and baseline
archmoj Nov 18, 2018
5b24f51
logic to enable turntable default
archmoj Nov 19, 2018
d338058
revert gl3d layout test
archmoj Nov 19, 2018
b8da9ba
revert baselines
archmoj Nov 19, 2018
899b568
changes to gl3d layout test
archmoj Nov 19, 2018
724416b
fixes for jasmine test
archmoj Nov 19, 2018
c9eed54
fix for restyle calls
archmoj Nov 19, 2018
b65fa61
adding jasmine tests for camera init
archmoj Nov 20, 2018
d25c335
added camera tests
archmoj Nov 20, 2018
f8bf472
revised auto logic i.e. without touching layout inputs
archmoj Nov 20, 2018
6af04f2
untouch gl3d layout test
archmoj Nov 20, 2018
9ef8a70
forced graphs using deprecated cameraposition attribute to stay upwards
archmoj Nov 21, 2018
f43f944
reverted and changed baseline that uses cameraposition
archmoj Nov 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/plots/gl3d/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,14 @@ exports.plot = function plotGl3d(gd) {
scene = sceneLayout._scene;

if(!scene) {
if(fullLayout[sceneId].camera.up.z !== 1 &&
fullLayout[sceneId].dragmode === 'turntable') {
fullLayout[sceneId].dragmode = 'orbit';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be better to do this in supplyDefaults

Not sure where we ensure ||up||===1 but it doesn't look like that's done in supplyDefaults, so I guess at that point we might need a more complicated test (up.x || up.y || up.z < 0 perhaps?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea of having more complicated test as you mentioned. On the other hand I was wondering that it is now rather difficult for the users to compute and pass z vector components. Could we add a function to normalize user inputs for this vector before reaching there too?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Only the direction of up matters, right? So feels to me like we shouldn't require it to be normalized at all. If the user passes in (0,0,0) we should revert to (0,0,1), otherwise seems to me we should normalize it for our own use but leave the un-normalized value in layout and fullLayout.


sceneLayout = fullLayout[sceneId];
// we may also call updateActiveButton to display the button change
}

scene = new Scene({
id: sceneId,
graphDiv: gd,
Expand Down
Binary file modified test/image/baselines/gl3d_ibm-plot.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/gl3d_surface-lighting.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 2 additions & 2 deletions test/image/mocks/gl3d_snowden_altered.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@
"z": 2
},
"up": {
"x": 1,
"y": 0,
"x": 0,
"y": 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

So... gl3d_ibm_plot has the old (verrrry old) camera format, and converts to the new format with a tilted up, and you allowed it to shift into orbit mode; that's great. And gl3d_surface_lighting you converted to the new camera format and turntable-compatible up, so the image changed because it's somewhat different from the output of the previous old->new conversion; also great.

But what's going on with gl3d_snowden_altered? The image didn't change, but the change you made to up is not to {x: 0, y: 0, z: 1} so I'd have expected the image to change, since now it will render in orbit mode.

"z": 0
}
}
Expand Down
31 changes: 17 additions & 14 deletions test/image/mocks/gl3d_surface-lighting.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,20 +537,23 @@
},
"showlegend": false,
"scene": {
"cameraposition": [
[
-0.4605922047730424,
0.09969805401182547,
0.46756921524156575,
-0.7478597113676797
],
[
0,
0,
0
],
2.165063509461097
],
"camera": {
"eye": {
"x": -1,
"y": 2,
"z": 1
},
"center": {
"x": 0,
"y": 0,
"z": 0
},
"up": {
"x": 0,
"y": 0,
"z": 1
}
},
"xaxis": {
"showbackground": true,
"type": "linear",
Expand Down