maplibre / maplibre-gl-js

MapLibre GL JS - Interactive vector tile maps in the browser
https://maplibre.org/maplibre-gl-js/docs/
Other
6.61k stars 712 forks source link

Quick vertical dragging of pitched 3d map with mouse results in zoom action [3.0.0-pre.3] #2094

Closed danielfaust closed 7 months ago

danielfaust commented 1 year ago

maplibre-gl-js version: 3.0.0-pre.3

browser: Google Chrome Version 109.0.5414.75 (Offizieller Build) (64-Bit)

Steps to Trigger Behavior

  1. Quickly and repeatedly click mouse, push mouse up and release mouse
  2. Quickly and repeatedly click mouse, push mouse down and release mouse

891E321D-8C19-4150-A649-BAEA13C713F6

Link to Demonstration

https://output.jsbin.com/decicab/1 -- maplibre-gl@v3.0.0-pre.3 -- broken behavior https://output.jsbin.com/netasuw/1 -- maplibre-gl@2.4.0 -- correct behavior (Does no longer work because of new MapTiler restrictions)

https://output.jsbin.com/decicab/11 -- maplibre-gl@v3.0.0-pre.3 -- broken behavior https://output.jsbin.com/netasuw/11 -- maplibre-gl@2.4.0 -- correct behavior (Terrain limited to Innsbruck area)

Expected Behavior

The altitude of the camera should remain at the same height, making a mouse up/down drag result only in panning of the map, like in version 2.4.0.

Actual Behavior

Panning has zooming added to it. So multiple quick drags backing off of the visible location result in not only panning backward, but also in zooming out. Multiple quick drags to fly over the visible location not only result in panning forward, but also in zooming in.

danielfaust commented 1 year ago

When I started playing around with the 3.0.0 pre-release, I noticed that when I drag the mouse, then hold it and look at the map, as soon as I release the mouse the map moves a very tiny amount. As if the last on_mousemove were not the final position, but the on_mouseup would do something else which finally changes this position.

For some reason I believe that this could be related to changes introduced to fix the issue of the map not zooming into the point where the cursor position is located.

HarelM commented 1 year ago

Thanks for sharing! I'll check with version 3.0.0-pre.2 to see if this is related to the fix I did. Feel free to submit a PR if you would like to fix it faster.

danielfaust commented 1 year ago

I just checked, 3.0.0-pre.2 does not have the issue:

https://output.jsbin.com/menunih

HarelM commented 1 year ago

Related bug: #1024 fixed by PR: #1977. Feel free to take a look at the PR to see where a possible fix can be made (there aren't a lot of code changes there). I thought I didn't change the drag behavior, but it might be related to the moveend event somehow that I introduced, which felt a lot cleaner, but might have been buggy. Does this happen if you "slowly" pan the map as well or only when fast panning it?

danielfaust commented 1 year ago

I can't notice it when I slowly pan, but that doesn't mean that it's not happening. It seems to be related to mow much is panned in a certain amount of time.

The thing I mentioned in my first comment, where releasing the mouse which was dragged and held then moves the map a tiny amount, seems to be unrelated, because 3.0.0-pre.2 does exhibit the same "bug", but is does not have this zooming issue.

Since it is possible to drag the mouse around fast without having the zoom effect as long as the mouse is not released, it appears to be something which happens between the last drag and the moveend events. Ideally the position(?) data in moveend should match the last drag, but it appears that it doesn't and upon release something is computed from a delta of these two events. The faster the mouse moved, the bigger the delta, the stronger the zoom effect. Whatever that delta is.

I will look closer at the PR, but I'm already confused when looking at the changes in src/ui/handler/scroll_zoom.ts and src/ui/handler_manager.ts since I haven't worked on it.

danielfaust commented 1 year ago

In pre.2 there was this _drag variable which contained the location and position information. This has been replaced by _terrainMovement which is only a boolean, so that this _drag information appears to be lost. But I also don't see where _drag is used to calculate the zoom.

https://github.com/maplibre/maplibre-gl-js/pull/1977/files#diff-7ecdaf02a10f7c8c5f4923ec88f0718b3e290835958f145782bdd902f4b6bfceL452-L457

