Repository navigation
feat(camera): add 4-way AltitudeMode camera and marker controls across Kotlin, Java, and Compose - #86
LoyalAbbas wants to merge 4 commits into
Conversation
kikoso
left a comment
kikoso
left a comment
There was a problem hiding this comment.
Hi @LoyalAbbas ! This is a nice addition, and the comparison table in the description is very useful. A few comments inline. It is also quite a big PR (around 2.3k lines across the three apps), so would it make sense to move the maps3d-compose change to its own PR? Thanks!
| <string name="fly_around">Fly around center</string> | ||
| <!-- Fly to the Empire State Building --> | ||
| <string name="fly_to">Fly to ESB 🏢</string> | ||
| <string name="fly_to_mountain">Fly to Mt. Rainier (Relative Alt)</string> |
There was a problem hiding this comment.
nit: the button uses whatever mode is selected in the chips, so "(Relative Alt)" is misleading. What about matching the ESB button?
| <string name="fly_to_mountain">Fly to Mt. Rainier (Relative Alt)</string> | |
| <string name="fly_to_mountain">Fly to Mt. Rainier ⛰️</string> |
| tilt = source.tilt.toTilt() | ||
| roll = source.roll.toRoll() | ||
| range = source.range.toRange() | ||
| if (source.altitudeMode == AltitudeMode.ABSOLUTE || |
There was a problem hiding this comment.
We repeat this same three-mode check four times (here, in copy, and in the two maps3d-compose copies). Could we extract a small isValidCameraAltitudeMode() helper? It would also be a good place to document why CLAMP_TO_GROUND is left out for cameras.
| if (oldConfig != config) { | ||
| // Config changed, update by adding again with same ID! | ||
| // Config changed, remove previous marker and recreate with updated options | ||
| marker.remove() |
There was a problem hiding this comment.
Just to understand it: the previous comment said that adding again with the same ID updates the marker, so why do we need the remove() now? Since the beacon key includes the mode and the refresh token, this sample never reaches this branch, so I would move this library change to its own PR (with a test) instead of hiding it inside a feat(camera).
| RELATIVE_CLEARANCE_METERS to AltitudeMode.RELATIVE_TO_GROUND | ||
| } | ||
| return markerOptions { | ||
| id = "${markerIdPrefix}_${mode}_$refreshToken" |
There was a problem hiding this comment.
I wonder if we need the refreshToken at all. Recreating the marker with a new ID after every flight and on onMapSteady reads like a workaround for RELATIVE_TO_MESH not resampling once the tiles load. If that's the case, could we add a comment explaining it (and maybe raise it with the SDK team)? Otherwise developers will copy this pattern thinking is required.
| altitudeMode = AltitudeMode.RELATIVE_TO_MESH | ||
| } | ||
|
|
||
| AltitudeMode.CLAMP_TO_GROUND -> camera { |
There was a problem hiding this comment.
Since Camera does not accept CLAMP_TO_GROUND, this branch ends up as a RELATIVE_TO_GROUND camera at 0m, while the telemetry panel still shows Mode: CLAMP_TO_GROUND. Should we show the mode the camera actually uses, or make the clamp chip affect only the beacon? Otherwise it may confuse people learning the API.
| activeTargetResId = targetResId | ||
| val isMountain = (targetResId == R.string.camera_target_mt_rainier) | ||
|
|
||
| // Clear NYC bounds restriction if active so the camera can travel to Washington State |
There was a problem hiding this comment.
nit: this duplicates the logic of the toggle_restriction click listener. What about extracting a small setCameraRestricted(enabled: Boolean) and using it in both places?
| class DataModelTest { | ||
|
|
||
| @Test | ||
| fun mtRainierCamera_relativeToGround_usesUnifiedClearanceAltitude() { |
There was a problem hiding this comment.
Most of these tests just assert that the builder returns the constants we passed in, so they will break when we tweak the framing values rather than catch real bugs. My two cents: keep the ones that check the mode mapping (e.g. CLAMP_TO_GROUND falling back to RELATIVE_TO_GROUND, isExtruded) and drop the rest. That would also trim a few hundred lines across the three apps.
Thanks for the review @kikoso ! I reverted the Map3DState.kt change completely and trimmed ~350 lines of redundant tests to shrink the PR. The only remaining maps3d-compose change is 23 lines in Utilities.kt (toValidCamera() / Camera.copy()) so GoogleMap3D(camera = ...) preserves altitudeMode for the Compose sample. Happy to split those 23 lines into a separate PR first if you still prefer! |
…s Kotlin, Java, and Compose
- Update Mt. Rainier button label to "Fly to Mt. Rainier ⛰️" and remove unused clamp string resource - Extract isValidCameraAltitudeMode() helper in common and maps3d-compose Utilities - Revert Map3DState.kt marker.remove() change in maps3d-compose - Refresh beacon marker only on onMapSteady when RELATIVE_TO_MESH is active - Show actual camera altitude mode (RELATIVE_TO_GROUND) in telemetry when CLAMP_TO_GROUND is selected - Extract setCameraRestricted(enabled) helper in Kotlin and Java CameraControlsActivity - Trim redundant constant assertions in DataModelTest across Kotlin, Java, and Compose
4512993 to
4a07c60
Compare
…camera framing, and marker clamping fixes
|
Hey @LoyalAbbas — thanks for putting together the 4-way We ran a full review + live-device verification across all three variants ( 1. Merge Conflicts with
|
…ith_altitudemode # Conflicts: # Maps3DSamples/ApiDemos/common/src/main/java/com/example/maps3d/common/Map3DSettingsController.kt # Maps3DSamples/ApiDemos/common/src/main/java/com/example/maps3d/common/Utilities.kt # Maps3DSamples/ComposeDemos/app/src/main/java/com/example/composedemos/cameracontrols/CameraControlsActivity.kt
dkhawk
left a comment
dkhawk
left a comment
There was a problem hiding this comment.
Please consider the comments I left, but good otherwise
Summary
Enhances
CameraControlsActivityacross Kotlin Views, Java Views, and Jetpack Compose to demonstrate all fourAltitudeModeoptions (RELATIVE_TO_GROUND,RELATIVE_TO_MESH,CLAMP_TO_GROUND, andABSOLUTE) at both the Empire State Building and Mount Rainier summit.Key Changes
AltitudeModePipeline: Consolidated camera and marker configuration into sharedbuildTargetCameraandbuildTargetBeaconOptions/buildTargetBeaconConfigbuilders inDataModelacross all three apps.AltitudeModeSupport: Added interactive filter chips forRelative to Ground(+100m DTM),Relative to Mesh(+360m DSM),Clamp to Ground(0m DTM), andAbsolute(212m MSL at ESB / 4,392m MSL at Mt. Rainier).flyCameraAroundorbit on arrival.onMapSteadysoRELATIVE_TO_MESHaccurately samples loaded 3D building/terrain tiles.toValidCamera()andCamera.copy()incommonandmaps3d-composeto preserve validaltitudeModevalues.CameraControlsActivityto launch directly at the Empire State Building and display the active target andAltitudeModein the camera telemetry panel.AltitudeMode Comparison
CLAMP_TO_GROUND0m(street base)0m(summit snowpack)RELATIVE_TO_GROUND+100m(lower tower)+100mabove terrainABSOLUTE212mMSL (mid-tower)4,392mMSL (summit)RELATIVE_TO_MESH+360m(above spire)+360mabove 3D meshTesting & Verification
DataModelTest) across Kotlin Views, Java Views, and Jetpack Compose verifying camera framing, marker extrusion, andAltitudeModepreservation../gradlew spotlessApplyacross all modified modules../gradlew :Maps3DSamples:ApiDemos:kotlin-app:assembleDebug :Maps3DSamples:ApiDemos:kotlin-app:testDebugUnitTest./gradlew :Maps3DSamples:ApiDemos:java-app:assembleDebug :Maps3DSamples:ApiDemos:java-app:testDebugUnitTest./gradlew :Maps3DSamples:ComposeDemos:app:assembleDebug :Maps3DSamples:ComposeDemos:app:testDebugUnitTest