Constrain camera to maximum bounds #2475

christian-boks · 2024-06-05T07:14:54Z

Having worked with maplibre-gl-js for a web solution I wanted to try maplibre-native for ios, but I couldn't find the functionality where you can set maximum bounds for the camera.

I can see that the issue has been discussed before in #1455 but it ended with some bikeshedding on what to call the missing method, and it was removed again. I added the missing code to the ios platform code, but found out that the current constraining code doesn't work the same way as the web code. It constrains the bounds in the center of the screen?

I've added the code required to get the same functionality as the web does, where the bounds are constrained to the edges of the screen. I'd appreciate a review from someone with a bit more experience in the codebase.

Merge changes from origin

louwers

Thanks for your contribution!

This is so much better than the naive implementation with a delegate. There all movement is blocked once you touch the border: slightly panning into the border, zooming out... It's bad.

Did you port this directly from MapLibre GL JS? In that case can you link to the source code for reference?

Not sure how easy it is to add test coverage for this change?

We would need an Android API as well for this, do you by any chance also work with Android? Otherwise I'll create an issue for it.

louwers · 2024-06-06T09:36:54Z

Some compile errors on Linux

/home/runner/work/maplibre-native/maplibre-native/src/mbgl/map/transform_state.cpp:767:68: error: declaration shadows a field of 'mbgl::TransformState' [clang-diagnostic-shadow,-warnings-as-errors]
void TransformState::constrainCameraAndZoomToBounds(CameraOptions& camera, double& zoom) const {
                                                                   ^
/home/runner/work/maplibre-native/maplibre-native/src/mbgl/map/transform_state.hpp:290:26: note: previous declaration is here
    mutable util::Camera camera;
                         ^
/home/runner/work/maplibre-native/maplibre-native/src/mbgl/map/transform_state.cpp:779:12: error: Value stored to 'currentScale' during its initialization is never read [clang-analyzer-deadcode.DeadStores,-warnings-as-errors]
    double currentScale = getScale();
           ^~~~~~~~~~~~   ~~~~~~~~~~
/home/runner/work/maplibre-native/maplibre-native/src/mbgl/map/transform_state.cpp:779:12: note: Value stored to 'currentScale' during its initialization is never read
    double currentScale = getScale();
           ^~~~~~~~~~~~   ~~~~~~~~~~
/home/runner/work/maplibre-native/maplibre-native/src/mbgl/map/transform_state.cpp:779:12: error: unused variable 'currentScale' [clang-diagnostic-unused-variable,-warnings-as-errors]
    double currentScale = getScale();
           ^
/home/runner/work/maplibre-native/maplibre-native/src/mbgl/map/transform_state.cpp:801:24: error: declaration shadows a field of 'mbgl::TransformState' [clang-diagnostic-shadow,-warnings-as-errors]
    mbgl::LatLngBounds bounds = getLatLngBounds();
                       ^
/home/runner/work/maplibre-native/maplibre-native/src/mbgl/map/transform_state.hpp:234:18: note: previous declaration is here
    LatLngBounds bounds;

github-actions · 2024-06-06T09:40:26Z

Bloaty Results (iOS) 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.0% +1.09Ki  [ = ]       0    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-2475-compared-to-main.txt

christian-boks · 2024-06-06T10:35:57Z

Thanks for your contribution!

This is so much better than the naive implementation with a delegate. There all movement is blocked once you touch the border: slightly panning into the border, zooming out... It's bad.

Yes, I tried that first too and was a bit disappointed with the results.

Did you port this directly from MapLibre GL JS? In that case can you link to the source code for reference?

I hoped I would be able to just port it over, but it turned out that the architecture of the two code bases differ in important ways. I had to add the anchor handling and the scaling of the resulting center in case of having to scale the requested zoom.
https://github.com/maplibre/maplibre-gl-js/blob/main/src/geo/transform.ts#L727

Not sure how easy it is to add test coverage for this change?

I'll see if I can figure out where to add a test or two.

We would need an Android API as well for this, do you by any chance also work with Android? Otherwise I'll create an issue for it.

I was under the impression from #1455 that the android platform already had functionality to set the bounds. #1455 (comment)

louwers

I was under the impression from #1455 that the android platform already had functionality to set the bounds.

The current implementation constrains the center of the screen (as you expected). I think being able to limit the visible area regardless of screen size is a useful feature. But I am not sure yet how the constrainCameraAndZoomToBounds, constrain, setLatLngBounds should interact with each other and what the best API design would be. I think the methods should make clear you are constraining either the visible area or where the center can be.

The contribution from @JulianBissekkou was reverted before the 6.0.0 release because there was a typo in the API. 🙈 I am not against including it again (with another name).

video_2024-06-06_15-04-24.mp4

Here you see the example app of Android on main (LatLngBoundsForCameraActivity), but the PR breaks the above behavior. We need to make this change in a backwards-compatible manner.

Sorry I stumbled, the approval was a misclick.

christian-boks · 2024-06-06T14:22:13Z

I was under the impression from #1455 that the android platform already had functionality to set the bounds.

The current implementation constrains the center of the screen (as you expected). I think being able to limit the visible area regardless of screen size is a useful feature. But I am not sure yet how the constrainCameraAndZoomToBounds, constrain, setLatLngBounds should interact with each other and what the best API design would be. I think the methods should make clear you are constraining either the visible area or where the center can be.

The contribution from @JulianBissekkou was reverted before the 6.0.0 release because there was a typo in the API. 🙈 I am not against including it again (with another name).

video_2024-06-06_15-04-24.mp4
Here you see the example app of Android on main (LatLngBoundsForCameraActivity), but the PR breaks the above behavior. We need to make this change in a backwards-compatible manner.

Sorry I stumbled, the approval was a misclick.

I did find the ConstrainMode Enum here:
https://github.com/maplibre/maplibre-native/blob/master/include/mbgl/map/mode.hpp#L24

enum class ConstrainMode : EnumType {
    None,
    HeightOnly,
    WidthAndHeight,
};

Unfortunately it looks like if you set the bounds it assumes that you want to constrain the center of the screen to the bounds, it would have been nice if there were a CenterToBounds in there too. But I guess if I add one called CameraAndZoomToBounds and then only call constrainCameraAndZoomToBounds if the constrainMode is set to CameraAndZoomToBounds would be the most elegant backwards-compatible approach?

…atible.

louwers · 2024-06-10T22:35:24Z

Actually I think those enums constrain transformations in the way you want, because it is used to make sure you cannot pan off the world.

    // Constrain scale to avoid zooming out far enough to show off-world areas on the Y axis.
    const double ratioY = (rotatedNorth() ? size.width : size.height) / util::tileSize_D;
    scale_ = util::max(scale_, ratioY);

    // Constrain min/max pan to avoid showing off-world areas on the Y axis.
    double max_y = (scale_ * util::tileSize_D - (rotatedNorth() ? size.width : size.height)) / 2;
    y_ = std::max(-max_y, std::min(y_, max_y));

    if (constrainMode == ConstrainMode::WidthAndHeight) {
        // Constrain min/max pan to avoid showing off-world areas on the X axis.
        double max_x = (scale_ * util::tileSize_D - (rotatedNorth() ? size.height : size.width)) / 2;
        x_ = std::max(-max_x, std::min(x_, max_x));
    }

If you just make max_x configurable here you'd be done.

Note that in transform.test.cpp there are tests which need to be extended.

christian-boks · 2024-06-11T06:45:46Z

Actually I think those enums constrain transformations in the way you want, because it is used to make sure you cannot pan off the world.

I did contemplate solving the problem like that, but came to the conclusion that it would be more elegant to actually compute a valid end point and zoom, instead of just start constraining movement mid path. Especially with flyTo where the start point, start zoom, end point, and end zoom are used to calculate the path taken.
It would look odd when movement just stops because it is being constrained, and then starts zooming in because it thinks it has reached the end point.
Computing a valid end point and zoom level is the solution that gives the best results.

louwers · 2024-06-11T07:52:17Z

Agreed!

for more information, see https://pre-commit.ci

christian-boks · 2024-06-14T06:29:02Z

@louwers I've added the requested unit tests and made sure that the change is backwards-compatible.

louwers

Starting to look good! I have some questions:

How can the user of the iOS SDK reset the max bounds again?
In BoundOptions there is a comment

Sets the latitude and longitude bounds to which the camera center are constrained

Constrain the center of the camera to be within these bounds.

Do these need to be updated?

Do you think ConstrainMode::CameraAndZoomToBounds is a good name? I feel like saying you are constraining zoom to bounds is a bit awkward. Maybe ConstrainMode::Screen would be better?
Reflecting the above, maybe we want setMaximumScreenBounds in case we want to add an Objective-C method to set the camera center bounds.

for more information, see https://pre-commit.ci

…ents.

…e event.

for more information, see https://pre-commit.ci

christian-boks · 2024-07-02T17:56:46Z

Starting to look good! I have some questions:

How can the user of the iOS SDK reset the max bounds again?

Currently you can't. This isn't part of my usecase. If someone needs it, i'm sure they are welcome to create a PR with that functionality.

In BoundOptions there is a comment

Sets the latitude and longitude bounds to which the camera center are constrained

Constrain the center of the camera to be within these bounds.

Do these need to be updated?

Do you think ConstrainMode::CameraAndZoomToBounds is a good name? I feel like saying you are constraining zoom to bounds is a bit awkward. Maybe ConstrainMode::Screen would be better?

Reflecting the above, maybe we want setMaximumScreenBounds in case we want to add an Objective-C method to set the camera center bounds.

All these should be fixed.

I hope I've added all the missing parts, it now constrains the screen to the bounds when you change orientation on your phone too. There were some very subtle bugs hidden in the resize method. I've made the changes backwards compatible if anyone are counting on the resize method behaving in a certain way. Please have a look and let me know what you think.
I'll try and add some tests when you give the changes a👍

github-actions · 2024-07-11T16:41:37Z

Bloaty Results 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.0% +16.2Ki  +0.0% +7.21Ki    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2475-compared-to-main.txt

Compared to d387090 (legacy)

    FILE SIZE        VM SIZE    
 --------------  -------------- 
   +30% +34.6Mi  +433% +25.8Mi    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2475-compared-to-legacy.txt

github-actions · 2024-07-11T16:43:59Z

Benchmark Results ⚡

Benchmark                                                     Time             CPU      Time Old      Time New       CPU Old       CPU New
------------------------------------------------------------------------------------------------------------------------------------------
OVERALL_GEOMEAN                                            -0.0309         -0.0309             0             0             0             0

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/benchmark-results/pr-2475-compared-to-main.txt

christian-boks · 2024-08-17T08:49:40Z

@louwers have you had a chance to look at the changes?

louwers · 2024-09-19T21:56:23Z

@christian-boks Will give it another spin tomorrow, thanks for the reminder.

louwers · 2024-11-14T01:59:27Z

@JesseCrocker Do you have a use case for this? If so, would you be willing to try it out?

for more information, see https://pre-commit.ci

christian-boks and others added 6 commits June 4, 2024 21:07

Merge pull request #1 from maplibre/main

d86c823

Merge changes from origin

Changes required to constrain the camera to the specified bounds.

5b16943

Merge branch 'main' of https://github.com/christian-boks/maplibre-native

0d6a54a

clang-format changes

c9d83a7

Add example to Swift App

3ca26ff

Fix typo

9d0e4a5

louwers requested changes Jun 6, 2024

View reviewed changes

louwers approved these changes Jun 6, 2024

View reviewed changes

christian-boks added 2 commits June 6, 2024 12:08

Fix clang compile errors

b59ec39

Merge branch 'main' of https://github.com/christian-boks/maplibre-native

aeaefc5

louwers requested changes Jun 6, 2024

View reviewed changes

Added new ConstrainMode enum entry to make the changes backwards-comp…

87d8940

…atible.

christian-boks requested a review from louwers June 10, 2024 16:49

christian-boks and others added 4 commits June 11, 2024 17:45

Added tests and fixed issue

3582ede

[pre-commit.ci] auto fixes from pre-commit.com hooks

272183e

for more information, see https://pre-commit.ci

Merge branch 'main' into main

ad9d0dd

[pre-commit.ci] auto fixes from pre-commit.com hooks

c6f3dd2

for more information, see https://pre-commit.ci

louwers requested changes Jun 14, 2024

View reviewed changes

christian-boks and others added 4 commits June 21, 2024 20:19

Renamed new ConstrainMode to Screen

c7f8c7a

Merge branch 'main' into main

42ee8a8

[pre-commit.ci] auto fixes from pre-commit.com hooks

13ce06d

for more information, see https://pre-commit.ci

Renamed MaximumBounds to MaximumScreenBounds and added some more comm…

875e40d

…ents.

Merge branch 'main' of https://github.com/christian-boks/maplibre-native

40c65b0

louwers mentioned this pull request Jun 21, 2024

Android Documentation: Constrain Center of Screen #2533

Open

christian-boks and others added 3 commits July 2, 2024 19:34

Added code to handle constraining screen to bounds because of a resiz…

6d45620

…e event.

Merge branch 'main' into main

fc29ce5

[pre-commit.ci] auto fixes from pre-commit.com hooks

c9f3a91

for more information, see https://pre-commit.ci

louwers added enhancement New feature or request iOS labels Jul 11, 2024

Merge branch 'main' into main

c31bb14

louwers self-requested a review July 12, 2024 20:05

Merge branch 'main' into main

7ff1380

christian-boks and others added 2 commits September 19, 2024 20:05

Merge branch 'main' into main

186d740

Merge branch 'main' into main

c7350cb

Merge branch 'main' into main

4e6006e

louwers requested a review from JesseCrocker November 14, 2024 01:58

christian-boks and others added 3 commits November 15, 2024 20:45

Merge branch 'main' into main

65be132

Added test of the transform::resize method

83b708a

[pre-commit.ci] auto fixes from pre-commit.com hooks

8c24d78

for more information, see https://pre-commit.ci

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Constrain camera to maximum bounds #2475

Constrain camera to maximum bounds #2475

christian-boks commented Jun 5, 2024

louwers left a comment •

edited

Loading

louwers commented Jun 6, 2024

github-actions bot commented Jun 6, 2024 •

edited

Loading

christian-boks commented Jun 6, 2024

louwers left a comment •

edited

Loading

christian-boks commented Jun 6, 2024

louwers commented Jun 10, 2024 •

edited

Loading

christian-boks commented Jun 11, 2024

louwers commented Jun 11, 2024

christian-boks commented Jun 14, 2024

louwers left a comment •

edited

Loading

christian-boks commented Jul 2, 2024

github-actions bot commented Jul 11, 2024 •

edited

Loading

github-actions bot commented Jul 11, 2024 •

edited

Loading

christian-boks commented Aug 17, 2024

louwers commented Sep 19, 2024

louwers commented Nov 14, 2024

Constrain camera to maximum bounds #2475

Are you sure you want to change the base?

Constrain camera to maximum bounds #2475

Conversation

christian-boks commented Jun 5, 2024

louwers left a comment • edited Loading

Choose a reason for hiding this comment

louwers commented Jun 6, 2024

github-actions bot commented Jun 6, 2024 • edited Loading

Bloaty Results (iOS) 🐋

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-2475-compared-to-main.txt

christian-boks commented Jun 6, 2024

louwers left a comment • edited Loading

Choose a reason for hiding this comment

christian-boks commented Jun 6, 2024

louwers commented Jun 10, 2024 • edited Loading

christian-boks commented Jun 11, 2024

louwers commented Jun 11, 2024

christian-boks commented Jun 14, 2024

louwers left a comment • edited Loading

Choose a reason for hiding this comment

christian-boks commented Jul 2, 2024

github-actions bot commented Jul 11, 2024 • edited Loading

Bloaty Results 🐋

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2475-compared-to-main.txt

github-actions bot commented Jul 11, 2024 • edited Loading

christian-boks commented Aug 17, 2024

louwers commented Sep 19, 2024

louwers commented Nov 14, 2024

louwers left a comment •

edited

Loading

github-actions bot commented Jun 6, 2024 •

edited

Loading

louwers left a comment •

edited

Loading

louwers commented Jun 10, 2024 •

edited

Loading

louwers left a comment •

edited

Loading

github-actions bot commented Jul 11, 2024 •

edited

Loading

github-actions bot commented Jul 11, 2024 •

edited

Loading