fix(sound): share pan sampling during playback

[martin-henz]

Re-opens #838, which was automatically closed (not merged) when its base branch conductor-migration was deleted after being merged into master via #680. This branch cherry-picks the original commits from @11suixing11 unchanged, onto current master.

Summary

• sample pan outputs once per timestamp and derive both PCM channels from that shared sample
• sample both the source and pan modulator once per timestamp for pan_mod
• preserve the internal shared stereo sampler when a Sound crosses the Conductor pair/array boundary, keyed by evaluator, stable wave closure IDs, and duration
• keep the public Sound representation and left/right Wave accessors unchanged

Why the boundary handling matters

The pure functions.ts optimisation alone is lost in normal Source usage: a transformed Sound is encoded as [[left_wave, right_wave], duration] and decoded again by play. TypedValue and pair wrappers are recreated during the Python round-trip, so the sampler metadata is retained using evaluator-scoped closure IDs instead of object identity. A modified Sound does not match the cache and falls back to the existing per-wave sampling path.

Testing (from original PR, re-verify on CI)

• counted generator-backed waves verify one source sample per timestamp for play(pan(...))
• counted source and modulator waves verify one sample each per timestamp for play(pan_mod(...))
• Conductor adapter tests verify fresh closure wrappers restore metadata without leaking across durations, closure pairs, or evaluators
• yarn workspace @sourceacademy/bundle-sound test (68 passed)
• yarn workspace @sourceacademy/bundle-sound tsc
• yarn workspace @sourceacademy/bundle-sound lint

Fixes #800

Original author: @11suixing11

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[martin-henz]

Reviewed the diff and checked out the branch to run the suite directly. This is a solid, well-motivated fix with good test coverage — one design question worth a second pair of eyes, no blockers.

The problem, as I traced it: pan/pan_mod build left/right channel Waves that both close over the same source wave (gainWave(wave, leftGain) and gainWave(wave, rightGain) in pan, similarly for pan_mod's amount/source waves). play() used to sample the left and right Waves independently via two separate sampleWave() calls, so the shared source wave got invoked twice per timestamp. For a pure deterministic wave that's just wasted compute, but per the comment in functions.ts, a Source-level student-defined wave threads all the way up through the CSE machine on each call — so this was doubling real interpreter work, and for any wave with actual side effects or non-determinism it would silently produce diverging left/right channels. The new countedWave tests demonstrate this directly (asserting the source is sampled exactly once per timestamp for both pan and pan_mod).

Why the boundary-crossing half is necessary, not just the pure fix: I confirmed this isn't hypothetical — under Conductor, pan(amount)(sound) and play(...) are both @moduleMethod-decorated (confirmed via index.ts:446+), so every real Source-level call like play(pan(0.5)(some_sound)) marshals the intermediate Sound out to [[left_wave, right_wave], duration] and back through conductorToSound/soundToConductor — exactly the round trip described in the PR body. So without conductorAdapters.ts, the functions.ts-level fix alone would be silently lost the moment a student actually calls pan from Source code, which is the normal case. Good catch, and the PR doesn't stop at the easy half of the fix.

conductorAdapters.ts cache design: WeakMap<evaluator, Map<leftClosureId, Map<rightClosureId, Map<duration, sampler>>>>, keyed by the closures' numeric Identifier (confirmed ClosureIdentifier is a branded number in @sourceacademy/conductor's types, so it's a stable, valid Map key — not object identity, which the comment correctly notes wouldn't survive the round trip). Scoping by evaluator via WeakMap means the whole cache is naturally cleaned up when a run's evaluator is GC'd. The "why this file is separate from index.ts" rationale (Vitest's transform doesn't handle this bundle's experimentalDecorators-based @moduleMethod decorators) checks out — index.ts does use @moduleMethod(...) decorators, so keeping the undecorated cache logic testable in isolation is a reasonable call, not incidental churn.

One design question, not a blocker: the cache keys on closure IDs + duration, scoped to one evaluator/run, with no explicit invalidation — entries just accumulate for the run's lifetime and are matched by (leftId, rightId, duration). That's fine if a given evaluator never reuses a closure ID for an unrelated closure within the same run. I couldn't verify that guarantee from this repo (the ID allocator lives in js-slang/py-slang, not here) — if IDs are ever recycled after GC within a run, a stale sampler could theoretically get matched to an unrelated Sound that happens to land on the same (id, id, duration) tuple. Worth a quick confirmation from whoever owns the evaluator-side ID allocation, but I don't think it should hold up this PR — the existing conductorAdapters.test.ts already covers the cases that matter for this bundle's own contract (no leakage across duration, closure pair, or evaluator).

Verified locally (checked out replace-pr-838):

• yarn workspace @sourceacademy/bundle-sound test — 68/68 passing, matches the PR's claim
• yarn workspace @sourceacademy/bundle-sound tsc — clean
• yarn workspace @sourceacademy/bundle-sound lint — same 6 pre-existing warnings present on master too (an environment-dependent @sourceacademy/throw-runtime-error rule that's off outside CI), nothing new from this PR

Minor nit, not worth blocking on: in both pan and pan_mod, the SoundSampler closures are written as duration => samplePannedChannels(wave, duration, clamped), where the parameter shadows the outer duration already captured by the closure. Since sampleSound() always invokes sound.sampleChannels(sound.duration) and sound.duration is set to that same outer duration by make_stereo_sound_with_sampler, the parameter is always redundant with the closed-over value — harmless, just a bit of unnecessary indirection.

No changes requested from me — the core fix is correct, well-tested, and the harder boundary-crossing half is handled thoughtfully.

[martin-henz]

Good work @11suixing11 . Thanks for contributing.