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

Remove finalize elevation when updating elevation on the fly #3944

Merged
merged 6 commits into from
May 16, 2024

Conversation

olsen232
Copy link
Contributor

@olsen232 olsen232 commented 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 #3878 if freezeElevation was not set.

Fixes #3878

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

@olsen232 olsen232 marked this pull request as draft April 4, 2024 23:52
@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 86.76%. Comparing base (aaeaf58) to head (9428223).

Files Patch % Lines
src/ui/camera.ts 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3944      +/-   ##
==========================================
+ Coverage   86.49%   86.76%   +0.27%     
==========================================
  Files         242      242              
  Lines       33041    33041              
  Branches     2036     2012      -24     
==========================================
+ Hits        28578    28669      +91     
+ Misses       3481     3404      -77     
+ Partials      982      968      -14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@HarelM
Copy link
Collaborator

HarelM commented Apr 7, 2024

This fix makes sense to me regardless of what I wrote in the issue.

@HarelM
Copy link
Collaborator

HarelM commented May 3, 2024

Can you add a test and a changelog item?
Since elevation is constantly updated there's no need to finalize it, right?
I think this should get in.

Copy link
Collaborator

@prozessor13 prozessor13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think this change makes absolutely sense!

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 olsen232 marked this pull request as ready for review May 6, 2024 01:32
@olsen232
Copy link
Contributor Author

olsen232 commented May 6, 2024

Added a test, updated the changelog.

src/ui/camera.test.ts Outdated Show resolved Hide resolved
@HarelM HarelM changed the title Experimental fix for #3878 Remove finalize elevation when updating elevation on the fly May 6, 2024
@olsen232 olsen232 requested a review from HarelM May 7, 2024 22:42
src/ui/camera.test.ts Outdated Show resolved Hide resolved
src/ui/camera.test.ts Outdated Show resolved Hide resolved
src/ui/camera.test.ts Outdated Show resolved Hide resolved
@olsen232 olsen232 requested a review from HarelM May 16, 2024 06:08
@HarelM HarelM merged commit 3000ba1 into maplibre:main May 16, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When simulating a GPS bearing the map zooms in/out with terrain
4 participants