It could also be some if else-if else branch execution which is getting skipped, something like this one

https://github.com/maplibre/maplibre-gl-js/pull/1977/files#diff-7ecdaf02a10f7c8c5f4923ec88f0718b3e290835958f145782bdd902f4b6bfceL460-L464

which got merged into the opening if block but in there attached to a moveend callback.

HarelM commented 1 year ago

This is the first bounty we are defining. Link to parent Bounty: https://github.com/maplibre/maplibre/issues/189

tempranova commented 1 year ago

Is anyone else able to replicate this issue? I'm willing to try to fix, but I can't recreate the zooming out situation no matter how I try to do the rapid panning.

HarelM commented 1 year ago

I can't reproduce this locally when running npm run start and surfing to: http://localhost:9966/test/debug-pages/terrain-satellite.html I changed the zoom and pitch to match the example jsbin code (which for some reason doesn't work anymore). @danielfaust can you try and reproduce this using the dev-env with latest version from main? While I don't think a solution was added to the code, I do need a way to reproduce this locally, which I fail, and so does @tempranova ... Thanks!

danielfaust commented 1 year ago

@HarelM I fixed the original demos, the issue is that MapTiler is no longer serving the get_your_own_OpIi9ZULNHzrESv6T2vL-key, which was used to get the terrain data (apparently they do have https://maplibre.org whitelisted).

I now use the demo tiles from https://demotiles.maplibre.org/ which are limited to the Innsbruck area. Yet it turns out that this bug occurs only where an actual height displacement is under the cursor.

I also added a new demo using 3.0.0-pre.5 from last week and the problem still persists: https://output.jsbin.com/tuzorab/2

Hope this helps.

HarelM commented 1 year ago

I can't reproduce, sorry, which OS are you using? I also added the hash parameter to the map options to see how it changes in the address bar, and It does change but around the zoom it started with... Can you try and reproduce this in a different machine maybe?

danielfaust commented 1 year ago

I created the issue while on Windows 7, the results from yesterday were on Windows 11. I now checked on Kubuntu 22.04 with Firefox and also have that issue there.

Keep in mind that you need to make many fast mouse movements in of short durations one direction in order to clearly notice the effect. If you press and drag for large amounts, it will barely be noticeable.

It may be related to texture loading? But it's always like this: panning to south zooms out, to north zooms in.

04B468D1-8244-48F0-B185-0EAE4B4DF93B

HarelM commented 1 year ago

I can't reproduce on win 11, chrome or fire fox, I tried hard to drag fast... https://user-images.githubusercontent.com/3269297/235863556-a1e3f125-6e28-405e-ab9d-65fd381da5c8.mp4 I believe there's a bug, and I'm pretty sure it's related to the move-end I introduced, but if I can't reproduce it, I won't be able to see that the issue is fixed if I change some code. So I guess you'll need to dig into it... :-(

danielfaust commented 1 year ago

Maybe this is related: It happens when releasing the cursor and may be dependent on where the cursor is placed and the zoom level it is at, but it can be very noticeable. No quick movements required.

3EE5C1E1-84B1-4374-A5C5-68ED62DFC491

This is the location/zoom on the map: https://output.jsbin.com/tuzorab#8.21/46.832/11.683/0/45

HarelM commented 1 year ago

This is a different issue (although related) which the elevation is adjusted at the end of the movement. This existed in 2.4 however. The reason for that is to create a smooth movement of the camera, which was not the case on first implementation of the terrain. See #1492.

HarelM commented 1 year ago

I think I managed to reproduce this without using the mouse by using small movements (compass). I'll see if I can share an example that reproduce this...

danielfaust commented 1 year ago

Now that you mention it, pressing the down key zooms out of the terrain, while the up-key zooms into the center. This feels wrong. Zoom out for a while, and as soon as the terrain is out of bounds, it begins to pan.

Google Map's behavior is to always pan with the arrow keys and it feels like the correct thing to do.

That might be related to this issue, but in any case I'm wondering if I should open a new one for this.

While the issue in this entire thread is about is some kind of regression between 2.4.0 and 3.0.0, what I just mentioned is also occurring in 2.4.0.

Edit: And panning sideways with the arrow keys seems to be really broken above terrain.

HarelM commented 1 year ago

Sounds like a different issue...

HarelM commented 1 year ago

I've added the following code to the jsbin you provided inside the map load event:

        let a = 5;
        setInterval(() => {
          a *= -1;
          map.easeTo(
            {
              center, 
              bearing: 100 + a,
              animate: true,
              easing: (x) => x,
              offset: [0, 100]
            })
        }, 200)

In 2.4 it zooms in and in 3.1 it zooms out... strange...

danielfaust commented 1 year ago

The entire panning behavior is confusing. It appears to work per drag for a short amount of time. And sometimes when zooming out (if one manages to do that) it tries to zoom in up to a certain point and then stays there.

Also it's nearly impossible to pan sideways so much that the patch with the terrain stays out of view, for some reason it always tries to show it (possibly by zooming out).

https://output.jsbin.com/biqoyusaha/1#9.67/47.3258/11.6297/5/45

HarelM commented 1 year ago

Ha, no, no, this is using easeTo with the center so it will center the map to the same location always. It "simlates" a behavior I saw with my app, that has the compass changing the map bearing in small intervals and causes a zoom-out without user interaction. The idea behind this code is that it causes the zoom to change over time when the terrain is on. There's no need to drap or pan the map as it will return to the original center.

danielfaust commented 1 year ago

Ok, I thought that the center would adjust after each pan, but you're right, it's a fixed variable.

But then, why is it possible to actually change the center by doing many small pans? It's possible to have the original center to be at the corner of the screen while back-and-forth rotation occurs at the center of the screen.

Some internal variable appears to be getting modified slightly during the first step of a pan, some sort of offset. ~And as soon as the patch, or the center is outside of the screen, it snaps back to the center of the screen.~ Sometimes it snaps back to the center of the screen.

danielfaust commented 1 year ago

Ok, so I just upgraded the demo of this entire issue so that it uses version 3.1.0, and the issue still persists:

https://output.jsbin.com/nepitazehe/1#13/47.27574/11.39085/0/45

But disregarding the sideway-panning, maybe the fact that the key-up and key-down events actually perform a zoom in and out respectively, when over terrain, instead of a pan forward / backward could be part of the explanation of what is happening with the mouse. Bear in mind that I don't known the code, but what if during the first animation frame a mouse-move-forward gets interpreted or handled as a key-up event and mouse-move-backwards as a key-down event, that could explain the slight zoom effect. Albeit "slight" is relative, because on fast, long "short-durationed" mouse movements the zoom effect is much stronger.

It appears to be time-related. The time between the mouse-down and the mouse-up event will decide how much zooming will occur, also in relation to the amount which the mouse moved.

Is there a way to simulate mouse events?

HarelM commented 10 months ago

@prozessor13 I would like to try and solve this, but I think I'm missing some basicis. When I think about how the code works for 2D, I assume the elevation is 0 and the camera stays at the same height. Why can we do the same with 3D? Remove all the freeze elevation stuff, remove the martix translate, and keep the camera at the same distance to the 0 plane, so basically the same height and the terrain will render above and below this 0 plane? Afterward we can restrict the zoom/pitch to avoid collision with the terrain, but this won't be related to the camera movement during pan/zoom.

cc: @SnailBones Any help would be appreciated here, this code is very confusing to me...

prozessor13 commented 10 months ago

Hi Harel, the problem is with your implementation, that the zoomlevel do no longer corresponds with the camera to center distance. That creates the following problems:

but you are right. panning the camera looks very natural, because no rerendering is done.

HarelM commented 10 months ago

So if I get what you are saying right - the problem with my idea is that I can try and zoom in into the dead see but I won't be able to zoom below sea level because the camera is always above the 0 plane (drawing plane = sea level). Hmm... I see... I now understand the difference between 2D and 3D in that aspect - the camera in 2D is always above 0 plane (which is the drawing plane and has nothing to do with elevation etc). Thanks for the explanation!

prozessor13 commented 10 months ago

Yes correct! The dead-sea could be fixed with a global offset of 450meters, but this not fixing the zoomlevel & rendering problem. Also a map.setZoom(18) on a mountain would not work. Ok, this also can be fixed when recalculate (e.g decrese) the zoomlevel during setZoom to not collide with terrain, but then there is still the rendering issue. Long story short. I think it can be done this way, but with a lot of effort for fixing all side-effects.

HarelM commented 10 months ago

Let's assume for a second that we have a way to change the zoom in case of a collision with the terrain. Is it possible somehow to configure the zoom so that if we zoom in we might move below the 0 plane for high zoom levels maybe? The current logic of 2D and zoom is to cut the distance in half every time we zoom in? I'm not sure I fully understand all the math in the transform method that is relevant to this... :-/ CC: @kubapelc @Pheonor maybe you guys have a better understanding of a possible solution to this issue?

prozessor13 commented 10 months ago

I also think, with the current logic, this is not possible.

Pheonor commented 10 months ago

I test it a little, but I did not manage to reproduce the same problem as mention here. I look into the code and think that the problem could be the camera transformation that used the elevation as reference.

Effectively, the pan handler has a special case with terrain to keep the same elevation during drag (for me, this system allows to set a reference and avoid altitude movement during the transformation.) But at the end, it recomputes the zoom level with associated center position and elevation. This mechanism introduces some side effect as the new zoom level / center / elevation could represent a slightly different position as the one obtain from the last pan movement. Even with perfect data, the math operation could generate some numerical precision problem and change the zoom level.

To avoid switching between two representations, perhaps a way is to perform all camera transformation into an absolute frame not linked with the current elevation at the center (i.e. the sea level, the reference ellipsoid), then computes all elements "on the fly" into the final frame associated with the terrain elevation.

As an observation, but I am not sure, another side effect was potentially due to the elevation information availability when computing this new zoom level / center / elevation. If the tile under the center is not totally refreshed, the used elevation was not the final one. Once the tile has updated data, the camera altitude is recomputed with the new elevation and generate a little zoom change. Using an absolute position from a stable reference not linked with the elevation could help with that.

HarelM commented 10 months ago

If I understand what you wrote, I think this is what I did in my PR - use the reference as sea level. But it has problems when it comes to below sea level places such as the dead sea and "under water" maps, as can be read from @prozessor13 's response. It might be that the solution is to animate the end of the movement to return to the original zoom level that it was when the movement started, but it might be the current code - the recalculate zoom method, not sure...

prozessor13 commented 10 months ago

The current implementation does the following (may i repeat me):

After reading @Pheonor post twice ;) i think he is totally right:

