Skip to content

fix(audio): initialize spatial parameters before sample playback - #334

Merged
joeykchen merged 2 commits into
goplus:spx4.4.1from
joeykchen:fix/positional-sample-startup
Sep 12, 2026
Merged

joeykchen merged 2 commits into
goplus:spx4.4.1from
joeykchen:fix/positional-sample-startup

Conversation

@joeykchen

Copy link
Copy Markdown

Short positional samples could finish before the first spatial volume update, producing no audible output on Web. Queue 2D/3D playback requests and start samples after their gains, bus routing, and pitch are initialized.

Share pending playback handling for pause, seek, stop, and polyphony limits. Preserve complete 3D bus routing and initialize pitch from the current configuration when no valid listener is available or the source is out of range.

Validation:

  • Web and macOS editor builds passed.
  • Safari short-sound checks and all five playback lifecycle checks passed.
  • Added seven engine regression tests; these have not been run.

Initialize spatial gains, bus routing and pitch before starting 2D and 3D samples. Share pending playback handling for pause, seek, cancellation and polyphony, and cover startup and lifecycle boundaries with regression tests.

Validated with a Web build, Safari audio capture and a macOS editor build. Native audio tests remain for manual validation.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review: fix/positional-sample-startup

This refactor is well-conceived and cleanly executed. Sample playbacks for AudioStreamPlayer2D/3D are now queued and started during NOTIFICATION_INTERNAL_PHYSICS_PROCESS with up-to-date spatial parameters (bus/volume/pitch), replacing the ad-hoc setplay/setplayback members with a shared pending_playbacks queue in AudioStreamPlayerInternal. Notable strengths:

  • Reentrancy is handled deliberately: the pending list is snapshotted before iterating and the index is re-validated after start_playback_stream.
  • The AudioStreamPlayback ↔ AudioSamplePlayback reference cycle is broken via set_sample_playback(nullptr) on the clear/evict paths, with direct test coverage.
  • Skipping pending voices in process() prevents never-started voices from being misread as "finished".
  • The shared 1D AudioStreamPlayer never populates the pending queue, so the modified internal remains backward-compatible for it (verified).
  • The deterministic SampleDriver test fixture directly asserts start offset/bus/pitch/volume — good coverage of the core goal.

Findings below, ordered by priority. Nothing here is blocking; the inline items are worth a look before merge.

Additional notes (no reliable inline anchor):

  • 3D _update_panning multi-listener routing (scene/3d/audio_stream_player_3d.cpp:452): output_bus_volumes = bus_volumes; is reassigned inside the per-listener loop, so with multiple 3D listeners the pending sample's initial route uses only the last listener's volumes. This mirrors the last-wins behavior already applied to started voices, so it's likely acceptable, but the multi-listener case is untested.
  • Per-frame HashMap allocation (scene/3d/audio_stream_player_3d.cpp:321-329): _update_panning now builds and returns a HashMap (with StringName hashing) every physics frame for every active 3D player, even in steady state when nothing is pending and the returned map is discarded. Bounded work, but an avoidable per-frame allocation on the audio hot path — consider building the map only when has_pending_playback() is true.
  • Doc polish (doc/classes/AudioStreamPlayer2D.xml / AudioStreamPlayer3D.xml, get_playback_position): now returns the queued start position while a sample is pending; the play/playing docs were correctly updated, but get_playback_position still reads only "Returns the position in the [AudioStream]."

Comment thread scene/audio/audio_stream_player_internal.cpp Outdated
Comment thread scene/audio/audio_stream_player_internal.cpp
Comment thread scene/audio/audio_stream_player_internal.cpp
Revalidate pending voices after driver callbacks and stop samples cancelled during startup. Collect initial 3D bus volumes only for pending playback, preserving routing and seek behavior.

Add regression coverage for reentrant startup, bus updates and polyphonic seek, and clarify queued playback positions. Formatting, C++ syntax and XML checks passed; native tests have not been run.
@joeykchen
joeykchen merged commit 526329b into goplus:spx4.4.1 Sep 12, 2026
18 checks passed
@joeykchen
joeykchen deleted the fix/positional-sample-startup branch September 20, 2026 13:06
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.

4 participants