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

When simulating a GPS bearing the map zooms in/out with terrain #3878

Closed
HarelM opened this issue Mar 21, 2024 · 6 comments · Fixed by #3944
Closed

When simulating a GPS bearing the map zooms in/out with terrain #3878

HarelM opened this issue Mar 21, 2024 · 6 comments · Fixed by #3944
Labels
💰 bounty M Medium Bounty, USD 250 bug Something isn't working PR is more than welcomed Extra attention is needed Terrain 3D

Comments

@HarelM
Copy link
Member

HarelM commented Mar 21, 2024

maplibre-gl-js version: 4.1.1
browser: chrome

Steps to Trigger Behavior

See below.

  1. Set terrain
  2. Move the bearing slowly back and forth

Link to Demonstration

https://jsbin.com/citaminibe/2/edit?html,js,output

Expected Behavior

Like in 2D - no movement

Actual Behavior

Depend on the terrain, this will cause a slow zoom in or zoom out.

@HarelM
Copy link
Member Author

HarelM commented Mar 21, 2024

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

@HarelM HarelM added the 💰 bounty M Medium Bounty, USD 250 label Mar 21, 2024
olsen232 added a commit to olsen232/maplibre-gl-js that referenced this issue Apr 4, 2024
Don't _finalizeElevation if freezeElevation has not been set.

This means that elevation logic has two distinct operating modes.

Mode one - freezeElevation is false - default:
- updateElevation is called continuously during the movement
- finalizeElevation is *not* called at the end.

Mode two - freezeElevation is true:
- updateElevation is *not* called continuoulsy during the movement
- finalizeElevation is called at the end.

Previously, both methods were called in mode one, which lead to
issue maplibre#3878 if freezeElevation was not set.
@olsen232
Copy link
Contributor

olsen232 commented Apr 5, 2024

You could argue that this is "working as intended" but certainly it is a bit indicative on the code here having gotten away from us over time, probably due to 3d terrain being added onto a system that wasn't originally designed for it (nobody's fault).

Simplest fix:
When requesting the easeTo, be sure to set freezeElevation to true.
See https://github.com/maplibre/maplibre-gl-js/blob/main/src/ui/camera.ts#L222
From the documentation:

Default false. Needed in 3D maps to let the camera stay in a constant height based on sea-level.

Simplest fix is applied here:
https://jsbin.com/wanucazeve/edit?html,js,output

So, that's why you could argue that this is okay - the documentation says this boolean should be set in a 3D map, and this is a 3D map, and if you set the boolean, things work okay. The documentation doesn't say what will happen if you forget to set the boolean, but it turns out, that this bug is what happens.

However, things could certainly be improved for the case where you forget to set freezeElevation.
Here is one option: #3944
Description from that PR:

This means that elevation logic has two distinct operating modes.
Mode one - freezeElevation is false - default:

  • updateElevation is called continuously during the movement
  • finalizeElevation is not called at the end.

Mode two - freezeElevation is true:

  • updateElevation is not called continuously during the movement
  • finalizeElevation is called at the end.

Previously, both methods were called in mode one, which lead to (this) issue.

However, note that this is not fully tested - you would have to try out a variety of different easeTo movements, both with freezeElevation set and unset, to make sure there were no regressions.

Another potential fix would be not to allow the client to supply freezeElevation, but have easeTo decide whether or not it is required in this case.

I don't really understand this code enough to fully understand why calling both updateElevation and finalizeElevation leads to a drift in the zoom over time. But, here's my high level understanding:

  • Ordinarily when moving the map (or easing to a new bearing), the zoom should remain constant.
  • However, when 3D terrain is active, the apparent altitude of the camera should remain constant. Since the scale that the terrain is drawn at and the offset it is drawn at are both functions of f(zoom, terrain-elevation), this means that zoom needs to be varied to compensate for a potentially varying terrain-elevation.
  • However however, this compensation step only happens once at the end of the movement. During the movement, there are other invariants, but I don't yet understand what they are.
  • Forgetting to set freezeElevation causes a mix of logic from different cases to happen, which caused some of the supposed invariants to drift.
  • Continously updating the bearing shouldn't really cause these kind of issues since the terrain elevation should remain constant throughout. However, finding the terrain elevation depends on code that finds the screen's current centerpoint, which uses the coords buffer - this is surely subject to rounding errors.
    (In practise if you output the elevation of the camera's centrepoint each time it changes bearing, you'll see that it's fluctuating continuously around 1160 +/- 10 metres. If this could be improved, this would also mitigate this kind of issue.)

@olsen232
Copy link
Contributor

olsen232 commented Apr 5, 2024

PS If anyone wants to rewrite the transform code to make it more like a first-person-shooter - the world has some fixed co-ordinate system, its size never changes, and the camera's position is described (at the lowest level) in terms of its position within this fixed world - I'm all for it, and it would get rid of this class of bugs where the different types of invariants get mixed up. However, I understand this would be a huge breaking change and I am not volunteering.

@HarelM
Copy link
Member Author

HarelM commented Apr 7, 2024

There's also the maps' level of details which is zoom defined by the spec, which also complicated everything further.
I tend to say that freezeElevation should be "on" when using terrain much like the freeze elevation that happens when panning, but allow the user to change this of needed.
But I think it's a breaking change, so changing that behavior might need to wait for version 5, maybe, IDK.
@prozessor13 do you remember why you added this flag?

@olsen232
Copy link
Contributor

olsen232 commented May 2, 2024

FYI I roughly diagnosed this issue but I don't have any strong feelings about what happens next - I could merge the linked PR #3944 if useful

@HarelM
Copy link
Member Author

HarelM commented May 3, 2024

Let's push the PR forward, thanks!

olsen232 added a commit to olsen232/maplibre-gl-js that referenced this issue May 6, 2024
Don't _finalizeElevation if freezeElevation has not been set.

This means that elevation logic has two distinct operating modes.

Mode one - freezeElevation is false - default:
- updateElevation is called continuously during the movement
- finalizeElevation is *not* called at the end.

Mode two - freezeElevation is true:
- updateElevation is *not* called continuoulsy during the movement
- finalizeElevation is called at the end.

Previously, both methods were called in mode one, which lead to
issue maplibre#3878 if freezeElevation was not set.
olsen232 added a commit to olsen232/maplibre-gl-js that referenced this issue May 7, 2024
olsen232 added a commit to olsen232/maplibre-gl-js that referenced this issue May 16, 2024
HarelM added a commit that referenced this issue May 16, 2024
* Fix for #3878

Don't _finalizeElevation if freezeElevation has not been set.

This means that elevation logic has two distinct operating modes.

Mode one - freezeElevation is false - default:
- updateElevation is called continuously during the movement
- finalizeElevation is *not* called at the end.

Mode two - freezeElevation is true:
- updateElevation is *not* called continuoulsy during the movement
- finalizeElevation is called at the end.

Previously, both methods were called in mode one, which lead to
issue #3878 if freezeElevation was not set.

* Fix for #3878 - address review comments (done -> async)

* Update src/ui/camera.test.ts

* Fix for #3878 - address review comments 2

---------

Co-authored-by: Harel M <harel.mazor@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💰 bounty M Medium Bounty, USD 250 bug Something isn't working PR is more than welcomed Extra attention is needed Terrain 3D
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants