Improve camera init #3585

archmoj · 2019-02-27T03:00:02Z

This PR would improve camera initialisation setup for gl3d plots.
First - it fixes bug #3576 by disabling undesirable rotation/pan when the click is done on the initial mouse location.

Second - it removes one extra expensive call to create camera object. This is done by improving gl-plot3d interface to also accept & reuse camera object created inside plotly.js scene. Also gl-plot3d's createCamera and createScene are revised and their mouse listeners are packed to be enabled by centralised functions.

Finally - all the previous gl3d jasmine tests with noCI flag could be run on CI.

@plotly/plotly_js

antoinerg · 2019-02-27T14:08:12Z

@archmoj I checked out your branch locally and I can't rotate any of the gl3d mocks!

This mock does not rotate: http://localhost:3000/devtools/test_dashboard/#gl3d_surface_after_heatmap
whereas it used to: https://rreusser.github.io/plotly-mock-viewer/#gl3d_surface_after_heatmap

Maybe there is something wrong on my side... Please let me know if rotation does work for the mock above. Also, if it doesn't work, we really should have a test to catch this 😮 !

archmoj · 2019-02-27T14:19:59Z

@antoinerg thanks for testing this. Interestingly it used to work without that so I thought it could be enabled by the gl-plot3d module. With that we may possibly enable it once the scene is complete.

antoinerg · 2019-02-27T14:36:09Z

@archmoj so you do confirm that you also can't rotate the gl3d traces? If that is the case, I am really worried about the fact that no tests failed. Why is that?

archmoj · 2019-02-27T14:38:05Z

@archmoj so you do confirm that you also can't rotate the gl3d traces?

@antoinerg Yes, I do confirm that.

… test - also avoid return in updateFx to ensure dragmode and hovermode stay on the same ground

… the identical camera object in creating gl3d scene

…k jasmine test

archmoj · 2019-02-28T05:18:03Z

package.json

@@ -80,7 +80,7 @@
    "gl-mat4": "^1.2.0",
    "gl-mesh3d": "^2.0.8",
    "gl-plot2d": "^1.4.2",
-    "gl-plot3d": "^2.1.1",
+    "gl-plot3d": "git://github.com/gl-vis/gl-plot3d.git#5f28e06ac02b248e6990ef385d2df49bb3a7baf8",


https://github.com/gl-vis/gl-plot3d/compare/accept-camera-object

archmoj · 2019-02-28T06:00:17Z

@archmoj so you do confirm that you also can't rotate the gl3d traces? If that is the case, I am really worried about the fact that no tests failed. Why is that?

I added this test in respect to that.

plotly.js/test/jasmine/tests/gl3d_plot_interact_test.js

Lines 131 to 145 in a200e78

    
           .then(_clickOtherplace) 
        
           .then(delay(20)) 
        
           .then(function() { 
        
               var cameraFinal = gd._fullLayout.scene._scene.glplot.camera; 
        
               expect(cameraFinal.up[0]).toBeCloseTo(0, 2, 'cameraFinal.up[0]'); 
        
               expect(cameraFinal.up[1]).toBeCloseTo(0, 2, 'cameraFinal.up[1]'); 
        
               expect(cameraFinal.up[2]).toBeCloseTo(1, 2, 'cameraFinal.up[2]'); 
        
               expect(cameraFinal.center[0]).toBeCloseTo(0, 2, 'cameraFinal.center[0]'); 
        
               expect(cameraFinal.center[1]).toBeCloseTo(0, 2, 'cameraFinal.center[1]'); 
        
               expect(cameraFinal.center[2]).toBeCloseTo(0, 2, 'cameraFinal.center[2]'); 
        
               expect(cameraFinal.eye[0]).not.toBeCloseTo(1.2, 2, 'cameraFinal.eye[0]'); 
        
               expect(cameraFinal.eye[1]).not.toBeCloseTo(1.2, 2, 'cameraFinal.eye[1]'); 
        
               expect(cameraFinal.eye[2]).not.toBeCloseTo(1.2, 2, 'cameraFinal.eye[2]'); 
        
               expect(cameraFinal.mouseListener.enabled === true); 
        
           })

test/jasmine/tests/gl3d_plot_interact_test.js

antoinerg · 2019-03-04T17:26:59Z

I could reproduce the bug and confirm that this PR fixes it. Here's the Codepen with the fix: https://codepen.io/antoinerg/pen/rRMOox?editors=1010

@etpinard, I think it's ready to go but maybe we should wait before merging to master?

etpinard · 2019-03-04T22:09:05Z

This looks good! I'm curious: how expansive was that extra call to create camera object?

I'm a little scared of releasing this in a patch release. Maybe we should wait for 1.46.0 🤔

@archmoj have you manually tested zoom/pan/rotate on multiple gl3d mocks?

archmoj · 2019-03-04T23:07:14Z

This looks good! I'm curious: how expansive was that extra call to create camera object?

Console.time may not not show much difference in terms of milliseconds. But it was complicated to modify camera object or link other events after it is was created in plotly.js, mainly because it was recreated and re-linked by gl-plot3d.

I'm a little scared of releasing this in a patch release. Maybe we should wait for 1.46.0

Yes I totally agree.

@archmoj have you manually tested zoom/pan/rotate on multiple gl3d mocks?

Yes and they seem to be working.

etpinard · 2019-03-04T23:08:43Z

Yes I totally agree.

Great. Let's put this under 1.46.0

archmoj · 2019-03-20T13:45:58Z

