Skip to content

feat(samples): align Camera Controls, Markers, Polygons, and Popovers across Kotlin, Java, and Compose - #76

Open
LoyalAbbas wants to merge 1 commit into
mainfrom
feat/filling_gap_all_platform
Open

LoyalAbbas wants to merge 1 commit into
mainfrom
feat/filling_gap_all_platform

Conversation

@LoyalAbbas

Copy link
Copy Markdown
Collaborator

Summary

Aligns feature parity across Kotlin, Java, and Jetpack Compose sample apps for Camera Controls, Markers, Polygons, and Popovers using the Kotlin implementation as the reference.

Changes

  • Camera Controls (Compose): Added auto-fly to Empire State Building, 3D NYC restriction boundary box, camera restriction toggle, map mode selector, roll slider, live telemetry readout, and camera action controls.
  • Markers (Compose & maps3d-compose): Added PinConfig/GlyphConfig support to maps3d-compose; updated Compose sample with 4-AltitudeMode Berlin markers, NYC custom pins, monsters.json popovers, and Monster Tour controls.
  • Polygons (Java, Compose & maps3d-compose): Updated Java extruded polygon to AltitudeMode.ABSOLUTE; updated Compose sample with the extruded museum polygon, Denver Zoo hole cutout, and entry fly-to animation.
  • Popovers (Java, Compose & maps3d-compose): Anchored Compose Popover directly to Marker instances; updated Java and Compose samples to match the Golden Gate Bridge interactive popover with Toggle Configured counter and camera fly-to transitions.

Testing

  • Verified unit tests (:maps3d-compose:testDebugUnitTest) and debug builds across Java and Compose sample modules.
  • Applied Spotless formatting on all modified files.

@LoyalAbbas
LoyalAbbas requested review from dkhawk and kikoso September 29, 2026 08:36

@kikoso kikoso left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review: cross-platform parity for Camera Controls, Markers, Polygons, Popovers

Nice breadth here, and the parity work is welcome. The PinConfig/GlyphConfig plumbing plus the AltitudeMode.ABSOLUTE fix in the Java PolygonsActivity all look right. A few things to sort out before this lands, though, and the two most serious ones are in maps3d-compose library code rather than sample code, so they affect every consumer.

Blocking (2):

  • GoogleMap3D.kt:181: the new camera dedup guard can never match (Camera has no equals), and because it writes a snapshot state it reads inside the AndroidView update lambda, it re-invalidates itself. Net effect: the camera is continuously snapped back and no fly animation can complete.
  • Map3DState.kt:314: removing popover.show() makes popovers invisible by default, because the SDK's add-popover path calls hide() on the new popover. The Compose Markers sample never wires up the replacement onPopoverCreated hook, so its popovers never appear.

Worth fixing (3): a stopMonsterTour() / flyCameraTo cancellation race in MarkersActivity (~L559 and four more sites), awaitMapSteady hijacking the map's single steady listener (~L754), and the 350 ms postDelayed { setCamera(...) } truncating the entry fly-to that this PR adds to the Polygons sample.

Minor (4): lifecycle onResume/onPause asymmetry, a leaked layout listener with a guard that misses the first layout, an unrecoverable popover after remove(), and an eagerly-set hasTriggeredInitialFlyTo flag. All inline.

Checked and clear: every CommonR string/drawable reference resolves; monsters.json is present in ApiDemos/common assets and has every key the new MonsterParser needs for all 8 entries; MarkerOptions.setStyle(PinConfiguration), zIndex, PolygonOptions.geodesic/drawsOccludedSegments and Glyph/PinConfiguration all exist in the SDK; the Java PolygonsActivity switch to AltitudeMode.ABSOLUTE is correct because museumBaseFace is built at museumAltitude (1 mile), matching the Kotlin reference; and PopoversVisualTest's textFound == true change genuinely fixes a potential null-unboxing NPE.

