Skip to content

Conversation

@andreas-umbricht
Copy link
Contributor

@andreas-umbricht andreas-umbricht commented Jun 20, 2025

Description

Adds the possibility to add a bounding box for the camera.

Test plan

Tested on Android and iOS. I observed different behavior on these platforms and I added them to the documentation of the composable.

Have you tested the changes? On which platforms?

  • Android: Samsung S21 Android 12, Pixel 8 Android 15
  • iOS: iPhone 16 Pro iOS 18.3

Coming from this discussion

Copy link
Collaborator

@sargunv sargunv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment on combining the API with zoomRange and pitchRange all into one.

Also, I can help with the nullability issue if needed

@westnordost
Copy link
Collaborator

westnordost commented Jun 21, 2025

See comment on combining the API with zoomRange and pitchRange all into one.

So was my assumption correct? I have to admit I did not look very deep into the code, and something didn't add up: Why would setting a min/max pitch overwrite setting a min/max zoom? Those to properties should be orthogonal, no?

@sargunv
Copy link
Collaborator

sargunv commented Jun 21, 2025

So was my assumption correct? I have to admit I did not look very deep into the code, and something didn't add up: Why would setting a min/max pitch overwrite setting a min/max zoom? Those to properties should be orthogonal, no?

I think they are orthogonal actually. See this code:

https://github.com/maplibre/maplibre-native/blob/1899cde3bd19dac0b88536c55d7a42b7507460ff/src/mbgl/map/map.cpp#L316-L360

So maybe not a sealed interface, but we should have a regular class CameraBounds or similar? Corresponding to BoundOptions in Native

@westnordost
Copy link
Collaborator

westnordost commented Jun 21, 2025 via email

@sargunv sargunv self-assigned this Jun 21, 2025
@sargunv sargunv self-requested a review June 21, 2025 22:25
@andreas-umbricht
Copy link
Contributor Author

I just double checked: Setting a min-max-Zoom is not affecting the bounding box that I implemented.
Of course if they are mutually exclusive, then you will land in a state, where the zoom cannot fit the bounding box. And if I see that correctly, the link that you provided to the maplibre-native library is not the code called. this.nativeMapView.setLatLngBounds(..) is called instead of setBounds(...)

@sargunv
Copy link
Collaborator

sargunv commented Jun 23, 2025

the link that you provided to the maplibre-native library is not the code called. this.nativeMapView.setLatLngBounds(..) is called instead of setBounds(...)

setLatLngBounds (C++ Android code) then calls setBounds (C++ Core code)

https://github.com/maplibre/maplibre-native/blob/bf4b55727d6612f2f54239dba3f6fadb69f6925d/platform/android/MapLibreAndroid/src/cpp/native_map_view.cpp#L325-L333

void NativeMapView::setLatLngBounds(jni::JNIEnv& env, const jni::Object<mbgl::android::LatLngBounds>& jBounds) {
    mbgl::BoundOptions bounds;
    if (jBounds) {
        bounds.withLatLngBounds(mbgl::android::LatLngBounds::getLatLngBounds(env, jBounds));
    } else {
        bounds.withLatLngBounds(mbgl::LatLngBounds());
    }
    map->setBounds(bounds);
}

@andreas-umbricht
Copy link
Contributor Author

Okay, now I have added a class CameraBounds that simply combines all zoom, pitch and bounds. What do you think about that?

@sargunv
Copy link
Collaborator

sargunv commented Jun 24, 2025

Thanks! I'll play with this a bit, fix the build, and merge sometime in the next few days

@sargunv sargunv force-pushed the camera-bounding-box branch from c3b8bc5 to e6d7662 Compare June 25, 2025 08:50
Copy link
Collaborator

@sargunv sargunv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went ahead and reversed the change to combine the bounds into a single param. Sorry for the churn! I agree with @westnordost , I like it better as separate params like you had it originally

@sargunv sargunv enabled auto-merge (squash) June 25, 2025 08:55
@sargunv sargunv merged commit e131edf into maplibre:main Jun 25, 2025
11 checks passed
@andreas-umbricht
Copy link
Contributor Author

@sargunv No worries, I also like it better that way. Do you have any estimation about a possible release of these changes?
And thank you very much for merging!

@westnordost
Copy link
Collaborator

Current planning is to release it in 0.10.0 for which the (rough) estimation is beginning of July. The biggest open todo for that version would be #374, I believe.

@sargunv
Copy link
Collaborator

sargunv commented Jun 25, 2025

Yup, my goal for the v0.10 cutline is to wrap up the snapshotter (#266), which requires a style state refactor (#374), and so I'm working on a revised demo app (#476) to streamline manual testing of the style state refactor (because historically the programmatic style management has been our main source of bugs and we don't yet have good automated testing for that).

That said, if #374 opens a whole can of worms or I don't have time to work on it in the next week or so, I don't mind punting that and #266 to the next release. Still, I want to wrap up #476.

TLDR: rough estimate is a week or two, with or without the snapshotter

@sargunv
Copy link
Collaborator

sargunv commented Jun 25, 2025

Oh and in the meantime, you can use the snapshot builds from GitHub Packages (see the getting started docs). The snapshot releases ~daily.

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.

3 participants