So how to implement a absolute position logic?

When continuing using zoomlevel as reference for the z-direction distance (i think this cannot be changed, right?):

Go into Camera can collide with terrain:

Lets asume you set setZoom(16) on a mountain. what should happen when on this mountain the camera collides with terrain at zoomlevel 15? To avoid collision, the max allowed zoomlevel must be calculated and used. But then you are very close on terrain, lets say zoomlevel 22 in distance-perspective. What happens, when you switch in this situation to 2D. Are you then also in zoomlevel 22?

Go into LOD is no longer proportional to Zoomlevels:

In every position where LOD is done, not the current zoomlevel can used as reference, instead the distance from camera to the center-coordinate. I think of transform.coveringTiles and symbol-placement, may there are more places.

Go into Current zoom-level-logic stops near sealevel:

This can be done via an elevationOffset easily, but thats may a hack.

HarelM commented 10 months ago

Can you clarify the solution options?Assuming there are options here, as I didn't fully understand if this is a single solution with parts or different approaches to solve the problem...

prozessor13 commented 10 months ago

I did not thought very much into detail. I think first it has to clarified if it's all worth for changing this. For me, the most problematic thing is the breaking-change of the zoom <=> camera-center-distance <=> LOD correlation. If this change is thought in every aspect, i think the code-changes are much more easy. And may there is another solution for absolute position logic??