One caveat on my verification: the bytecode checks behind the two blocking findings ran against play-services-maps3d 0.2.0 (the only version in my Gradle cache) while libs.versions.toml declares 0.2.2. The missing equals and the hide() call are very unlikely to have changed, but flagging it so you can confirm against 0.2.2.

applyUpdates()
}
// Sync hoisted state with the imperative map instance
if (lastSyncedCamera.value != validCamera) {

@kikoso kikoso Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

High: this dedup guard can never match, and it re-triggers the update block.

com.google.android.gms.maps3d.model.Camera extends AbstractSafeParcelable and declares no equals/hashCode (verified with javap against the published AAR: only getters/setters and writeToParcel). Since camera.toValidCamera() allocates a brand-new Camera on every pass, lastSyncedCamera.value != validCamera is always true.

Two consequences:

  1. setCamera runs on every update pass rather than only on change.
  2. lastSyncedCamera is read here and written on the next line inside the AndroidView update lambda, which Compose runs under snapshot read observation. The write invalidates the scope, runUpdate() re-runs, allocates another Camera, compares unequal, writes again.

Repro: open Markers / Polygons / CameraControls and pan the map, or start the Monster Tour. The camera is snapped back to the camera parameter, so no fly animation can complete.

Suggested fix: compare the field values, or keep the last-applied camera in a non-snapshot holder so the write doesn't invalidate:

val lastSyncedCamera = remember { Ref<Camera>() }   // not mutableStateOf
...
if (!lastSyncedCamera.value.matches(validCamera)) {
    googleMap3D.setCamera(validCamera)
    lastSyncedCamera.value = validCamera
}

currentOnMapReady(googleMap3D)
hasCalledOnMapReady.value = true
// Ensure native map viewport has stabilized before applying initial camera
map3dView.postDelayed({

@kikoso kikoso Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Medium: this 350 ms postDelayed overwrites camera animations started from onMapReady.

polygons/PolygonsActivity.kt (~L276-283) kicks off a 1000 ms flyCameraTo(denverCamera) inside onMapReady. 350 ms later this block hard-sets the camera to lastSyncedCamera, truncating the entry fly-to animation this PR adds. Any sample that animates from onMapReady hits the same thing.

Consider skipping the delayed setCamera if an animation is in flight, or driving the initial camera through Map3DInitConfig only (which this PR already wires up above) instead of re-applying it after the fact.

factory = { context ->
val map3dView = Map3DView(context, options)
map3dView.onCreate(null)
map3dView.onResume()

@kikoso kikoso Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Low: onResume() is called here, but onPause() only happens in onRelease.

There's no lifecycle observer, so a Map3DView that is now explicitly resumed never gets paused when the host activity is backgrounded, only when the composable leaves the composition. Worth pairing with LocalLifecycleOwner + a DisposableEffect observer so resume/pause track the host lifecycle.

onRelease = { map3dView ->
state.clear()
Map3DRegistry.clearInstance()
map3dView.onPause()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

See the note on the onResume() call in factory: this is the only onPause() in the composable, so backgrounding the activity leaves the view resumed.

}
}

config.onPopoverCreated?.invoke(popover)

@kikoso kikoso Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

High: dropping popover.show() leaves popovers permanently invisible for callers that don't opt in.

PopoverManager's add-popover path ends with invokevirtual com/google/android/gms/maps3d/Popover.hide() (confirmed by disassembling the AAR), and Popover.isVisible just reads content.visibility. So a library-created popover starts hidden. Replacing show() with onPopoverCreated means nothing appears unless the caller wires the hook up.

The new PopoversActivity samples do opt into manual toggling, but ComposeDemos markers/MarkersActivity.kt's createPopoverConfig (~L692) never sets onPopoverCreated. Repro: tap the Giant Ape or any monster marker in the Compose Markers sample, and no popover ever shows.

Rather than silently changing library behaviour, suggest adding an opt-out flag so the default stays "visible":

data class PopoverConfig(
    ...
    val startVisible: Boolean = true,
    val onPopoverCreated: ((Popover) -> Unit)? = null,
)

then if (config.startVisible) popover.show() here.

// When PopoverManagerImpl first receives the anchor's DrawingState, popover.content is
// still View.GONE (width = 0, height = 0), so its initial (x, y) is placed at the raw
// anchor coordinates before ComposeView measures. Adjust (x, y) when layout size changes.
popover.content.addOnLayoutChangeListener {

@kikoso kikoso Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Low: this layout listener is leaked, and its guard skips the first layout.

Two things:

  1. The OnLayoutChangeListener is never removed when the popover is removed, so it outlives the popover it corrects.
  2. The guard (view.x != 0f || view.y != 0f) skips the correction whenever the view hasn't been positioned yet (x == y == 0) at the moment the ComposeView first measures, which is exactly the 0x0 to measured transition the comment above describes. In that case the popover stays offset by half its width / its full height.

Consider tracking whether the correction has been applied rather than inferring it from the position, and removing the listener in the popover teardown path.

.alpha(0.85f)
.clip(CircleShape)
.combinedClickable(
onClick = {

@kikoso kikoso Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Medium: stopMonsterTour() followed by an immediate flyCameraTo cancels the new flight. (Same pattern at ~L597, L614, L635, L656.)

stopMonsterTour() only sets isTouring = false. The LaunchedEffect(isTouring, ...) coroutine isn't cancelled until the composition applies, after this handler has already started its flyCameraTo. When the cancellation does land, suspendCancellableCoroutine.invokeOnCancellation (~L727 and L744) calls stopCameraAnimation(), which kills the flight started here.

Repro: while the tour is running, tap the dice / Berlin / NYC / Reset controls. The camera starts moving and immediately freezes instead of flying to the target.

Fix: stop the animation only from the effect's own cleanup, or hand the pending target to the effect and let it perform the flight.

/**
* Suspends until the map reports it is steady (finished rendering 3D tiles), up to [timeout].
*/
private suspend fun GoogleMap3D.awaitMapSteady(

@kikoso kikoso Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Medium: this helper hijacks the map's single steady listener and usually burns the whole timeout.

GoogleMap3D supports only one setOnMapSteadyListener, so this overwrites the listener that the GoogleMap3D composable registered in its factory. After the first tour step the composable's onMapSteady parameter never fires again: the restored listener only calls onSteadyRestored, and isMapSteady is never reset to false.

Separately, the listener is registered after the 4 s fly-to has already finished, so if the map is already steady no further callback arrives and each tour step stalls for the full 5 s timeout.

Consider having the library expose a multiplexed steady callback (or a Flow) rather than having samples swap the single listener in and out.

scope.launch {
Log.d(TAG, "Marker clicked")
if (popoverToggleCount > 5) {
popover?.remove()

@kikoso kikoso Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Low: once remove() runs, the popover can't come back.

Map3DState.popovers still holds the entry under golden_gate_popover with an unchanged config, so syncPopovers never recreates it, and the hoisted popover state still points at the removed instance.

Repro: tap the marker 7 times. The popover is removed, and every later tap calls toggle() on a dead popover, so the sample is stuck for the rest of the activity.

This mirrors the Kotlin View sample, so it may be intentional parity, but the Compose port could clear the hoisted popover and re-key the config to recover.

LaunchedEffect(isMapSteady, googleMap3D) {
val map = googleMap3D
if (isMapSteady && map != null && !hasTriggeredInitialFlyTo) {
hasTriggeredInitialFlyTo = true

@kikoso kikoso Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Low: the guard flag is set before the delay, so a cancellation loses the fly-to permanently.

hasTriggeredInitialFlyTo = true happens ahead of delay(2000). If the effect is cancelled during that window (isMapSteady / googleMap3D changing, a configuration change, and so on) the flag stays set and the auto fly-to to the Empire State Building never happens.

Set the flag after the flight is actually issued, or key the effect so it can't restart mid-delay.

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.

2 participants