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

Prevent PlaySound overlapping #1692

Merged
merged 2 commits into from May 15, 2018

Conversation

Projects
None yet
3 participants
@akortunov
Copy link
Collaborator

akortunov commented May 1, 2018

If we play a new sound, we should stop the previous copy of this sound. This is easily to check: execute "playsound X" several times in console in vanilla game.
We already use similar approach for 3D sounds: we allow to play only one copy of given sound per object, so this PR just makes sound playing consistent.

As a side effect, this PR fixes a nightmare scene in Arktwend, so it should not crash now.

@psi29a

This comment has been minimized.

Copy link
Member

psi29a commented May 6, 2018

I'm OK with this, but it touches on sound so i'll defer to @kcat

return;

sndmgr->playSound(soundId, volume, pitch, MWSound::Type::Sfx, MWSound::PlayMode::NoEnv);
MWBase::Environment::get().getSoundManager()->playSound(soundId, volume, pitch, MWSound::Type::Sfx, MWSound::PlayMode::NoEnv);

This comment has been minimized.

Copy link
@kcat

kcat May 7, 2018

Contributor

Won't this change now cause the UI sound to get cutoff if it's played again while already playing, rather than the apparently intended behavior of letting the original sound finish without interruption?

This comment has been minimized.

Copy link
@akortunov

akortunov May 7, 2018

Author Collaborator

Should we treat GUI sounds differently from other ones?

This comment has been minimized.

Copy link
@psi29a

psi29a May 7, 2018

Member

How much work would it be to treat GUI sounds differently?

This comment has been minimized.

Copy link
@akortunov

akortunov May 7, 2018

Author Collaborator

How much work would it be to treat GUI sounds differently?

Almost none. We just should decide which behaviour is intended:

  1. Allow to stack most of GUI sounds, with special case for journal wheel scrolling to prevent overlapping.
    It is our current upstream implementation.
  2. Play old copy to end and do not launch new copy to prevent overlapping.
  3. Take the PR implementation.

Honestly, I can not hear much difference between all three variants (excepts of loudness in first case), so I can not advice which behaviour is better.

Also keep in mind that too much playing sounds can lead to crash on Windows.

This comment has been minimized.

Copy link
@psi29a
@kcat

This comment has been minimized.

Copy link
Contributor

kcat commented May 7, 2018

My only other real complaint is that it basically does loadSound(Misc::StringUtils::lowerCase(soundId)); twice when playing a sound now. First for playSound, then again in stopSound.

@psi29a

This comment has been minimized.

Copy link
Member

psi29a commented May 7, 2018

So you would rather have a temp variable to save another cycle? RAM's cheap, CPU is not. :)

@akortunov

This comment has been minimized.

Copy link
Collaborator Author

akortunov commented May 7, 2018

My only other real complaint is that it basically does loadSound(Misc::StringUtils::lowerCase(soundId)); twice when playing a sound now. First for playSound, then again in stopSound.

How do you suggest to avoid it?

@akortunov

This comment has been minimized.

Copy link
Collaborator Author

akortunov commented May 7, 2018

I added a new stopSound function, which accept Sound_Buffer.

@akortunov

This comment has been minimized.

Copy link
Collaborator Author

akortunov commented May 15, 2018

My only other real complaint is that it basically does loadSound(Misc::StringUtils::lowerCase(soundId)); twice when playing a sound now. First for playSound, then again in stopSound.

Should be fixed now. Any other issues?

@psi29a

This comment has been minimized.

Copy link
Member

psi29a commented May 15, 2018

@kcat mergable? :)

@kcat

This comment has been minimized.

Copy link
Contributor

kcat commented May 15, 2018

Looks alright at a glance. Assuming it works and no one else has objections, go for it.

@psi29a psi29a merged commit c75d774 into OpenMW:master May 15, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@psi29a

This comment has been minimized.

Copy link
Member

psi29a commented May 15, 2018

Merged, this should make Arktwend fans happy. :)
Thanks for the PR!

@akortunov akortunov deleted the akortunov:playsound branch Jun 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.