-
-
Notifications
You must be signed in to change notification settings - Fork 40
wip: map snapshotter #266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
wip: map snapshotter #266
Conversation
|
Thanks for this! Currently planning on reviewing next week or so. |
lib/maplibre-compose/src/androidMain/kotlin/dev/sargunv/maplibrecompose/core/util/util.kt
Outdated
Show resolved
Hide resolved
lib/maplibre-compose/src/jsMain/kotlin/dev/sargunv/maplibrecompose/core/JsMapSnapshotter.kt
Show resolved
Hide resolved
lib/maplibre-compose/src/commonMain/kotlin/dev/sargunv/maplibrecompose/core/MapSnapshotter.kt
Outdated
Show resolved
Hide resolved
lib/maplibre-compose/src/commonMain/kotlin/dev/sargunv/maplibrecompose/compose/CameraState.kt
Outdated
Show resolved
Hide resolved
lib/maplibre-compose/src/commonMain/kotlin/dev/sargunv/maplibrecompose/compose/CameraState.kt
Outdated
Show resolved
Hide resolved
lib/maplibre-compose/src/commonMain/kotlin/dev/sargunv/maplibrecompose/compose/CameraState.kt
Show resolved
Hide resolved
...bre-compose/src/androidMain/kotlin/dev/sargunv/maplibrecompose/core/AndroidMapSnapshotter.kt
Outdated
Show resolved
Hide resolved
|
Thank you for your review, I fixed all the things you mentioned. |
lib/maplibre-compose/src/commonMain/kotlin/dev/sargunv/maplibrecompose/core/SnapshotResponse.kt
Outdated
Show resolved
Hide resolved
...bre-compose/src/androidMain/kotlin/dev/sargunv/maplibrecompose/core/AndroidMapSnapshotter.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Super clean implementation
Appreciate your review! Did you test it on iOS? As I mentioned, I didn't, because I don't have a Mac right now. I will probably get one in a couple of weeks, so if there is any problem, I can fix it. |
|
I didn't test anything, just read the code. |
|
it takes CameraPosition and region, but it requires only one of them.
That sounds like a good use case for a sealed interface! E.g.
sealed interface SnapshotLocation {
data object Camera(position: CameraPosition) : SnapshotLocation
data object Bounds(bounds: BoundingBox) : SnapshotLocation
}
El 24 de mayo de 2025 21:33:43 CEST, "Michał Gwóźdź" ***@***.***> escribió:
…michalgwo left a comment (maplibre/maplibre-compose#266)
> * implementing a "share" feature, where an image of something the user is viewing on the map is shared as an image through the system share dialog. In this case, we probably want the snapshot to include all the same styling (including programmatic layers) as the actual interactive map the user is using, though image dimensions may be different than the view
I think this is the use case most people are using Snapshotter for. As I mentioned before, for now we can't include programmatic layers yet, but when https://github.com/issues/created?issue=maplibre%7Cmaplibre-native%7C3052 will be resolved, we can probably make it.
> * displaying efficient static maps, where perhaps many maps would be displayed in a list, or the map doesn't need to be interactive. In this case, we probably don't want the snapshot API to be dependent on the Compose map / CameraState at all; rather it's just a KMP wrapper around the MLN snapshot APIs. Would still be good to support programmatic styles, though that might be out of scope for this first implementation.
This is actually a wrapper around MLN Snapshotter, the `cameraPosition` isn't required here. It's a bit confusing how the snapshotter is made in the MLN, it takes `CameraPosition` and `region`, but it requires only one of them. When set in conjunction, `CameraPosition.target` is overridden by `region`.
> Do you have some other use cases you're intending to use this for, where the API considerations are different?
Not really
--
Reply to this email directly or view it on GitHub:
#266 (comment)
You are receiving this because you commented.
Message ID: ***@***.***>
|
I can imagine a scenario when someone wants to set both |
lib/maplibre-compose/src/desktopMain/kotlin/dev/sargunv/maplibrecompose/core/WebviewMap.kt
Show resolved
Hide resolved
|
That's... possible? after all, with tilt or camera rotation , the displayed area is not a bbox anymore. I'd presume that if a camera position is specified, at least if it has any rotation or tilt, the camera position takes precedence and the bbox is discarded(?)
El 24 de mayo de 2025 23:09:05 CEST, "Michał Gwóźdź" ***@***.***> escribió:
…michalgwo left a comment (maplibre/maplibre-compose#266)
> That sounds like a good use case for a sealed interface!
I can imagine a scenario when someone wants to set both `cameraPosition` and `region`, for example, when they have a BoundingBox of the region, but want it rotated/tilted, which needs to be set with `cameraPosition`.
--
Reply to this email directly or view it on GitHub:
#266 (comment)
You are receiving this because you commented.
Message ID: ***@***.***>
|
According to the documentation, when I just tested setting |
Perhaps our parameters should be bounding box or position/zoom, with optional bearing and tilt? |
|
This, plus the attribution state stuff, is making me think the style URL and style composition should be something passed into the style state rather than into the map. And then, the user can initialize a camera + style state however they wish, and pass those two into either a map, or a snapshotter, or both |
|
Relevant comment at #374 (comment) My suggestion to support StyleState with programmatic styling is possible in Android I'll take this PR to the finish line |
|
Ah nope I was wrong; programmatic styling is available in iOS: #374 (comment) So yeah, back to the original plan to land this after our style state refactor so we can use it here. |
Fixes #28
I implemented snapshot functionality for Android and iOS. There is no snapshotter in the MapLibre GL JS library, so I left it empty.
I also created a demo presenting snapshot functionality.
As for iOS, as I already mentioned in my previous PRs, I don't have a Mac, so I couldn't run it, but this time I tried my best to implement it for iOS as well, but as it's not tested, and I don't have any experience with iOS, it may not work 😅 Let me know if you are not interested in fixing it, so I will remove the iOS part, and leave only Android implementation.
A few remarks about the iOS implementation:
CameraPosition.toMLNCamera(), so I took my shot and used the same as the size of the snapshotcameraPositionparam for snapshotter options is nullable, but on iOS it's not, I'm not sure if it's the best solution, but I made it nullable in common code and on iOS created a new emptyMLNMapCamera()whencameraPositionisnullStringreturned on Android andNSErrorreturned on iOS. I also failed to make the snapshotter return error, so I have no idea what kind of error is there.