Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SCI: Restart sounds in kDoSoundPlay when already playing #2765

Merged
merged 2 commits into from Feb 13, 2021

Conversation

@sluicebox
Copy link
Member

@sluicebox sluicebox commented Feb 9, 2021

When kDoSoundPlay is called on a sound that's already playing, SSCI would restart that sound, but ScummVM has instead just allowed the existing sound to continue. Not many scripts depend on this but a few do and bugs have built up in the tracker. The codepaths for handling samples vs MIDIs are different but they're in the same function and it causes the same kinds of issues.

Fixed by restarting MIDIs:

  • QFG4 doorbell puzzle not playing the same bell multiple times in a row. The script expects to be able to interrupt and restart the bell. https://bugs.scummvm.org/ticket/12105
  • KQ6CD wallflower dance lockup when playing the flute while the music is still playing doesn't start the song over. If the song doesn't start over, the signals the script waits on never occur. https://bugs.scummvm.org/ticket/10812

Fixed by restarting samples:

  • SQ4CD keypads where pressing keys quickly doesn't play the next tone because the trailing silence from the first one is still playing. There should be a tone for every press as in the original. https://bugs.scummvm.org/ticket/9813

These changes will also cause MIDI sounds in saved games to consistently start from the beginning when restored if they were in the Playing status when saved, as that's also what happens in SSCI. (Update: that was wrong, SSCI has a function for fast forwarding on restore, these changes now preserve that behavior.)

The change to the MIDI logic brings it in sync with the logic a few lines earlier for whether or not to send init commands. Looking at the history surrounding these lines and how they evolved over a lot of little changes and fixes, it doesn't look like it was ever the specific intent to not restart a sound that's already playing. Little things just got moved around in isolation.

This should also take care of the remaining issues that PR #2128 addresses.

I want to do more testing on this but thought I'd get it out here in case someone can poke holes in it right away. I'm intimidated by sound stuff but this particular behavior is easy to verify. It's also easy to see what happens to the things that depend on it.

The sample change is simpler than it looks in the github diff, essentially it just removes the "if (!_pMixer->isSoundHandleActive(pSnd->hCurrentAud))" check.

@sluicebox sluicebox force-pushed the sluicebox:sci-audio-restarts branch from d378baf to c07866f Feb 11, 2021
sluicebox added 2 commits Feb 9, 2021
Fixes KQ6CD wallflower lockup, bug #10812
Fixes QFG4 door bell puzzle, bug #12105
Fixes SQ4CD keypad buttons, bug #9813
@sluicebox sluicebox force-pushed the sluicebox:sci-audio-restarts branch from c07866f to efd7352 Feb 13, 2021
@bluegr
Copy link
Member

@bluegr bluegr commented Feb 13, 2021

Great work! This was previously unhandled behavior in ScummVM. Glad it's solving these bugs. I've tested the aforementioned bugs, and the behavior is correct.

We should check if this change fixes any other sound issues from the tracker. It looks like this won't introduce regressions, but the only way to check this would be via testing, which can happen with the daily builds.

Merging this. Thanks again!

@bluegr bluegr merged commit b0d45d0 into scummvm:master Feb 13, 2021
3 checks passed
3 checks passed
Codacy Static Code Analysis Codacy Static Code Analysis
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deepcode-ci-bot Well done, no issues found!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants