Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
danielfaust opened this issue Jan 23, 2023 · 62 comments · Fixed by #3836
Closed
Labels
💰 bounty S Small Bounty, USD 100 bug Something isn't working PR is more than welcomed Extra attention is needed terrain

Comments

@danielfaust
Copy link

danielfaust commented Jan 23, 2023

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 -- [email protected] -- broken behavior
https://output.jsbin.com/netasuw/1 -- [email protected] -- correct behavior
(Does no longer work because of new MapTiler restrictions)

https://output.jsbin.com/decicab/11 -- [email protected] -- broken behavior
https://output.jsbin.com/netasuw/11 -- [email protected] -- 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
Copy link
Author

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
Copy link
Collaborator

HarelM commented Jan 23, 2023

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.

@HarelM HarelM added bug Something isn't working PR is more than welcomed Extra attention is needed terrain labels Jan 23, 2023
@danielfaust
Copy link
Author

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

https://output.jsbin.com/menunih

@HarelM
Copy link
Collaborator

HarelM commented Jan 23, 2023

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
Copy link
Author

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
Copy link
Author

danielfaust commented Jan 23, 2023

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
Copy link
Collaborator

HarelM commented Jan 24, 2023

This is the first bounty we are defining.
Link to parent Bounty: maplibre/maplibre#189

@HarelM HarelM added the 💰 bounty S Small Bounty, USD 100 label Jan 24, 2023
@tempranova
Copy link
Contributor

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
Copy link
Collaborator

HarelM commented Apr 27, 2023

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!

@HarelM HarelM added the need more info Further information is requested label Apr 27, 2023
@danielfaust
Copy link
Author

danielfaust commented Apr 28, 2023

@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
Copy link
Collaborator

HarelM commented Apr 28, 2023

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
Copy link
Author

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
Copy link
Collaborator

HarelM commented May 3, 2023

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
Copy link
Author

danielfaust commented May 3, 2023

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
Copy link
Collaborator

HarelM commented May 3, 2023

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
Copy link
Collaborator

HarelM commented Jun 15, 2023

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
Copy link
Author

danielfaust commented Jun 15, 2023

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
Copy link
Collaborator

HarelM commented Jun 16, 2023

Sounds like a different issue...

@HarelM
Copy link
Collaborator

HarelM commented Jun 18, 2023

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
Copy link
Author

danielfaust commented Jun 18, 2023

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
Copy link
Collaborator

HarelM commented Jun 18, 2023

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
Copy link
Author

danielfaust commented Jun 18, 2023

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
Copy link
Author

danielfaust commented Jun 18, 2023

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
Copy link
Collaborator

HarelM commented Dec 31, 2023

@Pheonor any chance you can draw it and publish the drawing?
Something like the drawing in the following issue: #1578 (comment), #1656 (comment)

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
Copy link
Collaborator

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
Copy link
Contributor

Pheonor commented Jan 3, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

HarelM commented Jan 4, 2024

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
Copy link
Collaborator

Yes, i think this is possible for me.

@Pheonor
Copy link
Contributor

Pheonor commented Jan 4, 2024

Yes, I will try to join.

@danielfaust
Copy link
Author

danielfaust commented Jan 24, 2024

The notification from the rejection of the PR #3511 (comment) 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
Copy link
Collaborator

HarelM commented Jan 24, 2024

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
Copy link
Author

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
Copy link
Collaborator

HarelM commented Jan 25, 2024

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

@danielfaust
Copy link
Author

danielfaust commented Jan 25, 2024

Thanks. I will open an issue but not right now. While creating the issue I also tested on a Linux bare bones installation as well as Linux and Windows inside VirtualBox and QEMU/KVM and noticed different behaviors so I cancelled the creation of the issue. I will first look into this before opening a new one.

@UberMouse
Copy link

This issue seems to the same/related to an issue we are experincing at Koordinates with our maplibre map-viewer renderer (it uses openlayers by default but you can opt into maplibre and we are intending to replace openlayers with maplibre). Basically any pan action when you are zoomed out enough to see most of Australia (I assume the same effect happens in other locations but Australia reproduces it easily), as long as the camera centre is over Australia (having Australia in view but centre over the sea has no issues), causes you to zoom further out until you can't zoom any further. See this video for an example. This issue is only present with terrain enabled.

Since I don't know where you can find an example to use with the maptiler terrain and up to date maplibre you can test this using our mapviewer at https://koordinates.com/data/ opt into the 3d map viewer from the modal that pops up (or opt into the 3D map viewer trial from the View menu in the mapviewer toolbar)

