Snap to integer zoom level

[quinncnl]

Add an option to snap to integer zoom level: https://github.com/maplibre/maplibre-gl-js/issues/1591

Example: https://tracestrack.s3-website.nl-ams.scw.cloud/dist/ Example: https://tracestrack.s3-website.nl-ams.scw.cloud/dist/#12.40/11.7933/53.4402 jsfiddle: https://jsfiddle.net/142rftnx/1/

Launch Checklist

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

[HarelM]

Thanks for taking the time to contribute! Quick question, What about touch gestures? Also note that there's a similar behavior to snap bearing to north, might be worth getting inspiration from, maybe...

[quinncnl]

Thanks for the note! I'll check those :)

[codecov-commenter]

Codecov Report

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

Project coverage is 86.46%. Comparing base (44b582e) to head (4c47772). Report is 51 commits behind head on main.

Files	Patch %	Lines
src/ui/handler/scroll_zoom.ts	0.00%	22 Missing and 1 partial :warning:
src/ui/map.ts	52.94%	8 Missing :warning:
src/ui/handler/box_zoom.ts	0.00%	5 Missing :warning:
src/ui/handler/click_zoom.ts	0.00%	0 Missing and 2 partials :warning:
src/ui/handler/tap_zoom.ts	75.00%	1 Missing and 1 partial :warning:
src/ui/camera.ts	88.88%	0 Missing and 1 partial :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3532      +/-   ##
==========================================
- Coverage   86.83%   86.46%   -0.37%     
==========================================
  Files         241      242       +1     
  Lines       32341    32530     +189     
  Branches     1962     2161     +199     
==========================================
+ Hits        28082    28128      +46     
- Misses       3340     3454     +114     
- Partials      919      948      +29     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[quinncnl]

I've done some changes to ScrollZoom. For TagDragZoom and TwoFingersTouchZoom it requires more investigation and probably a bigger change. Can we ignore the touch zoom for now?

[HarelM]

I don't think we can ignore the touch events without a flag that is specifying to which gestures the interger zoom applies I guess. Also I'm not sure the animation of the zoom scroll in the demo is working well... Can you add hash control to the demo so that zoom level will be easy to see?

[quinncnl]

OK will add hash control. For the touch event it'll take some time since it's quite complicated.

[HarelM]

What I meant to say is that if you decide not to implement touch handling the flag specifying the integer zoom should be something like: snapToIntegerZoom: {touch: false, scroll: true, controls: true} And not a simple boolean, this will on one hand allow more granularity and also provide a way to implement just the relevant part. When all are implemented, true and false can be added to indicate "apply to all" kind of configuration.

[HarelM]

Any updates on this?

[quinncnl]

Hey @HarelM sorry I got busy. Will try to followup on it this weekend. I'll break it into smaller pieces as you suggested.

[quinncnl]

Hi @HarelM I've updated the example map and added a jsfiddle. Could you please check it and provide feedback? Once it looks good I'll fix the tests and docs.

[HarelM]

I think this direction is OK.

[sbachinin]

Noticed that in your jsfiddle you can't zoom in. When I drag the mousewheel forward, it jumps to zoom 3.0 and you are stuck there.

[HarelM]

@quinncnl did you check/fix the issue reported by @sbachinin?

[quinncnl]

@quinncnl did you check/fix the issue reported by @sbachinin?

I can't seem to reproduce the scenario. @sbachinin could you try again? Maybe it was before my refator to scrollZoom.

[sbachinin]

This particular jsfiddle shows the same bad behaviour. It could be broken because of using some outdated bundle, I don't know.

Here's what I get when I try to zoom in:

https://github.com/maplibre/maplibre-gl-js/assets/11789697/c8fd5837-7b4a-4fcd-87ea-03a7c8e044c7

[quinncnl]

@sbachinin Thanks. I can reproduce it using touchpad scroll. Will check

[quinncnl]

Hi @sbachinin I updated the demo. Could you try again? It seems we have to round the zoom at the end of inertia animation. If this is not acceptable can we have a separate toggle for it, like trackpadScroll and do it later? @HarelM

[HarelM]

I would expect the last commit to be added with a test to make sure this is the expected behavior.

[sbachinin]

@quinncnl, all the demos given here still demonstrate zooming problems for me. (Chrome 122.0.6261.129) When I roll mousewheel forward: first several "clicks" are ignored, then I'm jumping to zoom 3. When I roll mousewheel backward: first several "clicks" are ignored, then begins a normal zoom out.

[quinncnl]

I tried a different computer on Windows and there it jumps zoom by 2 with single scroll. Looks like I need to test with different browser/OS/scroll device(trackpad/mouse) as they have different scroll values.

[quinncnl]

Hi @HarelM , do you think it's okay to move on without scroll_zoom? At least then people can build upon it.

[HarelM]

There are missing tests and reported errors. How do you suggest to address them?

[quinncnl]

There are missing tests and reported errors. How do you suggest to address them?

I will revert all changes related to scrollZoom as it's hard to fix with my knowledge. It does not pre-calculate a targetZoom making it hard to achieve integer final zoom.

I'll add some tests to the supported handlers.