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

Fix camera jumps with terrain and transformCameraUpdate. #4299

Merged

Conversation

chrneumann
Copy link
Contributor

@chrneumann chrneumann commented Jun 19, 2024

Fixes camera jumps at end of movement when terrain and transformCameraUpdate is enabled (fixes #4233).

Had to enable WebGL for headless Chrome, as I could not reproduce the bug without it in headless mode.
The problem was caused by directly accessing the map's transform, instead of using _getTransformForUpdate and _applyUpdatedTransform.

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.

@codecov-commenter
Copy link

codecov-commenter commented Jul 10, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 88.12%. Comparing base (b5a3159) to head (9a7eb89).
Report is 5 commits behind head on main.

Files Patch % Lines
src/ui/handler_manager.ts 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4299      +/-   ##
==========================================
+ Coverage   87.87%   88.12%   +0.25%     
==========================================
  Files         246      246              
  Lines       33372    33375       +3     
  Branches     2194     2189       -5     
==========================================
+ Hits        29325    29413      +88     
+ Misses       3050     2969      -81     
+ Partials      997      993       -4     

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

@chrneumann chrneumann changed the title Add test for camera jumps with terrain and transformCameraUpdate. Fix camera jumps with terrain and transformCameraUpdate. Jul 10, 2024
@chrneumann
Copy link
Contributor Author

The last commit should fix it. Thanks to @UberMouse for the hint at #4377 (comment)

@HarelM
Copy link
Member

HarelM commented Jul 10, 2024

Thanks for taking the time to fix this issue!
This looks like the right solution to the problem.
See my small comments above.

@HarelM
Copy link
Member

HarelM commented Jul 10, 2024

I think the following unit test can reproduce this issue to some extent - as the recalculate zoom will be done on the original transform and won't take the zoom changes that are done as part of the transfrom camera update:

    test('recalculate zoom is done on the camera update transform', () => {
        const map = createMap({
            interactive: true, 
            clickTolerance: 4,
            transformCameraUpdate: ({zoom}) => ({ zoom: zoom + 0.1})
        });
        map.terrain = {
            pointCoordinate: () => null,
            getElevationForLngLatZoom: () => 1000
        } as any;
        const canvas = map.getCanvas();
        simulate.dragWithMove(canvas, {x: 100, y: 100}, {x: 100, y: 150});
        map._renderTaskQueue.run();

        expect(map.getZoom()).toBe(0.20008255564976615);
    });

What do you think?

@chrneumann
Copy link
Contributor Author

Seems fine, still I think an integration test that involves transformCameraUpdate wouldn't be bad. But fine for me :)

@HarelM
Copy link
Member

HarelM commented Jul 11, 2024

I'm good with both tests, just please simplify the style in the integration test...

@chrneumann
Copy link
Contributor Author

I'm good with both tests, just please simplify the style in the integration test...

Ah, I did that on purpose. Thought about simplifying it, which would be fine for the automatic testing, but if you want to observe the bug, the more complete style is really helpful. But yeah, I can simplify it if you don't think thats important.

@HarelM
Copy link
Member

HarelM commented Jul 11, 2024

Yeah, the test should be as simple as possible, for debugging you can change the examples in order to reproduce an issue.
Please add the test I sent and simplify the integration test and I'll merge this PR.
Thanks!!

@chrneumann
Copy link
Contributor Author

I think the following unit test can reproduce this issue to some extent - as the recalculate zoom will be done on the original transform and won't take the zoom changes that are done as part of the transfrom camera update:
What do you think?

Ah, you did not actually test it? At least, it never fails for me.

@HarelM
Copy link
Member

HarelM commented Jul 12, 2024

I've test it with and without the fix and it gave different results.

@chrneumann
Copy link
Contributor Author

Maybe I'm missing something. I pushed your unit test, you can check it with the commit "04278f129 | Fix terrain jumps with transformCameraUpdate (#4233)" reversed, which lets the integration test fail for me but not the unit test.

@chrneumann
Copy link
Contributor Author

Ah, it fails when the fix is applied? Did you calculate the zoom by hand? Or is it fine to just fix the expected zoom value with the fix applied?

@HarelM
Copy link
Member

HarelM commented Jul 12, 2024

Must have written the test when I removed the fix, sorry...

@HarelM HarelM marked this pull request as ready for review July 12, 2024 10:24
@HarelM HarelM enabled auto-merge (squash) July 12, 2024 10:24
@HarelM HarelM merged commit eff44db into maplibre:main Jul 12, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

transformCameraUpdate leads to unexpected camera jumps when targetting terrain
4 participants