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: fix #10907, #10812 and fix music position on restore #2128

Closed
wants to merge 3 commits into from

Conversation

@ZvikaZ
Copy link
Contributor

@ZvikaZ ZvikaZ commented Mar 8, 2020

  • Reset 'ticker' when playing again an ongoing audio resource (bug #10812)
  • When restoring, play the actual audio resource that was playing, and not the audio resource that was initted, but not played yet (bug #10907)
  • When restoring, continue from the same music position (unreported bug)
@ZvikaZ ZvikaZ force-pushed the ZvikaZ:z_restore_music_sq4 branch from 294b289 to d2b8645 Mar 8, 2020
@ZvikaZ ZvikaZ changed the title SCI: fix #10907 - Wrong music when restoring games SCI: fix #10907, #10812 and fix music position on restore Mar 9, 2020
@ZvikaZ ZvikaZ force-pushed the ZvikaZ:z_restore_music_sq4 branch from dfe51eb to 64dc51a Mar 9, 2020
@@ -467,6 +467,9 @@ void SciMusic::soundInitSnd(MusicEntry *pSnd) {
pSnd->overridePriority = false;

pSnd->pMidiParser->loadMusic(track, pSnd, channelFilterMask, _soundVersion);

// When restoring, continue from the correct place
pSnd->pMidiParser->jumpToTick(pSnd->ticker);

This comment has been minimized.

@bluegr

bluegr Mar 15, 2020
Member

Why? We already do this when loading. Also, all the music events need to run when restoring (like instrument changes), otherwise music will sound distorted after loading

This comment has been minimized.

@ZvikaZ

ZvikaZ Mar 16, 2020
Author Contributor

Why?
Currently, when we save a game while a sound track is playing, and later restore it, the track is starting from the beginning. It's easy to see that in the scenerio of https://bugs.scummvm.org/ticket/10812 - KQ6 "wallflower dance music" - if you save the game few seconds before the music stops, and then restore, it starts from the beginning.

You're correct that sci\engine\savegame.cpp MusicEntry::saveLoadWithSerializer has s.syncAsSint16LE(ticker) , but it's not enough, as in sci\sound\music.cpp MusicEntry::onTimer() there is ticker = (uint16)pMidiParser->getTick(); which overrides it, to zero, and thus the track starts from the beginning.

Therefore, calling jumpToTick(pSnd->ticker) fixes it.

However, I changed it to be pSnd->pMidiParser->jumpToTick(pSnd->ticker, true, true, true); , which (if I understand correctly) should acheive exactly that - fast forward to the correct place, executing all midi controllers in the way, without playing music.
I copied this line, and its comment, from music.cpp, from near the end of SciMusic::soundPlay (line 609 in my version)

This comment has been minimized.

@ZvikaZ

ZvikaZ Apr 18, 2020
Author Contributor

Hi.
@bluegr , any news?

@ZvikaZ ZvikaZ force-pushed the ZvikaZ:z_restore_music_sq4 branch from 64dc51a to ef12a2b Mar 16, 2020
- Reset 'ticker' when playing again an existing resource (bug #10812)
- When restoring, continue from the same music position
@ZvikaZ ZvikaZ force-pushed the ZvikaZ:z_restore_music_sq4 branch from ef12a2b to 5aa2a8f Mar 16, 2020
@ZvikaZ ZvikaZ requested a review from bluegr Mar 23, 2020
@bluegr
Copy link
Member

@bluegr bluegr commented May 24, 2020

Nice work! :)
The fix for bug 10907 is quite straightforward, and has been cherry picked and committed. I would like to have a second look at the fix for bug 10812 before committing it.

In the future, please try and create separate pull requests for different bugs, if possible

@sev-
Copy link
Member

@sev- sev- commented Aug 24, 2020

@bluegr any update?

@bluegr
Copy link
Member

@bluegr bluegr commented Feb 13, 2021

The second fix has been redone in PR #2765. Closing this one, as the implementation in the other PR is less invasive. Thanks for your work once again! :)

@bluegr bluegr closed this Feb 13, 2021
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

3 participants