feat(globeControls): zoom on mouse position while using wheel #2041

AnthonyGlt · 2023-03-20T10:02:03Z

Description

In globe view, zoom to the cursor position while using mousewheel.

Motivation and Context

Linked issue: #2011

ftoromanoff · 2023-04-20T09:30:33Z

From what i understand of this PR, the purpose is that when you zoom in or out, the terrain under the mouse should always stay the same.

I tried this PR on a globe view. I noticed that zooming in on the mouse position seems to work when you are at close range but not at all when you are far enought and the zoom out never stay at the mouse position.
I havn't really gone into the code yet, and from my first test, the aim is not achived.

AnthonyGlt · 2023-04-21T07:42:10Z

Thanks for the feedback.

I use zoom coordinates interpolated between the mouse position and the camera position that's why we don't go directly to the mouse position. This allows us to have a smoother transition. The behaviour is similar to the globe view of Google Maps. This alpha parameter (interpolation factor) is fixed but depending on feedback, could be edited before release. Concerning the zoom out, the factor is different (dezoom_interp) to avoid brutal rotation while zooming out. Same as alpha, this could be edited before release. I could even make these values as parameters of globecontrols, then the user could decide of the zoom behaviour.

alexLuky · 2023-04-21T07:56:52Z

@ftoromanoff @AnthonyGlt @jailln
By the way, take a look at this control. It has all useful functions.
I've mentioned it here in another discussion.

I think it would be great to integrate it in iTowns. Also for navigation under the ground or in hidden globe mode.

gchoqueux · 2023-04-21T13:39:15Z

With your PR, the target position doesn't stay under the mouse cursor.

cursor.mp4

In the same condition with google earth, the target position stay under the cursor and doesn't come out of the screen.

cursorgoogleearth.mp4

AnthonyGlt · 2023-04-25T13:09:58Z

I was referring to Google Maps (cf attached record). If this isn't an appropriate solution, an alternative could be implemented to have a similar zoom as Google Earth instead. Currenctly, I used a bit of a mix of the zoom function from the planarControls (interpolation) and the double click zoom from the globeControls (travel -> lookAtCoordinate).

map.mp4

gchoqueux · 2023-05-02T13:09:26Z

it's good feature that improves the user experience.
I'm asking if it would be possible to factor your code for use in the handleDolly(event) method (call by pressing the middle button)

AnthonyGlt · 2023-05-05T12:27:25Z

it's good feature that improves the user experience. I'm asking if it would be possible to factor your code for use in the handleDolly(event) method (call by pressing the middle button)

It's now implemented. I store the coordinates when the wheel button is clicked to zoom at the precise mouse location. But it's not really a "dolly" anymore.
In a future improvement, we could also store the first coordinates when the mouse wheel is used, to zoom to the exact mouse location. I did it on another branch of my repo. I did a draft PR on it: #2091

dolly.mp4

gchoqueux

Your PR is good way to add this feature
In the future we will have to think about how to solve the issue of the pointer shift when zooming.
This issue is accentuated when the zoomFactor is increased

src/Controls/GlobeControls.js

gchoqueux · 2023-05-15T14:16:24Z

src/Controls/GlobeControls.js

+            if (camPos.x * point.x < 0) {     // Correct rotation at 180th meridian by using 0 <= longitude <=360
+                if (camPos.x - point.x > 180) { point.x += 360; } else if (point.x - camPos.x > 180) { camPos.x += 360; }
+            }


It's a lookAtCoordinate issue, it's not support a correct rotation at 180th meridian by using 0 <= longitude <=360?

No it's for the lerp method (interpolation). At the 180th meridian, we have values going from -179 to 179 (-179, -180/180, 179). So when interpolating a value between, for example, -170 and 172, we will obtain a value of 1 instead of the expecting -179.
To avoid that, I add 360 (to keep the same value modulo 360) and interpolate on positive values only. Then I substract the 360 to get the value back between -180 and 180.

