Constrain camera to maximum bounds

[christian-boks]

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.

[louwers]

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[bot]]

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]

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. https://github.com/maplibre/maplibre-native/pull/1455#issuecomment-1665460564

[christian-boks]

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?

[louwers]

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]

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]

Agreed!

[christian-boks]

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

[christian-boks]

Starting to look good! I have some questions:

1. 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.

2. 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?

3. 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?
4. 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[bot]]

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 d38709084a9865fe0bb8300aec70ebf8243b3d43 (legacy)

    FILE SIZE        VM SIZE    
 --------------  -------------- 
   +27% +31.8Mi  +424% +25.3Mi    TOTAL

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

[github-actions[bot]]

Benchmark Results ⚡

Benchmark                                                     Time             CPU      Time Old      Time New       CPU Old       CPU New
------------------------------------------------------------------------------------------------------------------------------------------
OVERALL_GEOMEAN                                            +0.0032         +0.0031             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]

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

[louwers]

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