Skip to content

feat(codelab): introduce aloha explorer 3d maps starter and solution - #36

Open
dkhawk wants to merge 1 commit into
mainfrom
feat/adds-codelab
Open

dkhawk wants to merge 1 commit into
mainfrom
feat/adds-codelab

Conversation

@dkhawk

@dkhawk dkhawk commented Apr 2, 2026

Copy link
Copy Markdown
Collaborator
  • Added starter architecture with comprehensive TODO curriculm.
  • Added full solution module reflecting best-practices.
  • Included instructional codelab.md guide covering Camera animations, Popovers, Extrusions and Compose interop.

@dkhawk
dkhawk requested a review from kikoso April 2, 2026 17:30
@dkhawk
dkhawk marked this pull request as ready for review April 10, 2026 19:45

@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.

There is one blocker that stops you at step one plus a few things that will bite us later.

Blocker: the Prerequisites tell you to put MAPS_API_KEY in secrets.properties, but both manifests read ${MAPS3D_API_KEY}. Anyone following the guide literally builds an app that silently authenticates with DEFAULT_API_KEY and sees an empty map, with no error pointing at the key. Detail inline on codelab.md:42.

The thing I care most about beyond that: codelab/ is two standalone Gradle builds with their own settings.gradle.kts and wrapper, and .github/workflows/build.yml enumerates its targets explicitly (:Maps3DSamples:ApiDemos:*, :Maps3DSamples:advanced:app, :PlacesUIKit3D). Nothing compiles codelab/starter or codelab/solution. This PR was opened in April and already carries playServicesMaps3d = "0.2.0" against 0.2.2 on main, agp = "9.1.0" against 9.2.1, and compileSdk = "36" against 37, which is the drift starting. A codelab that does not build is worse than no codelab, because the failure lands on someone learning the SDK who assumes it is their fault.

Could we add an assembleDebug job for both directories to build.yml in this PR? It is a working-directory matrix entry and it is what keeps this alive.

Everything else below is individually small.

Comment thread codelab/codelab.md

