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

archmoj · 2018-11-17T05:25:19Z

Fixes #2425 by seting dragmode: 'orbit' at init time to allow tilted camera z vectors and then possibly switch to turntable mode as used to be desired default user mode.

Background: in plotly.js dragmode: 'turntable' was applied as the default camera behaviour which is desirable for most cases and it modifies & locks camera z-axis rotation. However in certain cases when the user had specified camera z-axis this value was ignored at the start.
The first approach applied here was to try to turn on dragmode: 'orbit' in those edge cases. The prototypes was working interactively but that introduced new side-effects and required complicated changes.
On the other hand the second approach applied is to simply set orbit as the default and then switch to turntable mode in all cases where the camera.z.up vector is different from being upwards i.e. [0,0,1].

@etpinard
@alexcjohnson

alexcjohnson · 2018-11-17T06:17:28Z

src/plots/gl3d/index.js

@@ -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';


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?)

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?

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.

alexcjohnson · 2018-11-17T06:28:44Z

@archmoj I like this solution (subject to the comment above).

BTW, I notice a related issue: if you've set the plot in orbit mode, rotated to some different up vector, then you switch back to turntable mode; the plot rotates back to a compatible orientation, but layout.scene.camera.up, and the copy in _fullLayout, still have the up and eye they had at the end of the orbit interaction. But I think either I should take that in #3236 or we should address this after that merges, as it should use _guiRelayout or _storeDirectGuiEdits...

alexcjohnson · 2018-11-17T06:43:54Z

test/image/mocks/gl3d_snowden_altered.json

-                "x": 1,
-                "y": 0,
+                "x": 0,
+                "y": 1,


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.

etpinard · 2018-11-19T15:00:20Z

src/plots/gl3d/layout/layout_attributes.js

