SCUMM: Add support for Macintosh music in Monkey Island 1 and Loom #291
Conversation
This is based on the old Mac0-to-General MIDI conversion that we used to do (and which this patch removes), as well as the code for playing the Monkey Island 2 and Fate of Atlantis Macintosh music. I'm not sure how accurate it is, particularly in tempo and volume, but at this point it seems to work pretty well. Looping music is perhaps a bit off, but it was before as well. There is an annoying drawn out note in the music when you're following the shopkeeper, but that appears to have been there in the original as well.
|
How does this assure backwards compat with old savegames? It seems now iMuse won't be used for output anymore, but there is no steps taken to skip the iMuse data in old savegames as far as I can tell. I don't have the Macintosh version of Monkey Island 1, so I can't test this, but does returning 0 in getMusicTimer really work? For the (EGA) DOS version you would end up with a black screen for all of the intro then and it wouldn't continue except if you hit escape. Of course it might be that the scripts of the Macintosh version are different and I would assume you tested the intro, but it still looks a bit odd. |
|
I never though about savegames, so it doesn't. Until very recently, I didn't own Monkey Island for the Macintosh, so I didn't have any old savegames lying around. I'm not sure how to handle that properly. I tried the intro, and as far as I could understand it never checks the sound timer there. Instead, it only checks whether or not the music is still playing. The intro was the only part I could think of where music timing could be an issue, so for the rest of the music I've just listened to at least part of it (sometimes using boot params). |
|
While testing, I also noticed that it doesn't restart the music if I save in the SCUMM Bar and then reload that save. Haven't had the time to check how that's usually handled outside of iMUSE. |
|
It looks like we don't restore the currently playing music with any of the player_v* music players. I guess if someone saves at a point where it's waiting for the music to stop, restoring the game will cause it to trigger immediately. Not ideal, but probably not harmful either. I'm afraid I can't think of any good way of maintaining savegame compatibility. It's probably a bit like how in some games your savegames will break if you change music driver, except here the change in music driver is enforced. The least bad way I can think of would be to keep the conversion to General MIDI and play the music through the mac_68k.cpp player. (Personally, I'd rather break savegame compatibility.) |
…els. Otherwise it may crash if you quit before any instruments have been loaded. Oops.
It turns out that playing the Mac Loom music isn't particularly different from playing the Monkey Island 1 music, except the data layout is a bit different and there's no per-note volume.
The Monkey Island and Loom mac music is really quite similar. The data layout is a bit different, but most of the code was easy to separate into its own class. The Loom player doesn't do looped music but I don't remember off-hand if it ever should.
|
I've added a player for the Loom music as well. It seems to work, but it's much too late at night for me to play through the whole game. At least with Loom, it shouldn't break any old savegames. :) |
As clone2727 pointed out, the default case handles Loom. I guess it was a special case before to *prevent* it from trying to play the sound, and to keep some comments about the format.
|
I have an idea for how to maybe salvage older Monkey Island savegames. I'll hope I'll be able to experiment with it later tonight. (So this is probably not a good time to merge this.) |
Excplicitly cast to int to avoid a warning that I don't get, but which clone2727 does. At least, I hope it avoids the warning.
Try the Mac OS Roman form, the UTF-8 form and the filename without any trademark glyph.
Apparently we cannot (portably) call virtual functions from the constructor, so initialization has been moved to a separate function.
We no longer use iMuse for MI1 Mac so this never happens. The Mac player can only play one song at a time, so it should be all right.
|
My idea for salvaging the older Monkey Island savegames was to use a "dummy" iMuse object to just read and discard the saved data. But I wasn't able to get it to work, and I don't feel very motivated to keep trying tonight. (Anyone else feel like trying? he said, hopefully.) Other than that, and not saving its own music data, I'm fairly happy with this version. |
Note that while this removes _townsPlayer->saveLoadWithSerializer(s) it really shouldn't break anything because _musicEngine also points to the FM Towns player. Famous last words...
|
I've added saving/loading of the music player state. As far as I can tell, this does not break backwards compatibility with Loom savegames, though old MI1 savegames still remain broken. There is a chance I accidentally broke savegames for games that use the FM Towns player. I don't own any such games, so I can't test. But it should still be fine. |
For old savegames, we now use a "dummy" iMUSE objet to skip the old iMUSE save state. I had hoped to be able to do this without making any changes to the iMUSE code itself, but I was unable to. Also added note about how the save state for the new music will not quite work if the mixer output rate changes. Personally, I'm not too worried about that. It breaks, but it shouldn't break badly.
|
I've added a hack to preserve savegame compatibility with older Mac MI1 savegames, or at least it should work. I don't expect to make any more features commits to this branch. Of course, if someone finds any further bugs those will have to be dealt with. I'll see if I can muster the time and energy to play through the games for real this weekend. Loom at least doesn't usually take that long to get through. |
This keeps the music from breaking when loading a savegame that was made with a different sample rate than the current one. It also breaks all savegames made in the past eight hours, but I don't think it's necessary to maintain savegame compatibility within a pull request, as long as it still works with savegames made before it.
It was the remains of an experiment and no longer serves a purpose.
At least on my computer, when the note ended abruptly there would be an annoying "pop" at the end. This was particularly noticeable at the end of the distaff notes in Loom. To get around this, fade out the last 100 samples. There's nothing magical about 100 in particular, but it's a nice even number and it should be short enough that it's never a noticeable part of the note, even at low sample rates.
|
I've played through Loom with this patch now, and I didn't notice any obvious music-related glitches. The only glitches I noticed (apart from a bunch of "Code for object X not in room Y" warnings, which sound scary but which didn't seem to cause any harm, and double-click not working which I've already filed a bug report about) were text-related, where text got cut off at the right edge of the screen. I assume that's because we don't support the Mac version's high-resolution font. |
In looped music, prevent the music channels from drifting out of sync over time. This was noticeable after a few minutes in the SCUMM Bar. We do this by extending the last note (which is just zeroes, so we didn't even use to play it) so that it has the exact number of samples needed to make all channels the exact same length. (This is calculated when the music is loaded, so it does not need any extra data in the save games, thankfully.) As a result, the getNextNote() is now responsible for converting the duration to number of samples (out of necessity) and for converting the note to a pitch modifier (out of symmetry). I made several false starts before I realized how much easier it would be this way.
It shouldn't make any real difference, but it's probably more formally correct.
Properly treat rests as rests, not notes. Otherwise, it would try to play a really low note which just came out as a "pop".
|
I've played through Monkey Island 1, and fixed any bugs/regressions I've noticed along the way. The only remaining problem I saw was that the jungle music on Monkey Island restarted a couple of times. Using boot param 6565 and walking to the clearing (near the monkey head) reproduces that problem for me, but only the first time I do it. Apparently, the sound resource gets expired and so when the script asks if the sound is already playing, the engine says "No". Not sure if it's a bug or not. So I know I've said or thought this numerous times the last week, but this time I really think I've done all I set out to do with this player. I still don't know if the tempo is right, but that's easily adjustable. |
After some discussion on #scummvm, the player now locks the sound resource while the music is playing. This prevents the resource manager from expiring the resource, which at best could cause music to restart where it shouldn't.. At worst, I guess it could have crashed, but I never saw that happen.
|
After some discussion on #scummvm, I've added resource locking/unlocking while the music is playing. This fixes the restarting music in the Monkey Island jungle, and could possibly avert crashes in the future. I'm not even going to promise it's done this time. (But I hope it is. :-) |
| // \xAA is a trademark glyph in Mac OS Roman. We try that, but also the | ||
| // UTF-8 version, and just plain without in case the file system can't | ||
| // handle exotic characters like that. | ||
| if (!resource.exists("Loom\xAA") && !resource.exists("Loom\xE2\x84\xA2") && !resource.exists("Loom")) { |
There was a problem hiding this comment.
An additional check (\0x99) is required to detect the trademark glyph for Loom under Windows encoding too.
The code looks fine, other than that small required change for Windows.
Initialise _channel[] even when the instruments aren't available. Otherwise, ScummVM will crash in a number of places including, but not limited to, when loading savegames.
At least for me, hfsutils turns spaces into underscores so try both "Monkey Island" and "Monkey_Island".
| Common::StackLock lock(_mutex); | ||
| if (ser->getVersion() < VER(94)) { | ||
| if (_vm->_game.id == GID_MONKEY && ser->isLoading()) { | ||
| IMuse *dummyImuse = IMuse::create(_vm->_system, NULL, NULL); |
There was a problem hiding this comment.
Doesn't not deleting this leak memory?
I completely forgot to delete the dummy iMUSE object after using it to skip over the old music save information. Thanks to Lordhoto for pointing this out.
Some notes in the main theme are very staccato, and this could possibly explain why.
After listening to the original music in a Mac emulator (which unfortunately doesn't handle the music very well), I can only conclude that note value 1 means the note should continue playing. At first I thought maybe it was supposed to fade the current note, or perhaps change its volume, but I can't hear any traces of either. So I'm going to assume it just means "hold the current note", though for the life of me I cannot think of any valid reason for such a command. So it may be wrong, but it sounds closer to the emulator than it did before.
|
I've found that the MI1 music sounds much closer to the Mac emulator I'm using if I treat note value 1 as "hold current note" rather than "end current note". Though I can't understand why it would need that feature - just make the current note play longer! - so it probably means something subtly different. But, as I said, this does seem like a very noticeable improvement to me. |
|
Any reason which prevents this from being merged? |
|
Looks fine to merge to me |
|
Just talked with eriktorbjorn This pull request is complete code-wise. Older saved games should still work with this. Therefore, this is feature complete, and in a good state to be merged. if nobody has any objections by Sunday, I will merge it. |
|
Also checked with eriktorbjorn and he said he doesn't have any objections to merge this. There's still a few unknown/guessed bits in there, but delaying this till someone checks how the original handles it would delay this needlessly. Since the savegame issue has been taken care of, this looks fine to merge. Thanks for your work eriktorbjorn. Also thanks to Kirben and clone2727 for pointing out the filename encoding bits. |
SCUMM: Add support for Macintosh music in Monkey Island 1 and Loom
This is based on the old Mac0-to-General MIDI conversion that we used
to do (and which this patch removes), as well as the code for playing
the Monkey Island 2 and Fate of Atlantis Macintosh music. I'm not sure
how accurate it is, particularly in tempo and volume, but at this
point it seems to work pretty well. Looping music is perhaps a bit
off, but it was before as well.
There is an annoying drawn out note in the music when you're following
the shopkeeper, but that appears to have been there in the original as
well.
Since it turns out that the Loom music is pretty similar to the Monkey
Island music, I've added a player for that as well. It doesn't do looped
music, but I don't remember if it ever has to. The player is based on the
information in feature request #824221, AUDIO: LOOM Mac music
support and a tiny bit of guesswork.