Pheonor commented 10 months ago

I made some tests and I found that just remove the "this.transform.elevation" update during the "map::_render" method allows to avoid the camera glitch at the end of the pan movement. This avoid to change the elevation after the camera zoom update and keep the elevation used during this computation. Side effect, the used elevation is not the final one, but I think this is not a big deal as the one used during the zoom update is sufficiently close of the final one. Perhaps the elevation update when enable a terrain should be manage outside this render method.

HarelM commented 10 months ago

I believe the issue described here is not the "jump" of the map, but rather the fact that when the movement finishes, the zoom changes a bit, if you do a lot of movements (for example in my case I change the map direction to reflect the phone's orientation towards north), this adds up and you can find yourself in a total different zoom level than what you intended...

Pheonor commented 10 months ago

Ok. I was wondering that the cumulated error due to "elevation" change between each quick movement could be an explanation of the zoom change. The change in orientation change the "height" and so the "cameraToCenterDistance" used during all camera position computation. The absolute camera logic will fix it.

HarelM commented 10 months ago

Can you elaborate on your last sentence? "The absolute camera logic will fix it."

Pheonor commented 10 months ago

The current camera position (camera location and altitude) is computed with the "center of view" based on "width" and "height", the "cameraToCenterDistance" based on "height", the "pitch" and "bearing", the "pixel per meter" based on "zoom" and "center", and the "elevation" at "center" point. To keep the same camera position, it is needed to update some parameters when another change (e.g. if the "elevation" change, we need to update the "zoom" to keep the same camera position). When the "height" change, the "zoom" is not recomputed. We still point the same location (center and elevation) into the same direction (pitch and bearing) but not at the same distance due to the change into "cameraToCenterDistance".

One option is to recompute the "zoom" when the "height" change to ensure keeping the same camera position.

Another option is to use an absolute camera position with a fix position (camera location and altitude) and a fix target (center and elevation) and to compute all other elements from this couple. With this system, when the "height" change, the camera position remain the same but the transformation matrix to project the map into the camera view will be updated to take into account the height change.

HarelM commented 10 months ago

@Pheonor any chance you can draw it and publish the drawing? Something like the drawing in the following issue: https://github.com/maplibre/maplibre-gl-js/pull/1578#issuecomment-1246503321, https://github.com/maplibre/maplibre-gl-js/issues/1656#issue-1376751702

If I understand correctly, instead of changing the zoom one can change the transform parameters to achieve the same results? Seems like a better course of action since the zoom is a "publicly" available parameter while the transform is more "internal", I think...

prozessor13 commented 10 months ago

For me, the most problematic thing is the breaking-change of the zoom <=> camera-center-distance <=> LOD correlation.

May i have to clarify the camera-center-distance. I do not mean the variable cameraToCenterDistance, which is a more or less static value, based on FOV and map-height. What i mean is the value of the camera to the center-coordinate in meters, which depends also on the current zoomlevel, which factor is represented by the _pixelPerMeter IMG_3492

So to get rid of all the center-elevation stuff, some other parameter (as @Pheonor explained) must represent that logic. And i think this can only be the zoom-value. So here are two drawings which explains switching from 2D to 3D. First the current state with the elevation-value, second to change zoom-value. But this scenario is only one to think about.

IMG_3493 IMG_3494

But the next big issue is (as explained above): In drawing two you are then in zoom 15.4, but the distance from the camera to the peak of the mountain looks like, that we are in zoomlevel 17. And because all LevelOfDetail Logic is based on the current zoom, the peak is rendered with much to less details.

So, i think this can all be changed, but is it worth?

Another option is to use an absolute camera position with a fix position (camera location and altitude) and a fix target (center and elevation) and to compute all other elements from this couple.

@Pheonor: i do not fully understand what you mean. When letting the camera position as it is, the terrain beside the map-center has to offset in z-direction. But isn't that the same, than offset the camera in -z direction (as it is now)?

Pheonor commented 10 months ago

To clarify, I propose to store the camera 3D position and to compute the other element as the Zoom Level from this 3D position wheen needed. But as you explain, if the camera do not move when switching from 2D to 3D, the visible map bounds will change and this is not the expected effect. So, storing the camera 3D position is not a viable approach.

Do you think the LOD system could use a Zoom Level computed from the sea level without taking acount the terrain elevation ?

prozessor13 commented 10 months ago

Do you think the LOD system could use a Zoom Level computed from the sea level without taking acount the terrain elevation ?

If i understand your question right:

i think this is only be relevant when working with offsets, either camera in -z direction or render terrain in center-coordinate to sealevel and all around with a z-offset. So, i would say: this is exactly what currently is going on.

On the other hand, in a scenario like drawing 2, i think the distance (in meters) from the camera to the center-elevation is relevant for the zoomlevel of which the LOD stuff is working on. If you mean that in your question: yes, i think this can be done. May it is easy do-able with a second, internal, zoom-level, but i am not sure if the visibility checks of the surrounding tiles is then working correct.

I think it is an interesting idea to change the elevation-offset stuff, but i think this change needs some more efficient discussion, for example an "online-meeting". First to identify the problems with the current implementation, and second to build scenarios for a new implementation. Else i think this gets an endless discussion thread :)