This issue is present in v4.0.1 across all browsers/OS's we have tested (Vivaldi on Ubuntu 22 and Windows 12, Safari/Firefox/Chrome on macOS). I found this issue today while investigating the problem and found the changes in #3511, I took the changes in that PR and applied them as a patch to maplibre in our repo and verified that the problem is resolved.

Not that there aren't other issues caused by the patch, noticeably around being closely zoomed into terrain and the map "whiting out", and separately of course you can now move the camera into terrain (I say separately even though you would think those two things are related because you can certainly position the camera inside the terrain without the map "whiting out")

We're still doing testing on that change but I think we may accept the trade off of wonky camera terrain interactions for resolving the panning bug. As a company based in New Zealand panning over Australia is not an uncommon thing to do and having it zoom you out when you do that is... not great 😅

@prozessor13
Copy link
Collaborator

Hi, thank you for your explanation. I've encountered the same issue in Australia as well, which is beneficial for identifying the bug. As a temporary fix, you can adjust the minzoom setting of your terrain source to, for instance, 7 or 8. At these zoom levels, the visual disparity is minimal, thus mitigating the problem to some extent. Although the issue persists at higher zoom levels, it is considerably less pronounced. Additionally, I am currently swamped with tasks, making it impossible for me to address this issue in the near future.

@UberMouse
Copy link

UberMouse commented Feb 21, 2024

Interesting. Setting the minzoom to 7 for the terrain source solves the problem. Even when I can still see the terrain (I am zoomed out though so I can't tell if there's some fakery going with "2d" terrain at low zooms since it would be hard to tell the difference). So we've deployed that change to production and it's good enough for our purposes for now, thanks :)

@WuMianzhi
Copy link

Hey, I ran into another glitch when checking out this issue. When I use the arrow keys to move from a spot with no terrain to one that does have terrain, right at the edge, the map starts freaking out and shaking like crazy. Do you think this is happening for the same reason you guys talked about before, something to do with the camera reference plate? Anyway, I'm trying to fix this in my spare time.

@olsen232
Copy link
Contributor

Along the same lines: if you have implemented a Custom Layer Interface and you have terrain enabled and you find that your custom layer appears to be changing its Z-height relative to the terrain at the end of a panning motion - you might instead be experiencing the somewhat related but different issue #3797

@WuMianzhi
Copy link

Along the same lines: if you have implemented a Custom Layer Interface and you have terrain enabled and you find that your custom layer appears to be changing its Z-height relative to the terrain at the end of a panning motion - you might instead be experiencing the somewhat related but different issue #3797
Your are right, I just found this issue too, even if you just Map.flyTo a center, dont assign zoom option, the zoom of the map has also changed

olsen232 added a commit to olsen232/maplibre-gl-js that referenced this issue Mar 14, 2024
olsen232 added a commit to olsen232/maplibre-gl-js that referenced this issue Mar 14, 2024
As described in maplibre#2094
panning the map can result in a sudden jump in zoom-level as the
drag is released. This jump tends to be worse when more zoomed out,
and repeated short pans cause the zoom jumps to happen repeatedly
in the same direction, which will grow exponentially if each
one is zooming out - each zoom out causes the bug to get worse.

The problem lies in recalculateZoom, which tries to calculate a new
zoom level such that the previous camera altitude + terrain elevation
is the same as the current camera altitude + terrain elevation.
Camera altitude here refers specifically to the camera's height
above the terrain elevation, and terrain elevation is the terrain
height at the centre of the screen. (This doesn't really leave any
good nouns for describing the sum of these two variables, but
this sum should remain constant and is the camera's total height
above sea level).

A specific zoom maps - more-or-less - to a specific camera altitude.
Since the terrain elevation can be different (eg 100m higher) at the
end of a terrain drag, we need the camera altitude to be different
too (eg 100m lower to compensate). So, we need to recalculate the
zoom to one that maps to the required camera altitude. And so,
the function recalculateZoom exists to fulfil this purpose.

Unfortunately, the function miscalculates the required zoom, as
can be verified by comparing camera altitude + terrain elevation
before recalculateZoom is called vs after - very slight changes
in terrain elevation (10s of metres) lead to large and unrelated
changes in camera altitude (1000s of metres) when the camera is
very zoomed out.

Currently, the logic in recalculateZoom uses different math to
the math used to calculate the camera's altitude from the
current zoom. This different math may be close to correct, but,
it's impossible for me to debug. In this commit I've simply
replaced it with the inverse of the math used to calcualte the
camera's altitude from its zoom - ie:
if a zoom Z gives a camera altitude f(Z)