src/Controls/GlobeControls.js

gchoqueux · 2023-06-09T10:07:13Z

src/Controls/GlobeControls.js

+        // Store first position of mouse when dolly
+        if (!dollyOrigin) {
+            dollyOrigin = this.view.getPickingPositionFromDepth(event.viewCoords);        // mouse position
+        }
+


could you use pickedPosition instead of dollyOrigin, it seems to have a duplicate use

Yes you're right, I could use pickedPosition.

this.view.getPickingPositionFromDepth(event.viewCoords, pickedPosition); // mouse position

But I need something to know if I'm doing a dolly (cf next comment)

StateControl should handle dolly state?

gchoqueux · 2023-06-09T10:11:36Z

src/Controls/GlobeControls.js

-        this.updateTarget();
-        const delta = -event.delta;
-        this.dolly(delta);
+        var point = dollyOrigin ?? this.view.getPickingPositionFromDepth(event.viewCoords);        // mouse position


There's a case where the dollyOrigin is null

could you rename point with explicit name, pickedPoint?

If dolly, then I don't update pickedPoint value. So I can use a boolean instead of dollyOrigin. What do you think ?

Suggested change

var point = dollyOrigin ?? this.view.getPickingPositionFromDepth(event.viewCoords); // mouse position

var pickedPoint = dollyFlag ? pickedPosition : this.view.getPickingPositionFromDepth(event.viewCoords); // mouse position

gchoqueux · 2023-06-09T10:12:26Z

src/Controls/GlobeControls.js

@@ -759,26 +770,36 @@ class GlobeControls extends THREE.EventDispatcher {
        }
    }

