Repository navigation
feat(HillyRoadAlert): add Hilly & Undulating Road Early Alerts sample across Kotlin, Java, and Compose - #98
LoyalAbbas wants to merge 1 commit into
Conversation
…across Kotlin, Java, and Compose
dkhawk
left a comment
dkhawk
left a comment
There was a problem hiding this comment.
@LoyalAbbas Great work on the 3-way parity architecture, lookahead elevation sparkline, and unit tests!
I did a thorough review and found 2 blocking CI failures (build-ApiDemos unit test failure and :Maps3DSamples:ApiDemos:common:lintDebug missing permission errors), along with a few route dataset mismatches in hilly_routes.json, a BLIND_CREST hazard bounds bug in RoadTerrainAnalyzer, and a couple of lifecycle/performance items. I've left inline comments on the specific lines below.
Also, please add entries for Hilly & Undulating Road Alerts to the three module catalog READMEs (Maps3DSamples/ComposeDemos/app/README.md, Maps3DSamples/ApiDemos/kotlin-app/README.md, and Maps3DSamples/ApiDemos/java-app/README.md).
| put(R.string.feature_title_camera_restrictions, CameraRestrictionsActivity.class); | ||
| put(R.string.feature_title_flight_simulator, FlightSimulatorActivity.class); | ||
| put(R.string.feature_title_path_following, PathFollowingActivity.class); | ||
| put(R.string.feature_title_hilly_road_alerts, HillyRoadAlertsActivity.class); |
There was a problem hiding this comment.
Blocking CI Failure (build-ApiDemos): Adding HillyRoadAlertsActivity increased sampleActivities from 24 to 25, causing MainActivityTest > testSampleActivitiesContainsAllSamples (assertThat(samples).hasSize(24)) to fail in :Maps3DSamples:ApiDemos:java-app:testDebugUnitTest.
Please update Maps3DSamples/ApiDemos/java-app/src/test/java/com/example/maps3djava/mainactivity/MainActivityTest.java to hasSize(25) and assert .contains(com.example.maps3djava.hillyroadalerts.HillyRoadAlertsActivity.class).
| Sample(R.string.feature_title_camera_restrictions, CameraRestrictionsActivity::class.java), | ||
| Sample(R.string.feature_title_flight_simulator, FlightSimulatorActivity::class.java), | ||
| Sample(R.string.feature_title_path_following, PathFollowingActivity::class.java), | ||
| Sample(R.string.feature_title_hilly_road_alerts, HillyRoadAlertsActivity::class.java), |
There was a problem hiding this comment.
Unit Test Failure (:Maps3DSamples:ApiDemos:kotlin-app:testDebugUnitTest): Adding HillyRoadAlertsActivity here also increases samples from 24 to 25, which breaks MainActivityTest.kt (assertThat(samples).hasSize(24)).
Please update Maps3DSamples/ApiDemos/kotlin-app/src/test/java/com/example/maps3dkotlin/mainactivity/MainActivityTest.kt to hasSize(25) and assert .contains(com.example.maps3dkotlin.hillyroadalerts.HillyRoadAlertsActivity::class.java).
| const val BASE_ROUTE_POLYLINE_ID = "hilly_road_base_polyline" | ||
| const val TRAVELED_ROUTE_POLYLINE_ID = "hilly_road_traveled_polyline" | ||
| const val HAZARD_ROUTE_POLYLINE_ID = "hilly_road_hazard_polyline" | ||
| const val VEHICLE_MARKER_ID = "hilly_road_vehicle_marker" |
There was a problem hiding this comment.
Unused constant VEHICLE_MARKER_ID (and <string name="hilly_marker_vehicle_label"> in strings.xml:383) — can be removed since the vehicle is rendered using VEHICLE_MODEL_ID.
| return try { | ||
| val cm = context.getSystemService(Context.CONNECTIVITY_SERVICE) as? ConnectivityManager | ||
| ?: return true | ||
| val activeNetwork = cm.activeNetwork ?: return false |
There was a problem hiding this comment.
Blocking CI Failure (:Maps3DSamples:ApiDemos:common:lintDebug): Calling cm.activeNetwork (line 132) and cm.getNetworkCapabilities(activeNetwork) (line 133) inside :Maps3DSamples:ApiDemos:common fails Android Lint with 4 MissingPermission errors because Maps3DSamples/ApiDemos/common/src/main/AndroidManifest.xml does not declare <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" />.
Declaring <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" /> in Maps3DSamples/ApiDemos/common/src/main/AndroidManifest.xml will fix :common:lintDebug and automatically merge into the downstream app manifests.
| */ | ||
| COASTAL_DRIVE( | ||
| displayName = "Coastal Drive (Flat)", | ||
| subtitle = "Gentle < 2% grade control route" |
There was a problem hiding this comment.
Dataset vs. Preset Mismatch & Hardcoded Strings:
COASTAL_DRIVEis described here as"Coastal Drive (Flat)"/"Gentle < 2% grade control route", andMOUNTAIN_PASSas"Sustained steep climb & blind summit". However, inhilly_routes.json:coastal_drive(37.8700, -122.5899, Muir Beach Overlook) climbs+6.7%to145.3mand then drops65mdown to80.2mat-14.1%grade, triggeringBLIND_CRESTandSTEEP_DESCENT(WARNING) instead of acting as the flat< 2%grade control route (TerrainAlertType.NONE).mountain_passstarts at1380.2mand descends to1342.4mwith 21 closely spaced DEM oscillations (DIP/CRESTpairs), causing it to classify asUNDULATING_ROLLER_COASTERrather thanSTEEP_CLIMB/BLIND_CREST/STEEP_DESCENT.
displayNameandsubtitleare hardcoded English strings (violatingAGENTS.mdRule 4) and are unused by the UI (which usesR.string.hilly_route_*).
| LatLngAltitude(37.80600, -122.43780, 8.3), | ||
| LatLngAltitude(37.80600, -122.43610, 8.0) | ||
| ) | ||
| val state = RoadTerrainAnalyzer.analyze( |
There was a problem hiding this comment.
Please also test HillyRouteData.COASTAL_DRIVE_PATH and HillyRouteData.MOUNTAIN_PASS_PATH directly here (in addition to the synthetic test routes) so unit tests verify that the bundled JSON presets in hilly_routes.json produce their intended TerrainAlertType classifications.
|
|
||
| override fun onMapReady(googleMap3D: GoogleMap3D) { | ||
| super.onMapReady(googleMap3D) | ||
| googleMap3D.setMapMode(Map3DMode.HYBRID) |
There was a problem hiding this comment.
Calling googleMap3D.setMapMode(Map3DMode.HYBRID) directly after super.onMapReady(googleMap3D) overrides Map3DSettingsController.shared (which SampleBaseActivity.onMapReady already initializes with defaultMapMode = Map3DMode.HYBRID). Same in HillyRoadAlertsActivity.java:162.
| if (isReady) { | ||
| lastFrameTimeNanos = frameTimeNanos | ||
| } | ||
| Choreographer.getInstance().postFrameCallback(this) |
There was a problem hiding this comment.
Unconditional 60/120Hz Choreographer Loop & onPause Parity:
doFrame()re-postsChoreographer.getInstance().postFrameCallback(this)on every VSYNC tick even when playback is paused (!viewModel.currentState.isPlaying) or scrubbing. Consider only running the frame callback whileisPlaying && !isUserScrubbing(matching Compose'sLaunchedEffectandPathFollowingActivity.manageAnimationTicker).- For 3-way parity with Compose (
HillyRoadAlertsActivity.kt:164),onPause()in both Kotlin Views and Java Views should also pause playback (viewModel.setPlaying(false)).
| getString( | ||
| R.string.hilly_marker_toast_format, | ||
| markerLabel, | ||
| extremum.distanceAheadMeters.roundToInt(), |
There was a problem hiding this comment.
Stale distanceAheadMeters in Marker Click Toast: Because extrema markers are created once when extremum.id first enters the lookahead horizon (e.g., at ~495m ahead) and cached without recreation, capturing extremum.distanceAheadMeters at marker creation time causes the Toast to show the stale initial distance even when the vehicle is right next to the crest/dip.
Looking up the latest upcomingExtrema entry by extremum.id from viewModel.currentState.terrainAlert.upcomingExtrema inside the click listener (here, in HillyRoadAlertsActivity.java:674, and in Compose HillyRoadAlertsActivity.kt:340) will display the live distance ahead.
| /** | ||
| * Jetpack Compose showcase activity for **"Hilly & Undulating (Roller-Coaster) Road Early Alerts"**. | ||
| */ | ||
| class HillyRoadAlertsActivity : ComponentActivity() { |
There was a problem hiding this comment.
File Size vs. Thin View Guideline (AGENTS.md / ARCHITECTURE.md): At 1,026 lines, this file bundles the Activity, screen, HUD banner, custom sparkline Canvas, and collapsible bottom sheet in one monolithic file (vs. the ~250-line thin-view guideline). Extracting the stateless Compose HUD/Sparkline/BottomControls components into a dedicated file will keep HillyRoadAlertsActivity.kt thin and focused.
| return try { | ||
| val cm = context.getSystemService(Context.CONNECTIVITY_SERVICE) as? ConnectivityManager | ||
| ?: return true | ||
| val activeNetwork = cm.activeNetwork ?: return false |
There was a problem hiding this comment.
This is what makes lint fail: ACCESS_NETWORK_STATE is only declared in the three app manifests, and lint checks common against its own manifest. Since onMapSteady already calls onMapReady(), could we remove isInternetAvailable() together with the uses-permission entries? Otherwise the permission should move to common/src/main/AndroidManifest.xml.
| <string name="hilly_alert_distance_clear">Normal Grade</string> | ||
| <string name="hilly_telemetry_extrema_format" tools:ignore="PluralsCandidate">%1$d Crests / %2$d Dips</string> | ||
| <string name="hilly_telemetry_max_grade_format">Max Grade: +%1$.1f%% / %2$.1f%%</string> | ||
| <string name="hilly_telemetry_advisory_speed_format">Advisory: %1$d km/h</string> |
There was a problem hiding this comment.
I am a bit worried about showing an advisory speed in an official sample. The disclaimer only lives in the KDoc, and on a flat road we display a made-up "Advisory: 80 km/h". Could we drop the speed chip and its calculation? If we want to keep it, we should at least show it only when there is an alert, and say clearly on screen that it is not for driving.
There was a problem hiding this comment.
@kikoso I completely agree with your point. That's why, while creating this advisory speed, I clearly mentioned a disclaimer that it is for reference only : https://github.com/googlemaps-samples/android-maps3d-samples/pull/98/changes#diff-69d1ea93dbc25cd8c23178a9f1445f539113f322c9263e5a6f0cfcdd7f0a9817R924
Still you think we need to remove this ??
|
|
||
| // Only start forward elevation & terrain analysis once the 3D map is ready | ||
| val terrainAlert = if (isMapReady) { | ||
| RoadTerrainAnalyzer.analyze( |
There was a problem hiding this comment.
Just to understand it: analyze() rebuilds the segments and extrema for the whole route on every frame, but the route does not change after selectRoute(). Could we precompute them once per route and only slice the lookahead window here?
| /** | ||
| * Real-world GPS/DEM route trace for the undulating Roller-Coaster road preset. | ||
| */ | ||
| val SAN_FRANCISCO_HYDE_ST_ROLLER_COASTER: List<LatLngAltitude> |
There was a problem hiding this comment.
The coordinates for this route are around 36.49, -114.53 (Lake Mead area, Nevada), not Hyde Street in San Francisco. Should we give it a neutral name?
| */ | ||
| COASTAL_DRIVE( | ||
| displayName = "Coastal Drive (Flat)", | ||
| subtitle = "Gentle < 2% grade control route" |
There was a problem hiding this comment.
nit: the bundled coastal route goes from ~139 m down to ~82 m over ~1.35 km (around -4% net), so "< 2%" is not accurate. Would it make sense to add a test asserting this route yields NONE, if it is meant as the control?
| getString( | ||
| R.string.hilly_marker_toast_format, | ||
| markerLabel, | ||
| extremum.distanceAheadMeters.roundToInt(), |
There was a problem hiding this comment.
distanceAheadMeters is captured when the marker is created, so the toast keeps showing the same "N m ahead" while the car moves (same in the Java and Compose versions). Could we compute it at click time from the current state?
kikoso
left a comment
kikoso
left a comment
There was a problem hiding this comment.
Hi @LoyalAbbas ! A few things before this can go in. build-ApiDemos fails because MainActivityTest in both java-app and kotlin-app still expects 24 samples, so those need hasSize(25). Since this adds a new sample, let's use feat instead of fix in the title. And in general, ~6,900 lines is a lot for a sample: could we cut it down (one route, a simpler grade scan, no speed model) so it stays easy to follow?
Hi @kikoso Most of this code is shared infrastructure, boilerplate, and common logic needed across samples, with only the route definitions differing. If we were to cut it down, we'd essentially need to split this up by stack level (Java, Kotlin, and Compose). Would you prefer us to break this down by stack level to keep each module smaller? |
LoyalAbbas
changed the title
Summary
Adds the Hilly & Undulating (Roller-Coaster) Road Early Alerts showcase sample with full 3-way parity across Kotlin Views, Java Views, and Jetpack Compose.
This sample demonstrates how 3D elevation route profiles (
LatLngAltitudewith explicitABSOLUTEaltitudes) can be analyzed ahead of a moving vehicle in real time to detect rapid crests, dips, blind summits, and sustained steep climbs/descents before the driver reaches them.Key Features
RoadTerrainAnalyzer):250m,500m,750m) ahead of the vehicle to detect local elevation peaks (CREST) and troughs (DIP) using vertical prominence hysteresis (when (state)state machine).Max Grade: +X.X% / -Y.Y%) and classifies upcoming hazards (UNDULATING_ROLLER_COASTER,BLIND_CREST,STEEP_CLIMB,STEEP_DESCENT,NONE) withCAUTION/WARNINGseverity escalation.HillyRoadAlertViewModel):CAUTIONamber /WARNINGred), crest/dip 3D markers, and the 3D red car marker tracking the route.onMapReady) and gracefully handles offline launches (ACCESS_NETWORK_STATE) so the native Maps 3D offline prompt displays without crashing.ElevationSparklineView/ ComposeCanvas) with vehicle, crest, and dip indicators.Roller-Coaster,Mountain Pass,Coastal), lookahead horizon selector, playback speed (0.5x–5x), and crest camera preview toggle.hilly_routes.json&HillyRouteData):Maps3DSamples/ApiDemos/common/src/main/resources/hilly_routes.jsonand loads them lazily on first access to keep Kotlin sources and.dexstatic initialization lean.Architecture & 3-Way Parity
:Maps3DSamples:ApiDemos:common):RoadTerrainAnalyzer.kt— Pure Kotlin terrain, extrema, grade, sparkline, and advisory speed engine.HillyRouteData.kt&resources/hilly_routes.json— Lazy-loaded 3D route presets.HillyRoadAlertViewModel.kt— UDF presentation state machine exposingStateFlow(Kotlin/Compose) andLiveDataviaasLiveData()(Java) synchronized with hardware frame ticks.ElevationSparklineView.kt&res/layout/activity_hilly_road_alerts.xml— Shared custom sparkline view and Material 3 layout for Kotlin & Java Views.Maps3DSamples/ApiDemos/kotlin-app/.../hillyroadalerts/HillyRoadAlertsActivity.ktMaps3DSamples/ApiDemos/java-app/.../hillyroadalerts/HillyRoadAlertsActivity.javaMaps3DSamples/ComposeDemos/app/.../hillyroadalerts/HillyRoadAlertsActivity.ktTesting & Verification
:Maps3DSamples:ApiDemos:common:RoadTerrainAnalyzerTest.kt— Verifies flat/gentle control routes, earlyCAUTION→WARNINGescalation on undulating terrain, stable crest/dip IDs and grade deltas, sustained climb/blind crest/descent classification, and zero-distance/duplicate-point guards.HillyRoadAlertViewModelTest.kt— VerifiesonMapReadygating, frame-tick progression, route preset switching, horizon updates, scrubbing, and Jump to Hills behavior../gradlew spotlessApply,:Maps3DSamples:ApiDemos:common:testDebugUnitTest, and debug compilation across:Maps3DSamples:ApiDemos:kotlin-app,:Maps3DSamples:ApiDemos:java-app, and:Maps3DSamples:ComposeDemos:app.