HarelM commented 10 months ago

Can we use the next monthly meeting for this? It is in the coming Wednesday (10.1). Will you guys be able to join?

prozessor13 commented 10 months ago

Yes, i think this is possible for me.

Pheonor commented 10 months ago

Yes, I will try to join.

danielfaust commented 9 months ago

The notification from the rejection of the PR https://github.com/maplibre/maplibre-gl-js/pull/3511#issuecomment-1906911726 made me look at this issue again.

I did just test this with 4.0.0-pre.4 and it appears to be fixed, at least in regards to mouse movement. I believe there are still some minor offsets which get introduced upon mouse release, but they are barely noticeable and don't seem to add up in a bad way. I don't know what changed with 4.0.0-pre.X and what the PR was supposed to do, so I won't comment on the rejection of the PR.

A remaining problematic issue is that the navigation with the arrow keys is not behaving as expected. This might justify opening a new issue, but in my opinion it is strongly related to this one and also existed for as long as this one existed.

Arrow up key zooms in when over terrain, and arrow down zooms out. When not over terrain, up and down are panning actions.

For reference, this was @v3.1.0: https://output.jsbin.com/nepitazehe/1#13/47.27574/11.39085/0/45

This comment was tested on 4.0.0-pre.4: https://output.jsbin.com/rewogeneyi/1#13/47.27574/11.39085/0/45

