Skip to content

Fix the lifecycle, audio, and renderer issues from the full-app review - #31

Merged
jpcottin merged 1 commit into
mainfrom
review-fixes
Sep 4, 2026
Merged

Fix the lifecycle, audio, and renderer issues from the full-app review#31
jpcottin merged 1 commit into
mainfrom
review-fixes

Conversation

@jpcottin

@jpcottin jpcottin commented Sep 4, 2026

Copy link
Copy Markdown
Owner

Same treatment as VulkanAsteroids #17: a whole-app review (game logic, renderer and glasses bridge, audio, tests, build, CI), everything actionable fixed, then a second review pass on the diff itself.

This port had already absorbed most of the Asteroids findings (focus pause, per-image semaphores, dt-scaled decay, death-frame gating, a real smoke assertion), so the list is shorter and skews toward failure paths.

Correctness

  • RNG seed was undefined behaviour: uptime × 1e6 cast to uint32 overflows after ~72 minutes and saturated to a constant seed on AArch64, so every launch replayed the same drops and timings. Seeded from the clock's integer fields instead.
  • Music: a process-death restore left music off (the audio engine arrives after the round starts), a settings reload never re-applied the sound toggle, and the new sync rule initially ignored the glasses handoff. One syncMusic() rule now covers all of it.
  • Scoring after the table was written: the winning volley could still kill escorts after the boss; a tie with an existing row could duplicate it and is no longer flagged as a new record.
  • Title reset: invulnerability and the boss were not cleared, so the title ship blinked forever and the mothership stayed drawn under Settings. Shared resetBattlefield() with startLevel().
  • Renderer failure paths: a failed surface recreate left the next frame on a null surface; a half-built swapchain was trusted; a failed submit left a fence that would hang the next relaunch; DEVICE_LOST busy-looped. All now rebuild device state through a recovery timer with backoff, and preTransform is IDENTITY.
  • Audio: error-callback reopen synchronised with shutdown and the implementation held by shared pointer; native sample rate; pause on focus loss; wrapping music phases; soft clip and lower explosion gains.
  • JNI: local references leaked on every bridge call (the game thread never returns to Java); a failed lookup was retried every second.
  • Atomic saves with fsync, scores drawn as long, draw-command vector reused, per-engine FPS counters.

Tests, build, CI

  • Boss bomb test now asserts the bomb is aimed; win test asserts table equals final score; new tests for the title reset and the tie case.
  • Gating jobs get timeouts; the one NDK install without pipefail gets it; Oboe pinned to a hashed tarball; -Wall -Wextra on both native targets (clean); runNativeTests finds the newest binary; backup rules wired; missing proguard file reference dropped; repo .clang-tidy.
  • Replay script keeps the emulator process-group id while alive (KILL fallback) and no longer relaunches the app after run 1, matching the CI job it mirrors.

Behaviour changes worth knowing

  • Oboe sharing mode is now Shared (was Exclusive).
  • Audio pauses while the phone activity is unfocused.

Not changed

  • GitHub Actions still pinned to major tags; the Android CLI installer is still fetched unpinned.

Verified

  • 105 native tests pass on the MP36 AVD (arm64).
  • On MP36: title, play at 60 fps, shade pause, Home + relaunch; no Vulkan or Oboe warnings in logcat; stream opens at 48 kHz.

Gameplay / lifecycle
- Seed the RNG from the clock's integer fields: converting uptime * 1e6 to
  uint32_t is undefined past ~72 minutes and saturated to a constant seed.
- One music rule (syncMusic): plays from round start through the end
  screen when sound is on, never on the phone while the round runs on the
  glasses, and is re-applied when the audio engine arrives after a
  process-death restore, on settings reload, and on every state change.
- Stop the winning volley at the boss kill so nothing scores after the
  high-score table is written; a tie with an existing row is no longer
  celebrated as a new record and can no longer duplicate the row.
- Reset invulnerability and the boss on the return to the title (the title
  ship blinked forever; the mothership stayed drawn under Settings), via a
  shared resetBattlefield() used by startLevel() too.
- Atomic saves (temp file + fsync + rename) and scores drawn as long.

Audio
- Impl is shared so Oboe's error thread can never outlive it; the reopen
  after a disconnect is synchronised with shutdown and opens outside the
  lock. Shared mode at the device's native rate, requestStart checked.
- pause()/resume() on focus loss/gain (phone role only).
- Wrapping music phases (the bass line froze after ~8 minutes), soft clip,
  lower explosion gains.

Renderer
- A failed surface recreate no longer leaves the next frame running against
  a null surface; a half-built swapchain is torn down; every acquire /
  submit / present error rebuilds device-level state instead of leaving a
  reset fence that hangs the next relaunch.
- IDENTITY pre-transform (content was drawn sideways on landscape-native
  panels), surface-capability and swapchain-image results checked.
- Recovery timer with backoff in the main loop; the activity finishes only
  when the GPU is genuinely unsupported or the rebuild keeps failing.
- Draw-command vector reused across frames; FPS counters per engine (the
  phone and glasses instances share the process).

Glasses bridge
- Delete every JNI local reference (the game thread never returns to Java)
  and cache a failed bridge lookup instead of retrying it every second.

Tests / build / CI
- Boss bomb test asserts the bomb is aimed; win test asserts the table
  matches the final score; new tests for the title reset and the tie case.
- Gating jobs get timeouts; the one NDK install step without pipefail gets
  it. Oboe pinned to a hashed tarball, -Wall -Wextra on both native targets,
  runNativeTests finds the newest binary, backup rules wired, missing
  proguard file reference dropped, repo .clang-tidy.
- replay-preview-multirun.sh keeps EMU_PGID while the emulator is alive
  (with a KILL fallback) and no longer relaunches the app after run 1, so
  it measures what the CI job measures.

Verified: 105 native tests pass on the MP36 AVD; on MP36 the title, play at
60 fps, shade pause, and Home + relaunch all work with no Vulkan or Oboe
warnings in logcat, and the stream opens at the native 48 kHz.
@jpcottin
jpcottin merged commit 3194e2d into main Sep 4, 2026
14 checks passed
@jpcottin
jpcottin deleted the review-fixes branch September 4, 2026 03:22
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.

1 participant