Then, to get the camera to required altitude A,
we need to set the zoom to Z = inverse-of-f(A)
@olsen232
Copy link
Contributor

olsen232 commented Mar 14, 2024

The unwanted-zooming-when-finishing-panning behaviour is fixed by #3836
I just need to complete the launch checklist, but I'm linking to it here for others to test / comment on.

Any unwanted panning when finishing panning is not fixed, anything to do with keypresses is not fixed - this only affects how the zoom is calculated at the end of a pan.

olsen232 added a commit to olsen232/maplibre-gl-js that referenced this issue Mar 15, 2024
Added asserts that camera altitude remains invariant throughout the
recalculateZoom test. These new asserts fail before the fix for 2094.

Moved the asserts that the zoom is a particular value to after the
altitude asserts - a particular zoom is correct if and only if it
results in the correct altitude, so whether the altitude assert fails
is more important information to have in a failing test.

Updated CHANGELOG.md
HarelM added a commit that referenced this issue Mar 15, 2024
* Fix for #2094 - unwanted zoom changes

As described in #2094
panning the map can result in a sudden jump in zoom-level as the
drag is released. This jump tends to be worse when more zoomed out,
and repeated short pans cause the zoom jumps to happen repeatedly
in the same direction, which will grow exponentially if each
one is zooming out - each zoom out causes the bug to get worse.

The problem lies in recalculateZoom, which tries to calculate a new
zoom level such that the previous camera altitude + terrain elevation
is the same as the current camera altitude + terrain elevation.
Camera altitude here refers specifically to the camera's height
above the terrain elevation, and terrain elevation is the terrain
height at the centre of the screen. (This doesn't really leave any
good nouns for describing the sum of these two variables, but
this sum should remain constant and is the camera's total height
above sea level).

A specific zoom maps - more-or-less - to a specific camera altitude.
Since the terrain elevation can be different (eg 100m higher) at the
end of a terrain drag, we need the camera altitude to be different
too (eg 100m lower to compensate). So, we need to recalculate the
zoom to one that maps to the required camera altitude. And so,
the function recalculateZoom exists to fulfil this purpose.

Unfortunately, the function miscalculates the required zoom, as
can be verified by comparing camera altitude + terrain elevation
before recalculateZoom is called vs after - very slight changes
in terrain elevation (10s of metres) lead to large and unrelated
changes in camera altitude (1000s of metres) when the camera is
very zoomed out.

Currently, the logic in recalculateZoom uses different math to
the math used to calculate the camera's altitude from the
current zoom. This different math may be close to correct, but,
it's impossible for me to debug. In this commit I've simply
replaced it with the inverse of the math used to calcualte the
camera's altitude from its zoom - ie:
if a zoom Z gives a camera altitude f(Z)

Then, to get the camera to required altitude A,
we need to set the zoom to Z = inverse-of-f(A)

* Tests for fix for #2094 (unwanted zoom changes)

Added asserts that camera altitude remains invariant throughout the
recalculateZoom test. These new asserts fail before the fix for 2094.

Moved the asserts that the zoom is a particular value to after the
altitude asserts - a particular zoom is correct if and only if it
results in the correct altitude, so whether the altitude assert fails
is more important information to have in a failing test.

Updated CHANGELOG.md

---------

Co-authored-by: Harel M <[email protected]>
@HarelM
Copy link
Collaborator

HarelM commented Mar 15, 2024

@olsen232 if you want the bounty, please follow the relevant procedure.

@olsen232
Copy link
Contributor

I've never collected a bounty before so I'm gonna do it
https://opencollective.com/maplibre/expenses/193319

@HarelM
Copy link
Collaborator

HarelM commented Mar 21, 2024

While the main issue here is solved, there's still issues that I think are somewhat related I believe
I've opened the following issues:

@olsen232 if you happen to want to test your fixes with the above issue let me know, I tend to think it's somewhat related to what you are working on.

@wipfli
Copy link
Contributor

wipfli commented Mar 24, 2024

Thanks for fixing this issue @olsen232 and for submitting the expense https://opencollective.com/maplibre/expenses/193319 on opencollective.

Can you fill out the developer application form on https://maplibre.org/jobs/ ?

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💰 bounty S Small Bounty, USD 100 bug Something isn't working PR is more than welcomed Extra attention is needed terrain
Projects
None yet
9 participants