You can reproduce this by first zooming out a bit in order to pan out of the terrain area more easily and pan north with the mouse until you're well out of the terrain area (a bit over München), then use the downward arrow key to navigate to the south. As soon as the terrain section appears, the southward panning changes to zooming out. As soon as the section is left again, it will switch to panning again.

A349EFCD-96DB-4360-BAA0-714FFF6398FB

My apologies for not contributing code-wise.

HarelM commented 9 months ago

The "rejection" (I closed my own PR :-)) was a consequence of the discussion here and the discussion in the monthly meeting. My solution was simply too naive, as can be read in this thread. I don't think anything dramatically changed to have fixed this issue, if you remember my initial comment, it was not easy to reproduce, so it might be a browser version change or something on your end.

danielfaust commented 9 months ago

Regarding the arrow navigation, I just visited

https://maplibre.org/maplibre-gl-js/docs/examples/3d-terrain/

and set the perspective to be perpendicular to the ground and used the arrow keys to navigate. An odd thing is that this zooming is not constantly present, but every couple of key presses it then pans instead of zooms.

E7025450-2477-4552-8FB5-F7F930F2B15C

Regarding to the browser version change or something on my end, I just checked which version fixed the mouse panning issue, and while version 3.3.0 still had it, 3.4.0 no longer had it. I thought it was a fix related to 4.0 because I just tested the last one available, but it was 3.4.0. I can clearly reproduce the problem with 3.3.0.

v3.3.0: https://output.jsbin.com/wohipew/1#13/47.27574/11.39085/0/45

v3.4.0: https://output.jsbin.com/qizucan/1#13/47.27574/11.39085/0/45

HarelM commented 9 months ago

Navigation using the arrows is a different issue, I would recommend moving the discussion there once you open it.