#1063 closed defect (fixed)
DOTT: Hanging notes (regression?)
| Reported by: | eriktorbjorn | Owned by: | SF/jamieson630 |
|---|---|---|---|
| Priority: | high | Component: | Engine: SCUMM |
| Version: | Keywords: | ||
| Cc: | Game: | Day of the Tentacle |
Description
[Extracted from bug report #775097]
English CD "talkie" version.
At the end of the game, when the kids are hiding from purple tentacle, the music changes. When it does, some notes are usually (always?) left hanging. The same thing also happens sometimes when I push nurse Edna out of her room, but that's harder for me to reproduce.
I've look a bit closer at the first of these cases, and I think it must have something to do with the "smart jumping" logic in the MIDI parser, because if I disable this property I no longer get any hanging notes.
From what I understand, smart jumping means that instead of silencing all notes at once and then jump to a new position, it leaves the current notes to fade out on their own, thus allowing for a smoother transition. This involves scanning the track for note off events or the end of the track, to see when the notes would have been turned off.
I don't know what the unit of time measurement is, so I don't know what are reasonable values and what aren't, but when the track loops the time_left parameter is somewhere around 10000, and it only has to scan a few events ahead to figure that out.
Which makes the following calls to hangingNote(), which I captured during one test run, look rather suspicious to me:
hangingNote(11, 67, 8099928, 0) hangingNote(11, 71, 8615520, 0) hangingNote(11, 74, 16625052, 0) hangingNote(1, 35, 72028872, 0) hangingNote(2, 67, 78392304, 0) hangingNote(6, 67, 104966496, 0)
Not only that, but it had to scan hundreds - maybe thousands - of events ahead to find them.
Ticket imported from: #775654. Ticket imported from: bugs/1063.
Attachments (3)
Change History (16)
by , 21 years ago
| Attachment: | tentacle.s32.gz added |
|---|
comment:1 by , 21 years ago
| Owner: | set to |
|---|
comment:2 by , 21 years ago
What I'm wondering is, is "smart jump" a necessary feature for iMUSE, or is it just icing on the cake? If it's the latter, then one solution (albeit a rather heavy-handed one) would be to simply disable it for 0.5.0.
Or maybe the bug is easy to fix and I'm just worrying over nothing.
comment:3 by , 21 years ago
The functionality now called Smart Jump has been implemented in the earliest versions of ScummVM as "sustaining notes". Eliminating it does produce a noticeable difference during interactive jumps in the music (usually it sounds a bit "thin" during the jump, because too many instruments suddenly go silent at once). However, I agree that if the current Smart Jump code is still producing hanging notes, disabling it might be preferable for the release.
The numbers in your data spew are intriguing. I hope to address this one soon. (Computer emergencies are keeping me occupied right now.) Any problems with Smart Jump are of substantial concern to me.
comment:4 by , 21 years ago
| Priority: | normal → high |
|---|
comment:5 by , 21 years ago
A less drastic hack might be to clamp the time_left value to something sensible.
By the way, it seems to happen during the Sam & Max intro as well, but it's much less noticeable there. The DOTT case is a bit extreme in that respect I guess, since it changes from a quite loud track to a quite soft one.
comment:6 by , 21 years ago
Is it really enough for hangAllActiveNotes() to just check for note off and end of track events? The "note on with velocity 0" case seems to be covered by midiparser_smf.cpp silently transforming it into a "note off" event, but what about controller events?
On the other hand, my attempt to add checks for "all notes off" and "all sounds off" had absolutely no effect at all, so maybe I'm just spouting nonsense...
comment:7 by , 21 years ago
Can we get a solution on this soon? I wanna be able to finalise 0.5.0 this week...
comment:8 by , 21 years ago
Give me a drop-dead date, Ender. I'm in the middle of an emergency and won't even be able to look at this in depth until Tuesday at the earliest. If a solution can't be delivered by the drop-dead date of your choice, I recommend going with eriktorbjorn's suggestion that we disable SmartJump altogether for 0.5.0.
eriktorbjorn: Controller events shouldn't be a problem because they were also not accounted for in the original Sustaining Notes code. That code also just looked for Note Off and End of Track events. (It also looked for Note On events under some very strange and convoluted circumstances.) As to why it would end up scanning thousands of events and coming up with such outrageously large sustaining times (which are in microseconds, BTW), I could not begin to postulate on until I've actually traced through the code at the problem area.
comment:9 by , 21 years ago
| Resolution: | → fixed |
|---|
comment:10 by , 21 years ago
I love it. As soon as I post that I don't have time, I find a little window in which to do some debugging.
I have committed a fix to the trunk that takes care of the hanging notes when Purple Tentacle is chasing the Three-In- One Monster. The regression occurred when I modified the relationship between sustaining notes and active notes to correct MI2 bug 766426 (V5 games: Adlib SFX not looped). Sustaining notes were left in the active notes list, which causes a problem if a track switch occurs. (Normally a track switch is followed immediately by a jump to tick. If sustaining notes are still in the active notes list, that's a problem because those notes were NOT part of this new track, so of course reasonably timed Note Off events for those notes are not going to be found.)
I do not have time to test this with the Edna scene. eriktorbjorn? If this works fine there as well, someone needs to commit it to the 0.5.0 branch. I had a problem doing so last time I tried to get something from the trunk to the branch.
Will leave this bug open until fix is verified.
comment:11 by , 21 years ago
I don't seem to be able to reproduce the Edna bug any more in the trunk, even though I made several attempts. (I never figured out exactly how to trigger it, but I only needed three attempt with the 0.5.0 branch.)
So it's probably fixed there as well.
comment:13 by , 21 years ago
| Status: | new → closed |
|---|
Savegame near the end of the game