refactor: externalize SPX module and streamline builds

[joeykchen]

No description provided.

[fennoai]

FennoAI Review — PR #331

Threading runtime_abi and a lock-driven toolchain (scons/emsdk/android_ndk/jdk) through the SPX build workflows, incorporating engine-toolchain-sha256 into cache keys, and restructuring the macOS universal template + web minigame/miniprogram builds to reuse artifacts. The diff is well-constructed: the embedded lock/profile validators are strict (duplicate-key rejection, exact schema/ABI/field-set checks, orchestration-key denylist), producer/consumer artifact names match, cache restore-key chains preserve useful partial hits, and the macOS/web restructures remove genuinely redundant compiles. Two minor findings below (both inline); no blocking issues.

Adjacent observations (non-blocking, not in this diff):

• engine-toolchain-sha256 is hashed from all four tools and embedded in every platform's cache key, so bumping e.g. emsdk will cold-invalidate the Linux/Windows/macOS/iOS/Android SCons caches that don't use emsdk. This over-invalidation is correctness-safe and the tradeoff is documented in the cache-contract comment — noted only for awareness.
• The android_ndk → setup-ndk alias map in setup-spx-module/action.yml currently has a single entry; bumping toolchain.android_ndk in the lock hard-fails until that map is also updated. Worth keeping the coupling discoverable.

[fennoai]

Review summary

This is a large, well-structured refactor that removes the custom SPX module, the OBS/hybrid movie-recorder runtime, the standalone audio recorders/importers, and the bundled lunasvg third-party sources, replacing the SPX lifecycle hooks with a self-contained MainLoopPhaseCallbackBus. The new bus is cleanly designed (single-owner movie lease, snapshot iteration for re-entrancy safety) and comes with a thorough test suite. Most web/JS changes are reverts back toward upstream Godot.

No build-breaking dangling references to the removed subsystems were found (headers, class XML, docs, and web APIs are all consistently cleaned up). The findings below are the concrete, actionable items; inline comments cover diff-line findings.

Performance (main/main_loop_phase_callback_bus.cpp)

The bus is invoked every frame (notify_update / notify_fixed_update from main.cpp), and each dispatch:

• Per-frame heap allocation — _snapshot_registration_ids() allocates and frees a fresh LocalVector<RegistrationID> on every phase, every frame, purely to tolerate re-entrant register/unregister. Consider iterating registrations directly in the common (no-mutation) case, or reusing a clear()-ed member buffer that retains capacity.
• O(N²) dispatch with full struct copy — after snapshotting IDs, each ID is re-found via _get_callbacks (a linear scan) which copies the entire ~72-byte Callbacks struct by value. For each phase this is O(N²) plus a struct copy per subscriber per frame, when the IDs were just read from the same vector. In the non-re-entrant path, iterating registrations directly avoids both the round-trip and the copy.

For the expected tiny registration count these are not functional bugs, but they add avoidable per-frame churn to a hot path; at minimum a comment documenting the intentional simplicity would help future maintainers.

CI hardening (non-blocking)

• .github/actions/godot-build/action.yml (inline) — untrusted ${{ github.event.number }} is interpolated directly into a shell run: block; move it into env: and reference "${PR_NUMBER}" to follow the pattern used elsewhere in the same file. (Only reachable via maintainer-controlled workflow_call/workflow_dispatch, so low practical risk, but it is the injection sink the new SPX build path now depends on.)
• The new SPX web workflows (spx_module_checks.yml, a_depoly_web.yml) reference the third-party action mymindstorm/setup-emsdk@v14 by mutable tag; pinning to a full commit SHA is recommended for supply-chain safety.
• .github/actions/setup-spx-module checks out and compiles code from spx_repository@spx_ref (default goplus/spx@dev, a mutable branch). Since triggers are maintainer-controlled this is by design, but pinning spx_ref to a commit SHA is worth considering for reproducible/trusted builds.

Minor

• main/main_loop_phase_callback_bus.cpp:179-182 — inside the movie_requires_live_audio branch, callbacks_after_policy is re-fetched only to guard the continue on a self-unregistering policy callback; the fetched struct is otherwise unused. This works but reads as incomplete next to the movie_claim path — a short comment clarifying intent would help.
• scene/resources/audio_stream_wav.cpp:36-37 — new file-scope constants TRIM_DB_LIMIT / TRIM_FADE_OUT_FRAMES use generic unprefixed names; consider static constexpr in an anonymous namespace (and -50.0f for the float literal).

Nothing here is blocking. The two clearest code-level items are the WAV importer error swallowing and the JS _playbackPosition default regression (inline).

[fennoai]

import() now discards both results and always returns OK. AudioStreamWAV::load_from_file can return a null/invalid Ref (it ERR_FAIL_COND_Vs on empty file data) and ResourceSaver::save returns an Error that is ignored. The editor will report a successful import even when the WAV failed to load or the .sample failed to write, leaving a broken/empty resource with no diagnostic (the previous code propagated err).

Suggest checking sample.is_null() and returning the ResourceSaver::save result, e.g.:

Ref<AudioStreamWAV> sample = AudioStreamWAV::load_from_file(p_source_file, options);
ERR_FAIL_COND_V(sample.is_null(), ERR_FILE_CANT_READ);
return ResourceSaver::save(sample, p_save_path + ".sample");

Also note r_platform_variants, r_gen_files, and r_metadata are now unused — worth confirming that's intended.

[fennoai]

this._playbackPosition = options.offset; drops the ?? 0 default that the line just above (this.offset = options.offset ?? 0;) and the base version (this._playbackPosition = this.offset;) apply. When options.offset is undefined, _playbackPosition becomes undefined instead of 0, which then flows into getPlaybackPosition() and the this.offset + ... arithmetic elsewhere. Recommend this._playbackPosition = this.offset; to preserve the default.

[fennoai]

res = sw_canvas->clear(true); assigns to res but the function returns OK on the next line, so the result is never checked — unlike every other ThorVG call in this function which uses ERR_FAIL_V_MSG. Either check it or drop the assignment (sw_canvas->clear(true);).