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 1 commit
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
Prev Previous commit
Next Next commit
forced graphs using deprecated cameraposition attribute to stay upwards
  • Loading branch information
@archmoj
archmoj committed Nov 21, 2018
commit 9ef8a705526d463b71ab8fea8d2dd624b032c9bb
2 changes: 1 addition & 1 deletion src/plot_api/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ exports.cleanLayout = function(layout) {
scene.camera = {
eye: {x: eye[0], y: eye[1], z: eye[2]},
center: {x: center[0], y: center[1], z: center[2]},
up: {x: mat[1], y: mat[5], z: mat[9]}
up: {x: 0, y: 0, z: 1} // we just ignore calculating camera z up in this case
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previous calculations of camera.up for gl3d_surface-lighting & gl3d_ibm-plot didn't set up vector close to [0,0,1].
I thought we may ignore camera z up if cameraposition depreciated setting is used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

... and I presume the gl3d_opacity-surface.png baseline changed because of this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ignore that last comment. I didn't see #3256 (comment)

Thanks!

};

delete scene.cameraposition;
Expand Down
16 changes: 0 additions & 16 deletions test/image/mocks/gl3d_text-weirdness.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,22 +42,6 @@
],
"layout": {
"autosize": true,
"undefined": {
"cameraposition": [
[
0.13855639464070454,
0.5365430934822464,
0.7678929376547012,
-0.32134727420767745
],
[
0,
0,
0
],
1.8771354322421991
]
},
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 thought this might not be necessary. If one wants to enable cameraposition it is required to use "scene" instead of "undefined"! Also now I am curious about this mock & why it was named weirdness? Also if we could possibly remove it or reduce the width/height.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow. Thanks for fixing this. That undefined shouldn't be there.

"title": "Scatter3d with text weirdness",
"showlegend": false,
"height": 758,
Expand Down