The `starter` project comes with the Secrets plugin pre-configured for you! All you need to do is create a new file called `secrets.properties` in the root of your project and add:
```properties
MAPS_API_KEY=YOUR_API_KEY_HERE

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.

This is the blocker. The key name here does not match what either app reads.

  • codelab/starter/app/src/main/AndroidManifest.xml declares com.google.android.geo.maps3d.API_KEY with value ${MAPS3D_API_KEY}.
  • codelab/starter/local.defaults.properties defines MAPS3D_API_KEY=DEFAULT_API_KEY.

So a learner who creates secrets.properties with MAPS_API_KEY as instructed gets no override at all. The secrets plugin falls back to the default file, the manifest is injected with the literal DEFAULT_API_KEY, and the app builds and installs cleanly. The map then just fails to authenticate, with the only clue buried in logcat. That is the worst possible failure shape for step one of a codelab.

Suggested change
MAPS_API_KEY=YOUR_API_KEY_HERE
MAPS3D_API_KEY=YOUR_API_KEY_HERE

Two smaller things in this same section while you are here:

  • Step 3 says the same thing twice. Lines 46-54 tell you to uncomment implementation(libs.play.services.maps3d), then line 55 says "Ensure the Maps 3D SDK dependency is also present in dependencies { ... }" with an identical snippet. I would drop the second one.
  • codelab/solution/local.defaults.properties uses MAPS3D_API_KEY=YOUR_API_KEY while the starter and every other defaults file in the repo (root, snippets/, Maps3DSamples/advanced/, Maps3DSamples/ApiDemos/) use DEFAULT_API_KEY. Worth aligning, since YOUR_API_KEY reads like a placeholder someone forgot to fill in.

* the next animation or interaction. This implementation wraps the callback-based
* `setOnMapSteadyListener` into a standard Kotlin `suspend` function.
*/
suspend fun awaitMapSteady(map: GoogleMap3D) = suspendCancellableCoroutine { continuation ->

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.

This is a fork of Maps3DSamples/ApiDemos/kotlin-app/src/main/java/com/example/maps3dkotlin/common/MapExtensions.kt on main, with the robustness taken out. The canonical version is:

suspend fun GoogleMap3D.awaitMapSteady(timeout: Duration): Boolean {
    val result = withTimeoutOrNull(timeout) { ... }
    return result != null
}

and MarkersActivity.kt:223 calls it as map.awaitMapSteady(5.seconds). The timeout is the entire point: if the map never reports steady, because tiles fail to load, the device is offline, or the API key is wrong (which, per my note on codelab.md:42, is the state most first-time learners will be in), this version suspends forever. The button appears dead, no exception, no log line.

Given that this lives in the section literally titled "Robustness (The Secret Sauce)", teaching the unbounded variant sends the wrong lesson. I would port the withTimeoutOrNull version verbatim and make the timeout a talking point in section 3.

* Similar to `awaitMapSteady`, this pauses our code execution until the camera finishes its flight.
* This allows us to write strict sequences like: "Fly to A, THEN wait, THEN fly to B".
*/
suspend fun awaitCameraAnimation(map: GoogleMap3D) = suspendCancellableCoroutine { continuation ->

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.

Two divergences from the MapExtensions.kt version on main, both of which matter here:

1. The animation is started outside the suspend block. Callers do map.flyCameraTo(...) and then awaitCameraAnimation(map), see MainActivity.kt:237-250 and MainActivity.kt:623-628. If the animation completes in the window between those two statements, the end listener is registered after the event has already fired and the coroutine never resumes. Main avoids this by taking the options and calling flyCameraTo as the last statement inside suspendCancellableCoroutine, after the listener is installed:

suspend fun GoogleMap3D.awaitCameraAnimation(options: FlyToOptions) {
    suspendCancellableCoroutine<Unit> { cont ->
        this.setCameraAnimationEndListener { ... }
        cont.invokeOnCancellation { ... }
        this.flyCameraTo(options)   // started last, on purpose
    }
}

That is a narrow window for a 2 second flight, but it is not narrow for the short hops in flyTour, and an intermittently hanging codelab is a support burden we will not enjoy.

2. Cancellation does not stop the camera. Main's invokeOnCancellation calls stopCameraAnimation() before clearing the listener; this one only clears the listener. Every button in setupButtons does currentAnimationJob?.cancel() first, and btn_clear does nothing else, so tapping Clear mid-flight detaches the coroutine while the camera keeps flying to wherever the cancelled animation was headed. Easy to reproduce: tap Scenic Tour, then Clear.

Adopting MapExtensions.kt wholesale would fix both and give the codelab the same helpers the samples ship, which also means one place to maintain.

sourceCompatibility = JavaVersion.VERSION_17
targetCompatibility = JavaVersion.VERSION_17
}
buildFeatures {

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.

Duplicate buildFeatures block, the first is on line 12. Kotlin DSL applies both to the same extension so it compiles and behaves identically, but it hides the one real difference between them: line 13 sets viewBinding = true, and this second block does not.

That viewBinding = true also looks unused. Both MainActivity.kt files reach for views through findViewById (solution line 92, plus lines 517-616), and nothing in either module imports a generated *Binding type. The starter's build.gradle.kts does not enable it at all, so the two projects differ in a way the codelab never explains.

Suggest merging into the single block below and dropping viewBinding unless a later step is meant to use it:

Suggested change
buildFeatures {
buildFeatures {
compose = true
buildConfig = true
}

lifecycleRuntimeKtx = "2.10.0"

# TODO: Prerequisites - Add the Maps 3D SDK version
# playServicesMaps3d = "0.2.0"

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.

0.2.0 here against playServicesMaps3d = "0.2.2" in the root gradle/libs.versions.toml on main. Same story for agp = "9.1.0" on line 7 versus 9.2.1, and compileSdk = "36" on line 2 versus 37.

Not a correctness problem today, but it is the concrete evidence for the CI point in my top-level comment: this catalogue is pinned to what was current when the branch was cut in April and nothing will tell us when it stops resolving. Dependabot does not see it either, since .github/dependabot.yml would need the two new Gradle directories registered.

Bumping to match main before merge, plus a CI job, would start this off in a maintainable place.

// TODO: Step 1.4 - Map Lifecycle

// TODO: Step 1.5 - Configure the UI buttons
// setupButtons()

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.

The commented call is setupButtons(), but the function it refers to is declared private fun setupButtons(map: GoogleMap3D) on line 225. A learner who uncomments this line as written gets No value passed for parameter 'map'. The solution calls it correctly as setupButtons(googleMap3D) (line 119), from inside onMap3DViewReady rather than from onCreate.

That placement difference matters too: the map is not ready at this point in onCreate, so even the corrected call belongs in the onMap3DViewReady body sketched on line 69, not here.

Also, the step numbering around this is inconsistent and will confuse people navigating by step:

  • Line 63 calls button setup Step 1.5, line 216 calls the same work Step 1.2, and Step 1.2 is already used on line 20 for state management. codelab.md has ### Step 1.2: Project Skeleton, so line 216 looks like the wrong one.
  • HonoluluData.kt labels WAIKIKI as "Step 6" but it is used by the polylines work, which is Step 8 in both codelab.md and line 165 here. It labels IOLANI_PALACE_GEO as "Step 5" while the polygon step is 6.

Worth one pass to make the in-code step labels match the codelab.md headings, since that mapping is the main navigation aid learners have.

*/
class MainActivity : AppCompatActivity() /*, TODO: Step 1.3 - Implement OnMap3DViewReadyCallback */ {

private var fortuneIndex = 0

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.

Nits, batched, all non-blocking:

  • fortuneIndex here is never read or written anywhere in either the starter or the solution. Looks like a leftover from a different sample; it will show up as an unused-property warning in the learner's IDE on first open, which is not the first impression we want.
  • Lines 29-31: companion object { // Constants are now located in HonoluluData.kt } is empty. The comment is useful, the empty companion is not. I would keep the comment as a plain one and delete the block.
  • Line 27 currentAnimationJob is live code but every one of its uses sits inside the commented region at lines 217-303, so it is also unused until the learner reaches Step 1.2. Fine to leave, just noting it is the same warning class.
  • codelab/solution/.../Utilities.kt:8 imports kotlin.math.floor, which the file never uses.

- Added starter architecture with comprehensive TODO curriculm.
- Added full solution module reflecting best-practices.
- Included instructional codelab.md guide covering Camera animations, Popovers, Extrusions and Compose interop.
- Properly excluded build/ and .idea/ artifacts.
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