-    handleZoom(event) {
+    handleZoom(event, dollyOrigin) {


I can't remember if there's a change of state for the zoom?
to handle the state zoom

no state change for the zoom. I need to disguish between a regular zoom and a dolly one. Maybe I could

Suggested change

handleZoom(event, dollyOrigin) {

handleZoom(event, dollyFlag) {

with dollyFlag used to know if I need to update pickedPosition value

no state change for the zoom. I need to disguish between a regular zoom and a dolly one. Maybe I could

This would seem to confirm that StateControl should manage its two states?

in my last commit, i've used directly the event type to distinguish between the 2 zoom

src/Controls/GlobeControls.js

AnthonyGlt · 2024-06-18T09:42:02Z

@Desplandis rebased 👍

AnthonyGlt · 2024-09-10T08:03:45Z

@Desplandis rebased 👍

mgermerie

Hi ! Thanks for the improvement on zoom ! There is just one functional issue and a few remarks on form listed in my review.
Also, when you'll rebase your commits, could you add a breaking change note in the commit message ?

mgermerie · 2024-09-20T15:51:27Z

src/Controls/GlobeControls.js

+            }
+            point.lerp(  // point interpol between mouse cursor and cam pos
+                camPos,
+                (event.delta > 0 ? this.zoomInScale : this.zoomOutScale), // interpol factor


You could directly use zoomScale defined on line 768 instead of having to check event.delta again.

src/Controls/GlobeControls.js

mgermerie

Thanks for your commit that indeed fixes the issue when zooming over minDistance value.

Trying out the example again, I came upon a glitch that occurs when zooming with the mouse wheel while orbiting (with ctrl+drag). On master branch, one can do so as you can test on view_3d_map example

When doing the same on your branch, I noticed that the camera moves strangely. Moreover, if I keep orbiting after having rolled the mouse wheel, the camera zooms out. The zoom should not be affected by an orbit movement.

I found out this is due to this line in lookAtCoordinates method which you call in your new handleZoom. It modifies cameraTarget which is used as a reference in handlePanoramic, thus for orbit movement. Removing this line fixes the issue I mention but brings out some more issues, notably with "dolly" movement (wheel-click + drag)

AnthonyGlt · 2024-09-30T10:19:36Z

@mgermerie we could just cancel the Panoramic once the zoom is triggered (looks like how it's handled in Google Earth), WDYT ?
By adding
this.currentState = this.NONE;
before
this.dispatchEvent({ type: this.ZOOM._event, delta: event.deltaY, viewCoords });

in

itowns/src/Controls/StateControl.js

Lines 394 to 401 in 8de37c9

    
           onMouseWheel(event) { 
        
               event.preventDefault(); 
        
               if (this.enabled && this.ZOOM.enable) { 
        
                   viewCoords.copy(this._view.eventToViewCoords(event)); 
        
                   this.dispatchEvent({ type: this.ZOOM._event, delta: event.deltaY, viewCoords }); 
        
               } 
        
           }

Desplandis · 2024-10-04T08:02:37Z

@AnthonyGlt I agree with you on that point, we should disable this behavior (is this really used BTW?) to mimic the one in Earth and to decrease the complexity of state handling.

@mgermerie Any thoughts?

AnthonyGlt · 2024-10-10T08:56:53Z

@Desplandis squashed 🎾

mgermerie · 2024-10-11T09:16:45Z

@AnthonyGlt Your proposition is ok for me in terms of how to code it.
Now, I personally would not be so found of deleting it. The behavior already exists in master branch, and is implemented in Google Maps as well. However, this is a personal preference and I don't know if this behavior is used in iTowns so if you decide to remove it, it is ok for me as long as you document a breaking change.

BREAKING CHANGE: disabled multi actions when zooming

AnthonyGlt · 2024-10-11T09:35:53Z

@AnthonyGlt Your proposition is ok for me in terms of how to code it. Now, I personally would not be so found of deleting it. The behavior already exists in master branch, and is implemented in Google Maps as well. However, this is a personal preference and I don't know if this behavior is used in iTowns so if you decide to remove it, it is ok for me as long as you document a breaking change.

@mgermerie I've added the breaking change to the commit

Thank you for your review

AnthonyGlt force-pushed the zoom_clean branch from d0ddc9b to 446713b Compare March 20, 2023 11:24

jailln requested a review from mgermerie April 19, 2023 07:28

AnthonyGlt force-pushed the zoom_clean branch from 446713b to f19b0da Compare April 20, 2023 14:57

AnthonyGlt force-pushed the zoom_clean branch from 6795e45 to 12224ec Compare May 5, 2023 12:13

AnthonyGlt mentioned this pull request May 5, 2023

feat(globeControls): zoom improved #2091

Closed

gchoqueux reviewed May 15, 2023

View reviewed changes

AnthonyGlt force-pushed the zoom_clean branch from 0a3fda1 to b22baa5 Compare May 16, 2023 10:23

AnthonyGlt requested a review from gchoqueux May 17, 2023 07:41

gchoqueux added the feature 🍏 Adds a new feature label May 17, 2023

gchoqueux reviewed Jun 9, 2023

View reviewed changes

jailln assigned AnthonyGlt and mgermerie Jul 13, 2023

AnthonyGlt force-pushed the zoom_clean branch from 877cbbd to c0dd9b8 Compare July 26, 2023 10:29

AnthonyGlt requested a review from gchoqueux July 26, 2023 10:33

AnthonyGlt force-pushed the zoom_clean branch from c0dd9b8 to 3aa0f4f Compare March 7, 2024 10:31

AnthonyGlt requested a review from Desplandis March 7, 2024 10:33

AnthonyGlt force-pushed the zoom_clean branch from 3aa0f4f to 6b70062 Compare June 13, 2024 15:44

AnthonyGlt force-pushed the zoom_clean branch from 6b70062 to c55cd1e Compare September 10, 2024 08:03

mgermerie reviewed Sep 20, 2024

View reviewed changes

AnthonyGlt force-pushed the zoom_clean branch from c55cd1e to 05ea375 Compare September 24, 2024 13:47

AnthonyGlt requested a review from mgermerie September 24, 2024 15:14

mgermerie reviewed Sep 27, 2024

View reviewed changes

feat(globeControls): zoom on mouse position while using wheel

bb237f7

AnthonyGlt force-pushed the zoom_clean branch from 2bbb9cb to 06e7874 Compare October 10, 2024 08:56

feat(controls): disabled multi actions when zooming

412ec12

BREAKING CHANGE: disabled multi actions when zooming

AnthonyGlt force-pushed the zoom_clean branch from 06e7874 to 412ec12 Compare October 11, 2024 09:35

mgermerie approved these changes Oct 11, 2024

View reviewed changes

AnthonyGlt merged commit 89bbbd8 into iTowns:master Oct 11, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(globeControls): zoom on mouse position while using wheel #2041

feat(globeControls): zoom on mouse position while using wheel #2041

AnthonyGlt commented Mar 20, 2023

ftoromanoff commented Apr 20, 2023

AnthonyGlt commented Apr 21, 2023

alexLuky commented Apr 21, 2023

gchoqueux commented Apr 21, 2023

AnthonyGlt commented Apr 25, 2023

gchoqueux commented May 2, 2023

AnthonyGlt commented May 5, 2023 •

edited

Loading

gchoqueux left a comment

gchoqueux May 15, 2023

AnthonyGlt May 16, 2023

gchoqueux Jun 9, 2023

AnthonyGlt Jun 13, 2023 •

edited

Loading

gchoqueux Jun 28, 2023

gchoqueux Jun 9, 2023

gchoqueux Jun 9, 2023

AnthonyGlt Jun 13, 2023 •

edited

Loading

gchoqueux Jun 9, 2023

AnthonyGlt Jun 13, 2023

gchoqueux Jun 28, 2023

AnthonyGlt Jul 26, 2023

AnthonyGlt commented Jun 18, 2024

AnthonyGlt commented Sep 10, 2024

mgermerie left a comment

mgermerie Sep 20, 2024

mgermerie left a comment

AnthonyGlt commented Sep 30, 2024

Desplandis commented Oct 4, 2024

AnthonyGlt commented Oct 10, 2024

mgermerie commented Oct 11, 2024

AnthonyGlt commented Oct 11, 2024

	var point = dollyOrigin ?? this.view.getPickingPositionFromDepth(event.viewCoords); // mouse position
	var pickedPoint = dollyFlag ? pickedPosition : this.view.getPickingPositionFromDepth(event.viewCoords); // mouse position

	handleZoom(event, dollyOrigin) {
	handleZoom(event, dollyFlag) {

feat(globeControls): zoom on mouse position while using wheel #2041

feat(globeControls): zoom on mouse position while using wheel #2041

Conversation

AnthonyGlt commented Mar 20, 2023

Description

Motivation and Context

ftoromanoff commented Apr 20, 2023

AnthonyGlt commented Apr 21, 2023

alexLuky commented Apr 21, 2023

gchoqueux commented Apr 21, 2023

AnthonyGlt commented Apr 25, 2023

gchoqueux commented May 2, 2023

AnthonyGlt commented May 5, 2023 • edited Loading

gchoqueux left a 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

AnthonyGlt Jun 13, 2023 • 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

AnthonyGlt Jun 13, 2023 • 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

AnthonyGlt commented Jun 18, 2024

AnthonyGlt commented Sep 10, 2024

mgermerie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgermerie left a comment

Choose a reason for hiding this comment

AnthonyGlt commented Sep 30, 2024

Desplandis commented Oct 4, 2024

AnthonyGlt commented Oct 10, 2024

mgermerie commented Oct 11, 2024

AnthonyGlt commented Oct 11, 2024

AnthonyGlt commented May 5, 2023 •

edited

Loading

AnthonyGlt Jun 13, 2023 •

edited

Loading

AnthonyGlt Jun 13, 2023 •

edited

Loading