Snap to integer zoom level #3532

quinncnl · 2024-01-02T18:44:08Z

Add an option to snap to integer zoom level: #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

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.

HarelM · 2024-01-02T20:37:15Z

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 · 2024-01-02T22:30:56Z

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

codecov-commenter · 2024-01-07T19:56:53Z

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 ⚠️
src/ui/map.ts	52.94%	8 Missing ⚠️
src/ui/handler/box_zoom.ts	0.00%	5 Missing ⚠️
src/ui/handler/click_zoom.ts	0.00%	0 Missing and 2 partials ⚠️
src/ui/handler/tap_zoom.ts	75.00%	1 Missing and 1 partial ⚠️
src/ui/camera.ts	88.88%	0 Missing and 1 partial ⚠️

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

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

quinncnl · 2024-01-07T19:59:24Z

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 · 2024-01-08T22:05:01Z

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 · 2024-01-12T12:57:17Z

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

HarelM · 2024-01-12T16:28:27Z

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 · 2024-03-08T14:42:54Z

Any updates on this?

quinncnl · 2024-03-08T15:57:12Z

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 · 2024-03-10T20:45:30Z

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.

src/ui/map.ts

src/ui/handler/tap_zoom.ts

HarelM · 2024-03-11T06:39:31Z

I think this direction is OK.

sbachinin · 2024-03-12T05:36:14Z

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.

src/ui/map.ts

src/ui/camera.ts

test/build/min.test.ts

HarelM · 2024-03-17T20:01:40Z

src/ui/map.ts

