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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

SCUMM: COMI: Fix #4635/#10473 and #4391 (iMUSE Digital) #2596

Merged
merged 7 commits into from Nov 12, 2020

Conversation

@AndywinXp
Copy link
Contributor

@AndywinXp AndywinXp commented Nov 4, 2020

SCUMM: COMI: Fix iMUSE Digital #4635/#10473 bugs

This commit fixes the early stopping of ArriveBarber sequence, during the first arrival in Barbery Coast.
If this particular cutscene is skipped while this sequence is playing (e.g.: via ESCAPE key), the sequence is faded out
and the main Barbery Coast theme is played, as per correct behavior.
That's also the only COMI sequence using a transitionType with value 4, so the fix should not be able to break anything else, and in fact I haven't been able to break anything during my test run.

SCUMM: COMI: Fix #4391 iMUSE Digital fades

This commit implements fade-ins during song transitions (e.g. statePreVooOut --> statePreVooIn --> statePreVooLady) and also between regions of the same song (e.g. any JUMP instruction used for looping songs).

This results in a linear cross-fade. This is not ideal since the correct thing would be to implement a logarithmic crossfade (or "audio taper"), but for now this will do without changing too much of the current codebase. I'm still going to investigate the possibility of implementing this in a correct way.

I know, I know the implementation looks terrible, a "real" fix would supposedly use fade tracks also for fade-ins, and require some kind of cloneToFadeInTrack(track, fadeDelay) to be used together with cloneToFadeOutTrack.

I didn't do this for two reasons:

  • I think this would create some desync problems between fade tracks and proper tracks (for the life of me I just can't get fade out tracks to always be in perfect sync with the "parent" track).
  • I would have to delaying putting the new song in the parent track until the correspondent fade in has occurred, and I don't know how to do it.

Having said that, this fix unfortunately does not account for the sync issue between fade-out and parent track, but it already sounds much better than the current implementation (even in the 30% of times in which the fade-outs happen not to be in sync). And besides, these bugs have remained unfixed for more than 10 years, so at this point I doubt anyone else has a plan to have a proper attempt at this anytime soon.

There might be another solution, for sure, but it's currently beyond my knowledge, I'm sorry 馃槩

@aquadran
Copy link
Member

@aquadran aquadran commented Nov 8, 2020

some changes are risky, and may also break The Dig

@AndywinXp
Copy link
Contributor Author

@AndywinXp AndywinXp commented Nov 8, 2020

some changes are risky, and may also break The Dig

Thanks for the answer, I would love to improve the solution (also, I'm working on an alternate version of cloneToFadeOutTrack right know, I hope my idea works).
Could you please elaborate on why these are risky, and what could break The Dig? Expecially about commit 769971d.

@aquadran
Copy link
Member

@aquadran aquadran commented Nov 8, 2020

you are changing logic in fades, that may break The Dig. I suggest to separate flow only for COMI

@AndywinXp
Copy link
Contributor Author

@AndywinXp AndywinXp commented Nov 8, 2020

you are changing logic in fades, that may break The Dig. I suggest to separate flow only for COMI

Thanks for the suggestion, I will correct my code accordingly here and also in the changes I haven't commited yet.

@AndywinXp
Copy link
Contributor Author

@AndywinXp AndywinXp commented Nov 10, 2020

Changes introduced in the commit above (all exclusive to COMI):

  • Implemented audio crossfading (with equal power fades! So no more linear fading curves, but quarter sine ones) for transitions;
  • Fixed music timing issues for both crossfades and JUMP instructions;
  • Removed most of cloneToFadeOutTracks calls for the game, since they're not used in transition or in region switches (I couldn't get them to play on sync no matter what): as a result the loops in the music tracks are seamless now.

What's left to do?

  • The loops and the region jumps are seamless and in sync, but in the original interpreter there is crossfading between the previous region and the new region. I couldn't get the crossfade to work properly by fading out the current track, flushing it afterwards, and starting a new track with the new region fading in: it kept spitting out exceptions about the region offset being either negative or bigger than the region itself;
  • There is a strange music bug involving the end of the cannon minigame in the first part of the game: after destroying the last boat, the current track (stateCannon) jumps to a particular region of the song (by calling seqLastBoat) and then SEQ_NULL is immediately sent, effectively fading out and ending the song. After that, the game calls for stateHold2 with a hook for the JUMP with id 1. The intended effect is to start the song from the beginning (region 1 I guess?) and use the JUMP to skip to a farther region of the song. This almost works: the jump is done correctly but the song is started from the very beginning of the file (region 0, which is silent!), leaving everything silent until region 1 starts. This bug has been here for ages, if I recall correctly.

I'll be sure to send a ticket in the issue tracker about the last point, and I won't work both of these points for now, I'd just love to get these current fixes in the master, for now.

@deepcode-ci-bot
Copy link

@deepcode-ci-bot deepcode-ci-bot bot commented Nov 10, 2020

DeepCode failed to analyze this pull request

Something went wrong despite trying multiple times, sorry about that.
Please comment this pull request with "Retry DeepCode" to manually retry, or contact us so that a human can look into the issue.

鈥 dimuse_track.cpp
@aquadran aquadran merged commit 924a62b into scummvm:master Nov 12, 2020
1 of 2 checks passed
1 of 2 checks passed
deepcode-ci-bot Analysis failed. Please comment "Retry DeepCode" or contact us:
Details
continuous-integration/travis-ci/pr The Travis CI build passed
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
You can鈥檛 perform that action at this time.