@etpinard This PR may be a good candidate to merge now that #3601 merged. What do you think?

etpinard · 2019-03-20T13:50:38Z

This PR may be a good candidate to merge now that #3601 merged. What do you think?

It is! I'm testing it one last time now. 👌

etpinard · 2019-03-20T13:57:59Z

test/jasmine/tests/isosurface_test.js

@@ -313,7 +313,7 @@ describe('Test isosurface', function() {
        });
    });

-    describe('@noCI hover', function() {
+    describe('hover', function() {


Not anything new, but I noticed this test

plotly.js/test/jasmine/tests/isosurface_test.js

Line 272 in e938c40

it('should clear *cauto* when restyle *cmin* and/or *cmax*', function(done) {

is missing a @gl tag.

etpinard · 2019-03-20T13:59:37Z

💃 once you fix https://github.com/plotly/plotly.js/pull/3585/files/e938c407c5889b60d04ee9cbdc3835bfba111a02#diff-72ac990ccb2a9251e4dcf5e7090af0c4 and bump gl-plot3d

etpinard · 2019-03-20T15:42:57Z

💃 💃 let's get this in master 🍾

fix issue 3576 disable camera mouse listener before scene is complete

54b1ca1

archmoj added bug something broken status: reviewable labels Feb 27, 2019

archmoj added status: in progress and removed status: reviewable labels Feb 27, 2019

archmoj added 7 commits February 27, 2019 10:16

enable mouse listener when the scene is complete and improved jasmine…

bb252af

… test - also avoid return in updateFx to ensure dragmode and hovermode stay on the same ground

improved the test

5b8406a

added a getter and setter

c759396

fixed camera init to avoid multiple calls to create camera and resuse…

66da760

… the identical camera object in creating gl3d scene

fix gl-plot3d link now use ortho attribute when passing camera object in

e48c820

improved gl-plot3d scene and camera modules to fixed issue 3576 - loc…

6bd58c9

…k jasmine test

added noCI flag for the jasmine test

a200e78

archmoj commented Feb 28, 2019

View reviewed changes

archmoj changed the title ~~Bug fix: disable camera mouse listener before scene is complete~~ Improve camera init Feb 28, 2019

archmoj added status: reviewable and removed status: in progress labels Feb 28, 2019

antoinerg reviewed Mar 1, 2019

View reviewed changes

test/jasmine/tests/gl3d_plot_interact_test.js Show resolved Hide resolved

etpinard added this to the v1.46.0 milestone Mar 4, 2019

archmoj added 2 commits March 5, 2019 12:14

Merge branch 'master' into fix3576-no-rotate-camera-before-modebar

09a40af

Merge branch 'master' into fix3576-no-rotate-camera-before-modebar

e57631f

archmoj and others added 5 commits March 7, 2019 15:45

was able to remove noci flag from some of gl3d jasmine tests

4b035d6

gl3d jasmine tests run on CI

e3ec0e4

fixed syntax

98b28ac

resolved conflicts in gl-plot3d

25b78ed

Merge branch 'master' into fix3576-no-rotate-camera-before-modebar

e938c40

etpinard reviewed Mar 20, 2019

View reviewed changes

add the missing gl tag on one test and bump gl-plot3d 2.2.0

6f0330b

archmoj merged commit 12ce725 into master Mar 20, 2019

archmoj deleted the fix3576-no-rotate-camera-before-modebar branch March 20, 2019 15:44

etpinard mentioned this pull request Mar 20, 2019

Undesirable camera reposition just after plot and first left/middle/right click #3576

Closed

archmoj mentioned this pull request Mar 22, 2019

Clean CI run of gl hovers #3661

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve camera init #3585

Improve camera init #3585

archmoj commented Feb 27, 2019 •

edited

Loading

antoinerg commented Feb 27, 2019 •

edited

Loading

archmoj commented Feb 27, 2019

antoinerg commented Feb 27, 2019

archmoj commented Feb 27, 2019

archmoj Feb 28, 2019

archmoj commented Feb 28, 2019

antoinerg commented Mar 4, 2019

etpinard commented Mar 4, 2019

archmoj commented Mar 4, 2019 •

edited

Loading

etpinard commented Mar 4, 2019

archmoj commented Mar 20, 2019

etpinard commented Mar 20, 2019

etpinard Mar 20, 2019

etpinard commented Mar 20, 2019 •

edited

Loading

etpinard commented Mar 20, 2019

Improve camera init #3585

Improve camera init #3585

Conversation

archmoj commented Feb 27, 2019 • edited Loading

antoinerg commented Feb 27, 2019 • edited Loading

archmoj commented Feb 27, 2019

antoinerg commented Feb 27, 2019

archmoj commented Feb 27, 2019

archmoj Feb 28, 2019

Choose a reason for hiding this comment

archmoj commented Feb 28, 2019

antoinerg commented Mar 4, 2019

etpinard commented Mar 4, 2019

archmoj commented Mar 4, 2019 • edited Loading

etpinard commented Mar 4, 2019

archmoj commented Mar 20, 2019

etpinard commented Mar 20, 2019

etpinard Mar 20, 2019

Choose a reason for hiding this comment

etpinard commented Mar 20, 2019 • edited Loading

etpinard commented Mar 20, 2019

archmoj commented Feb 27, 2019 •

edited

Loading

antoinerg commented Feb 27, 2019 •

edited

Loading

archmoj commented Mar 4, 2019 •

edited

Loading

etpinard commented Mar 20, 2019 •

edited

Loading