@@ -140,7 +140,7 @@ module.exports = {
        valType: 'enumerated',
        role: 'info',
        values: ['orbit', 'turntable', 'zoom', 'pan', false],
-        dflt: 'turntable',
+        dflt: 'orbit',


Hmm. I would prefer removing the dflt key entirely - as the default value is now dynamic.

Could you also add some info in the description about the new orbit-vs-turntable logic?

I updated the description. Thanks @etpinard for the note. I will give removing dflt a try.

Once I tried to remove dragmode.dflt, the default behaviour for drag using right-click (orbit/turntable) is not activated.
So we may keep it there.

You need to pass the dynamic default to dragmode here:

plotly.js/src/plots/gl3d/layout/defaults.js

Line 141 in c9eed54

coerce('dragmode', opts.getDfltFromLayout('dragmode'));

getDfltFromLayout is part of it but then there's the dynamic default based on camera. I don't think you want to be altering sceneIn.dragmode with enableTurnTable, you just want to be setting the dynamic default. The logic is something like:

If getDfltFromLayout returns a value, use that.

If not but the user provides a camera that's not {x: 0, y: 0, z: >0} dfltDragmode='orbit'

If neither of those, dfltDragmode = 'turntable'

Then you call coerce('dragmode', dfltDragmode) so the user can still explicitly provide whatever.

The only case that's still weird at that point is if the user explicitly says dragmode: 'turntable', camera: {up: {not z-up}}. My inclination at that point is to set sceneOut.camera.up (and NOT sceneIn.camera.up) to {x: 0, y: 0, z: 1}. That won't change the behavior - the user might still wonder why up is not being honored - but at least it would raise an alarm if you call Plotly.validate.

Thanks @alexcjohnson, I will modify it to be this way.

etpinard · 2018-11-19T15:01:43Z

test/jasmine/tests/gl3d_plot_interact_test.js

@@ -394,7 +394,7 @@ describe('Test gl3d plots', function() {

    it('@gl should be able to reversibly change trace type', function(done) {
        var _mock = Lib.extendDeep({}, mock2);
-        var sceneLayout = { aspectratio: { x: 1, y: 1, z: 1 } };
+        var sceneLayout = { aspectratio: { x: 1, y: 1, z: 1 }, dragmode: 'turntable' };


Why did you need to change this? I'm fearing a breaking change here.

In order to set turnable the default behaviour the function needs to add the dragmode attribute to the sceneLayout.

So without setting dragmode: 'turntable' here, it would default to 'orbit'?

What are the camera settings in this case?

@etpinard notice this isn't a setting, it's an expectation. But I agree, we shouldn't be mutating the input scene, per my comment above

etpinard · 2018-11-21T17:03:40Z

src/plots/gl3d/layout/defaults.js

+
+                if(!x || !y || !z) {
+                    dragmode = 'turntable';
+                } else if(z / Math.sqrt(x * x + y * y + z * z) > 0.999) {


This PR is getting better by the minute. Thanks for your efforts @archmoj

Now, would there be a way to tweak this condition such that gl3d_ibm-plot and gl3d_surface-lighting still default to dragmode: 'turntable'?

Good point @etpinard. Let me check.

archmoj · 2018-11-21T20:21:06Z

src/plot_api/helpers.js

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


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.

Nice solution.

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

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

Thanks!

archmoj · 2018-11-21T20:26:10Z

Also concerning the changes of 'gl3d_opacity-surfacebaseline (which is now the only baseline change in this PR): Becausedragmode: 'zoom'is applied with the latest fix it is oriented exactly upwards while thezoom` button is still enabled by default.

archmoj · 2018-11-21T20:31:11Z

test/image/mocks/gl3d_text-weirdness.json

-        ],
-        1.8771354322421991
-      ]
-    },


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.

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

etpinard · 2018-11-22T17:22:56Z

Very nice fix. 💃

interaction between previous changes in this PR and #3256

archmoj added 3 commits November 16, 2018 23:09

fix for issue 2425

f82007b

revised camera positions in 3 mocks

d8ef9eb

updated mocks and baseline images

556268b

archmoj added type: bug status: reviewable labels Nov 17, 2018

alexcjohnson reviewed Nov 17, 2018

View reviewed changes

moved overwrites to layout defaults

eb72856

archmoj added status: has TODOs and removed status: reviewable labels Nov 17, 2018

archmoj added 13 commits November 17, 2018 17:32

fix syntax

789cfd9

revised snowden mock and baseline

4014c35

revert snowden mock

1ae12b3

revert gl3d_surface-lighting mock to better display the old bug

cb6dab7

improved logic

67ea429

minimal fix

5c3742c

changed defaults and baseline

3ab1b1d

logic to enable turntable default

5b24f51

revert gl3d layout test

d338058

revert baselines

b8da9ba

changes to gl3d layout test

899b568

fixes for jasmine test

724416b

fix for restyle calls

c9eed54

archmoj added status: reviewable and removed status: has TODOs labels Nov 19, 2018

etpinard reviewed Nov 19, 2018

View reviewed changes

archmoj added 2 commits November 20, 2018 16:23

adding jasmine tests for camera init

b65fa61

added camera tests

d25c335

archmoj added 2 commits November 20, 2018 17:15

revised auto logic i.e. without touching layout inputs

f8bf472

untouch gl3d layout test

6af04f2

etpinard reviewed Nov 21, 2018

View reviewed changes

archmoj added 2 commits November 21, 2018 14:39

forced graphs using deprecated cameraposition attribute to stay upwards

9ef8a70

reverted and changed baseline that uses cameraposition

f43f944

archmoj commented Nov 21, 2018

View reviewed changes

archmoj merged commit 2f6c6e7 into master Nov 22, 2018

archmoj deleted the issue-2425 branch November 22, 2018 17:25

alexcjohnson added a commit that referenced this pull request Nov 29, 2018

write camera.up back to layout only when necessary

8d11985

interaction between previous changes in this PR and #3256

alexcjohnson mentioned this pull request Nov 30, 2018

Preserve ui state across Plotly.react redraws #3236

Merged

etpinard mentioned this pull request Jan 10, 2019

Saving 3D figures to PNG results in a different rotation #3425

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

archmoj commented Nov 17, 2018 •

edited

Loading

alexcjohnson Nov 17, 2018

archmoj Nov 18, 2018

alexcjohnson Nov 18, 2018

alexcjohnson commented Nov 17, 2018

alexcjohnson Nov 17, 2018

etpinard Nov 19, 2018

archmoj Nov 19, 2018

archmoj Nov 20, 2018

alexcjohnson Nov 20, 2018

archmoj Nov 20, 2018

etpinard Nov 19, 2018

archmoj Nov 19, 2018

etpinard Nov 20, 2018

alexcjohnson Nov 20, 2018

etpinard Nov 21, 2018

archmoj Nov 21, 2018

archmoj Nov 21, 2018

etpinard Nov 22, 2018

etpinard Nov 22, 2018

etpinard Nov 22, 2018

archmoj commented Nov 21, 2018

archmoj Nov 21, 2018

etpinard Nov 22, 2018

etpinard commented Nov 22, 2018

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

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

Conversation

archmoj commented Nov 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented Nov 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archmoj commented Nov 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Nov 22, 2018

archmoj commented Nov 17, 2018 •

edited

Loading