+        if (options === null || options === undefined) {
+            this._snapToIntegerZoomOptions = {boxZoom: false, clickZoom: false, scrollZoom: false, tapZoom: false};
+        } else {
+            this._snapToIntegerZoomOptions = options;


I think this will reset other snap options, wouldn't it? i.e. if I set scroll to true in the first call and set box to true in the second call, I'll reset scroll configuration.
Consider doing a for loop here maybe? Not sure if it's a great idea though...

Or maybe consider the API to be similar to the get/should

I see your point. I think replacing the entire options is more logical here because we are setting SnapToIntegerZoomOptions instead of type: SnapToIntegerZoomType. It's also same like other setters like style: StyleSpecification.

Or maybe consider the API to be similar to the get/should

Something like setSnapToIntegerZoom(type: SnapToIntegerZoomType, value: boolean)? Normally this options is set once at the map init instead of fine tune later. Using this kind of setter is not very convenient in this common case. As you would find codes like:

setSnapToIntegerZoom("boxScroll", true); setSnapToIntegerZoom("clickScroll", true);

instead of simple setOptions({boxScrolll: true, clickScroll: true})

HarelM · 2024-03-17T20:09:31Z

src/ui/handler/scroll_zoom.ts

+
+            zoomTarget = Math.round(zoomTarget);
+
+            this._map.easeTo({


It seems like there is a explicit animation code for regular zoom, how come this can be avoided using easeTo instead of writing the animation code (using frames etc)?

The explicit animation is about smoothOutEasing which doesn't have a targetZoom. The end zoom is based on an easing function which is hard to ease to an integer level. So I just skip that and use a simple easeTo instead.

HarelM · 2024-03-17T20:11:48Z

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

quinncnl · 2024-03-18T20:06:20Z

@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 · 2024-03-19T02:01:05Z

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:

1234.mp4

quinncnl · 2024-03-19T12:50:07Z

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

quinncnl · 2024-04-02T19:40:36Z

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 · 2024-04-03T05:51:32Z

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

sbachinin · 2024-04-03T08:07:45Z

@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 · 2024-04-03T17:39:09Z

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.

init

db23f92

quinncnl marked this pull request as draft January 2, 2024 18:44

quinncnl changed the title ~~init~~ Snap to integer zoom level Jan 2, 2024

update

994a807

update scroll zoom handler

b7d185d

HarelM added the waiting for code review fixes label Mar 8, 2024

quinncnl added 2 commits March 10, 2024 17:23

Merge branch 'maplibre:main' into integerZoom

c2919c9

use fine-grained zoom options and improve scroll zoom

8f6165c

HarelM reviewed Mar 11, 2024

View reviewed changes

src/ui/map.ts Outdated Show resolved Hide resolved

HarelM reviewed Mar 11, 2024

View reviewed changes

src/ui/map.ts Outdated Show resolved Hide resolved

HarelM reviewed Mar 11, 2024

View reviewed changes

src/ui/handler/tap_zoom.ts Outdated Show resolved Hide resolved

HarelM reviewed Mar 11, 2024

View reviewed changes

src/ui/handler/tap_zoom.ts Outdated Show resolved Hide resolved

quinncnl added 2 commits March 16, 2024 21:00

Merge branch 'main' into integerZoom

82dc983

review fix

3ea6356

HarelM reviewed Mar 16, 2024

View reviewed changes

src/ui/map.ts Outdated Show resolved Hide resolved

HarelM reviewed Mar 16, 2024

View reviewed changes

src/ui/map.ts Outdated Show resolved Hide resolved

quinncnl added 2 commits March 16, 2024 22:13

update docs and fix tests

c84b801

review fix

d76e212

quinncnl commented Mar 16, 2024

View reviewed changes

src/ui/camera.ts Show resolved Hide resolved

HarelM reviewed Mar 16, 2024

View reviewed changes

test/build/min.test.ts Outdated Show resolved Hide resolved

review fix

3404715

quinncnl marked this pull request as ready for review March 17, 2024 19:24

add docs and fix build

d98eae8

HarelM reviewed Mar 17, 2024

View reviewed changes

fix trackpad scroll issue

4c47772

fix trackpad scroll issue

6af6253

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Snap to integer zoom level #3532

Snap to integer zoom level #3532

quinncnl commented Jan 2, 2024 •

edited

HarelM commented Jan 2, 2024 •

edited

quinncnl commented Jan 2, 2024

codecov-commenter commented Jan 7, 2024 •

edited

quinncnl commented Jan 7, 2024

HarelM commented Jan 8, 2024

quinncnl commented Jan 12, 2024

HarelM commented Jan 12, 2024

HarelM commented Mar 8, 2024

quinncnl commented Mar 8, 2024

quinncnl commented Mar 10, 2024

HarelM commented Mar 11, 2024

sbachinin commented Mar 12, 2024 •

edited

HarelM Mar 17, 2024

HarelM Mar 17, 2024

quinncnl Mar 18, 2024 •

edited

HarelM Mar 17, 2024

quinncnl Mar 18, 2024

HarelM commented Mar 17, 2024

quinncnl commented Mar 18, 2024

sbachinin commented Mar 19, 2024

quinncnl commented Mar 19, 2024

quinncnl commented Apr 2, 2024

HarelM commented Apr 3, 2024

sbachinin commented Apr 3, 2024

quinncnl commented Apr 3, 2024

Snap to integer zoom level #3532

Are you sure you want to change the base?

Snap to integer zoom level #3532

Conversation

quinncnl commented Jan 2, 2024 • edited

Launch Checklist

HarelM commented Jan 2, 2024 • edited

quinncnl commented Jan 2, 2024

codecov-commenter commented Jan 7, 2024 • edited

Codecov Report

quinncnl commented Jan 7, 2024

HarelM commented Jan 8, 2024

quinncnl commented Jan 12, 2024

HarelM commented Jan 12, 2024

HarelM commented Mar 8, 2024

quinncnl commented Mar 8, 2024

quinncnl commented Mar 10, 2024

HarelM commented Mar 11, 2024

sbachinin commented Mar 12, 2024 • edited

HarelM Mar 17, 2024

Choose a reason for hiding this comment

HarelM Mar 17, 2024

Choose a reason for hiding this comment

quinncnl Mar 18, 2024 • edited

Choose a reason for hiding this comment

HarelM Mar 17, 2024

Choose a reason for hiding this comment

quinncnl Mar 18, 2024

Choose a reason for hiding this comment

HarelM commented Mar 17, 2024

quinncnl commented Mar 18, 2024

sbachinin commented Mar 19, 2024

quinncnl commented Mar 19, 2024

quinncnl commented Apr 2, 2024

HarelM commented Apr 3, 2024

sbachinin commented Apr 3, 2024

quinncnl commented Apr 3, 2024

quinncnl commented Jan 2, 2024 •

edited

HarelM commented Jan 2, 2024 •

edited

codecov-commenter commented Jan 7, 2024 •

edited

sbachinin commented Mar 12, 2024 •

edited

quinncnl Mar 18, 2024 •

edited