[Bugfix] Sound.play_varied() restores pitch/volume immediately after play(), cancelling the variation #407

New issue

Open

opened 2026-07-27 15:38:16 +00:00 by john · 1 comment

john commented

2026-07-27 15:38:16 +00:00

Owner

Found while working #394.

PySound::py_play_varied applies the randomised pitch/volume, calls play(), then immediately
restores the originals:

    self->data->play();

    // Restore originals (SFML will use the values set at play time)
    // Note: we restore after play() so the variation applies only to this instance
    self->data->setPitch(origPitch);
    self->data->setVolume(origVolume);

The comment is wrong for the OpenAL backend. sf::Sound::setPitch / setVolume map to
alSourcef(AL_PITCH / AL_GAIN), which apply live to an already-playing source — they are not
sampled once at play(). So the restore un-varies the playback essentially instantly.

Corroborated: while sound.playing is True immediately after play_varied(), sound.pitch reads
back the original value, i.e. the live source pitch is the original, not the drawn one.

If that is right, play_varied() has never audibly varied anything, and the feature is a no-op with
a plausible-looking implementation. Worth confirming by ear before choosing a fix; the fix is
probably to stop restoring (and document that play_varied leaves the varied values on the object),
or to hold the variation for the lifetime of the playback.

Related but separate: as of #394 play_varied() returns the (pitch, volume) it drew, so the draw
is at least observable from Python now — that is what made it testable. This issue is about whether
the draw ever reaches the speakers.

Found while working #394. `PySound::py_play_varied` applies the randomised pitch/volume, calls `play()`, then immediately restores the originals: ```cpp self->data->play(); // Restore originals (SFML will use the values set at play time) // Note: we restore after play() so the variation applies only to this instance self->data->setPitch(origPitch); self->data->setVolume(origVolume); ``` **The comment is wrong for the OpenAL backend.** `sf::Sound::setPitch` / `setVolume` map to `alSourcef(AL_PITCH / AL_GAIN)`, which apply *live* to an already-playing source — they are not sampled once at `play()`. So the restore un-varies the playback essentially instantly. Corroborated: while `sound.playing` is `True` immediately after `play_varied()`, `sound.pitch` reads back the original value, i.e. the live source pitch is the original, not the drawn one. If that is right, `play_varied()` has never audibly varied anything, and the feature is a no-op with a plausible-looking implementation. Worth confirming by ear before choosing a fix; the fix is probably to stop restoring (and document that `play_varied` leaves the varied values on the object), or to hold the variation for the lifetime of the playback. Related but separate: as of #394 `play_varied()` returns the `(pitch, volume)` it drew, so the draw is at least observable from Python now — that is what made it testable. This issue is about whether the draw ever reaches the speakers.

john added the

Bugfix

priority:tier2-foundation

Tiny Feature

labels

2026-07-27 15:38:16 +00:00

john commented

2026-07-27 15:52:57 +00:00

Author

Owner

Reviewer note from the #394 review (commit ac29b35), for whoever picks this up.

A mutation that makes py_play_varied compute the varied pitch/volume, return them, but apply
the originals to the source before play() survives all three of #394's test files green:

R1_play_varied_applies_original   SURVIVES   unit=0/0  audio=0/0  integ=0/0

So the returned tuple is gated as the values that were drawn — which is exactly what its docstring
claims and what #394 needed — but nothing anywhere gates that those values were the ones actually
handed to the source. That is not a defect in #394 (the draw is what it set out to make observable),
but it is the same hole this issue is about, one layer up: today play_varied could stop varying
anything at all and no test would notice.

When this is fixed, the fix should come with an assertion that closes that: after play_varied()
returns, while sound.playing is True, sound.pitch / sound.volume should read back the
drawn values rather than the originals. That assertion is only writable once the restore is
removed, which is what makes it the right gate to add here rather than in #394.

Reviewer note from the #394 review (commit `ac29b35`), for whoever picks this up. A mutation that makes `py_play_varied` compute the varied pitch/volume, **return them**, but apply the *originals* to the source before `play()` survives all three of #394's test files green: ``` R1_play_varied_applies_original SURVIVES unit=0/0 audio=0/0 integ=0/0 ``` So the returned tuple is gated as *the values that were drawn* — which is exactly what its docstring claims and what #394 needed — but nothing anywhere gates that those values were the ones actually handed to the source. That is not a defect in #394 (the draw is what it set out to make observable), but it is the same hole this issue is about, one layer up: today `play_varied` could stop varying anything at all and no test would notice. When this is fixed, the fix should come with an assertion that closes that: after `play_varied()` returns, while `sound.playing` is `True`, `sound.pitch` / `sound.volume` should read back the **drawn** values rather than the originals. That assertion is only writable once the restore is removed, which is what makes it the right gate to add here rather than in #394.

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

john/McRogueFace#407

No description provided.

Rows
Columns