Keep orthographic zoom scales in fullLayout #4292

archmoj · 2019-10-21T18:33:52Z

Fix #4274.
Orthographic scroll zoom changes aspectratio values. These changes should be reflected in fullLayout and also relayout should be emitted.

Before
After

In addition, there was a mistake noticed in calling arguments i.e. when try recovering the context which was fixed in d486dcd.

Also to mention commit 496059c simplifies calling arguments by applying scene pixelRatio.

@plotly/plotly_js

package.json

etpinard · 2019-10-21T21:07:27Z

src/plots/gl3d/scene.js

@@ -243,7 +244,26 @@ function tryCreatePlot(scene, cameraObject, pixelRatio, canvas, gl) {
        }
    }

-    return failed < 2;
+    if(failed < 2) {
+        scene.wheelListener = mouseWheel(scene.glplot.canvas, function(dx, dy) {


A couple questions:

why can't we add logic to this block:

plotly.js/src/plots/gl3d/scene.js

Lines 296 to 300 in 4a649f0

scene.glplot.canvas.addEventListener('wheel', function() {

if(gd._context._scrollZoom.gl3d) {

relayoutCallback(scene);

}

}, passiveSupported ? {passive: false} : false);

which looks like also adds a mouse-wheel handler to the scene

Shouldn't we also emit a plotly_relayout event with the aspectratio keys just like we do here:

plotly.js/src/plots/gl3d/scene.js

Lines 283 to 290 in 4a649f0

var relayoutCallback = function(scene) {

if(scene.fullSceneLayout.dragmode === false) return;

var update = {};

update[scene.id + '.camera'] = getLayoutCamera(scene.camera);

scene.saveCamera(gd.layout);

scene.graphDiv.emit('plotly_relayout', update);

};

for perspective projections?

Addressed in fe19e47

archmoj · 2019-10-21T21:15:37Z

@etpinard thanks very much for the review.
Setting the status to in progress...

- combine two wheel listeners - use gl-plot3d aspect setter and getter

etpinard · 2019-10-23T20:16:52Z

src/plots/gl3d/scene.js

+        update[scene.id + '.camera'] = getLayoutCamera(scene.camera);
+
+        // scene updates
+        update[scene.id + '.aspectratio'] = scene.glplot.getAspectratio();


This should only part of the event data on scenes with orthographic projections

... in other words, the event data should have the minimal set of keys that changed during the (here: scroll) interaction.

Done in b92230a.

And in d825c0e.

etpinard · 2019-10-23T20:21:51Z

src/plots/gl3d/scene.js

    if(hasChanged) {
        var preGUI = {};
        preGUI[this.id + '.camera'] = cameraDataLastSave;
+        preGUI[this.id + '.aspectratio'] = aspectDataLastSave;


Here too, we should only store the aspectratio change for orthographic projections.

Done in b92230a.

- consider aspectratio changes in modebar home and initial views - bump gl-plot3d 2.3.0

- test modebar with orthographic and add checks for initial aspect ratios - make sure plotly_relayout is emitted after each interaction

archmoj · 2019-10-24T18:10:41Z

@etpinard This PR should be ready for another review.
Thanks!

etpinard · 2019-10-24T18:50:25Z

src/plots/gl3d/scene.js

+            var aspectFullNP = Lib.nestedProperty(fullLayout, this.id + '.aspectratio');
+            aspectFullNP.set(aspectData);
+
+            this.glplot.redraw();


Hmm, why do we need to redraw here?

We used to redraw in scene wheel listener after updating gl-plot3d.aspect which is now moved to here so that we record the new before update.

I guess that's ok, it's just feels a little odd to me that a method called saveLayout ends up redrawing things in some cases.

Non-blocking.

We could possibly make gl3d/scene.js file more readable at one point.
I keep this in mind when we have time to refactor it in another PR.

I keep this in mind when we have time to refactor it in another PR.

Awesome. Thank you very much cleaning up this code!!

test/jasmine/tests/gl3d_plot_interact_test.js

etpinard · 2019-10-25T13:43:46Z

Nicely done @archmoj - 💃

record glplot aspect in fullLayout aspectratio

4a649f0

archmoj added bug something broken status: reviewable labels Oct 21, 2019

etpinard reviewed Oct 21, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

etpinard reviewed Oct 21, 2019

View reviewed changes

archmoj added status: in progress and removed status: reviewable labels Oct 21, 2019

Store direct aspectratio edits and emit relayout

fe19e47

- combine two wheel listeners - use gl-plot3d aspect setter and getter

archmoj mentioned this pull request Oct 23, 2019

move orthographic scroll zoom from the gl-plot3d scene to plotly scene gl-vis/gl-plot3d#24

Merged

archmoj added status: reviewable and removed status: in progress labels Oct 23, 2019

etpinard reviewed Oct 23, 2019

View reviewed changes

Save camera and aspectratio only if they change before redraw the scene

b92230a

- consider aspectratio changes in modebar home and initial views - bump gl-plot3d 2.3.0

archmoj added this to the v1.51.0 milestone Oct 23, 2019

archmoj added 4 commits October 23, 2019 22:04

improve relayout update object - now depends on changed parameter

d825c0e

correct initializeGLPlot argument

d486dcd

simplify gl3d pixelRatio pass

496059c

add new tests to cover both orthographic and perspective cases

e4a30be

- test modebar with orthographic and add checks for initial aspect ratios - make sure plotly_relayout is emitted after each interaction

etpinard reviewed Oct 24, 2019

View reviewed changes

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

add few more tests to lock down the orthographic relayout issue

3b28fae

archmoj merged commit 3b6832e into master Oct 25, 2019

archmoj deleted the fix4274-orthographic-zoom branch October 25, 2019 13:46

archmoj mentioned this pull request Oct 25, 2019

gl3d orthographic zoom using modebar button #4239

Closed

snyk-bot mentioned this pull request Feb 4, 2020

[Snyk] Upgrade plotly.js from 1.45.3 to 1.52.0 beauremus/ftp-viewer#1

Merged

snyk-bot mentioned this pull request Feb 22, 2020

[Snyk] Upgrade plotly.js from 1.35.2 to 1.52.1 raindigi/eShopOnContainersAI#1

Open

snyk-bot mentioned this pull request Apr 17, 2020

[Snyk] Upgrade plotly.js from 1.41.3 to 1.52.3 dunzoit/redash#4

Open

archmoj mentioned this pull request Jul 21, 2020

3d camera reset after react #5005

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep orthographic zoom scales in fullLayout #4292

Keep orthographic zoom scales in fullLayout #4292

archmoj commented Oct 21, 2019 •

edited

Loading

etpinard Oct 21, 2019

archmoj Oct 23, 2019

archmoj commented Oct 21, 2019

etpinard Oct 23, 2019 •

edited

Loading

etpinard Oct 23, 2019

archmoj Oct 23, 2019

archmoj Oct 24, 2019

etpinard Oct 23, 2019

archmoj Oct 23, 2019

archmoj commented Oct 24, 2019

etpinard Oct 24, 2019

archmoj Oct 24, 2019

etpinard Oct 24, 2019

archmoj Oct 24, 2019

etpinard Oct 24, 2019

etpinard commented Oct 25, 2019

	scene.glplot.canvas.addEventListener('wheel', function() {
	if(gd._context._scrollZoom.gl3d) {
	relayoutCallback(scene);
	}
	}, passiveSupported ? {passive: false} : false);

	var relayoutCallback = function(scene) {
	if(scene.fullSceneLayout.dragmode === false) return;

	var update = {};
	update[scene.id + '.camera'] = getLayoutCamera(scene.camera);
	scene.saveCamera(gd.layout);
	scene.graphDiv.emit('plotly_relayout', update);
	};

Keep orthographic zoom scales in fullLayout #4292

Keep orthographic zoom scales in fullLayout #4292

Conversation

archmoj commented Oct 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archmoj commented Oct 21, 2019

etpinard Oct 23, 2019 • edited Loading

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 Oct 24, 2019

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

etpinard commented Oct 25, 2019

archmoj commented Oct 21, 2019 •

edited

Loading

etpinard Oct 23, 2019 •

edited

Loading