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

SCUMM: [RFC, don't merge yet] Change heuristic for identifying Indy 3 Mac sound (bug #14663) #5365

Merged
merged 1 commit into from Nov 5, 2023

Conversation

eriktorbjorn
Copy link
Member

This is an attempt at fixing https://bugs.scummvm.org/ticket/14663

Note that it refers to the original 16 color Macintosh version. The one sold these days on Steam is a much later port of the VGA version, as far as I know.

The problem is that we're trying to identify what is a digitized sound (and not a piece of music) in the Macintosh version of Indiana Jones and the Last Crusade by looking at values in a barely understood header, and this value turns out not to be identical in all sounds. One exception (and perhaps the only one?) is the sound used when pouring liquids, e.g. when loosening the torch in the Venice sewers, or when filling the beer stein in the castle Brunwald kitchen.

So here I'm instead trying to look for a value that appears to be constant for all music in the game, and if it's anything different it's a digitized sound. The scary thing is that this value is within one of the unknown parts of the audio header, so I don't know if it's just luck that it seems to work.

Note that I have not played through the whole game with this change. I'm still a bit weary of it from playing through it to test the Macintosh GUI.

I do not own the EGA version, so I can't compare. But I believe it uses synthesized sound effects, not digitized ones?

Instead of trying to identify it by looking for a value expected to be
the same for all sound effects (which turns out to not be as reliable as
hoped), look for a value that appears to be the same in all music.

This fixes a missing sound effect when pouring liquids, e.g. when
filling the beer stein in the castle Brunwald kitchen, or pouring liquid
on the Venice catacomb torch to loosen it.
@bluegr
Copy link
Member

bluegr commented Oct 21, 2023

Could we use the debugger to test all the Mac sounds with this change, instead of playtesting the whole game?

@eriktorbjorn
Copy link
Member Author

Could we use the debugger to test all the Mac sounds with this change, instead of playtesting the whole game?

That's pretty much what I did. The result is in a comment to that bug report I mentioned: https://bugs.scummvm.org/ticket/14663

The pouring sound was the only one I found that definitely didn't work. There were a few sound effects I couldn't identify. One of them would crash ScummVM whether it was played as sound or music, so I hope that one isn't used anywhere! For some, the resource manager returned a NULL pointer, so I guess there are gaps in the resources.

My impression was that my proposed patch wouldn't break anything - at least nothing that wasn't already broken - but it feels like replacing one poorly understood hack with another poorly understood hack.

@bluegr
Copy link
Member

bluegr commented Oct 23, 2023

So, we got two sounds which are broken:

  • 7: Pouring liquid
  • 34: Crashes ScummVM

Are these broken in the same manner without your change?

@eriktorbjorn
Copy link
Member Author

I don't have the time to double-check right now, but I believe 34 (which I never encountered while playing the game) is broken regardless of if we handle it as sound or music, i.e. my change shouldn't make any practical difference.

The pouring sound (7) is played as music (I couldn't hear it at all) without my change. With my change it's played as a digitized sound, like it should be.

@bluegr
Copy link
Member

bluegr commented Oct 29, 2023

So, overall, these changes have been tested, don't break anything in the Mac version of Indy 3, and fix the broken pouring sound.

The resource type detection is a hack indeed, but it's a tested hack, which makes sense, too

I don't see any issue merging it. Please speak if you have a different opinion on this

@sev-
Copy link
Member

sev- commented Nov 5, 2023

Thank you, merging.

@sev- sev- merged commit 1f1a013 into scummvm:master Nov 5, 2023
6 of 8 checks passed
@eriktorbjorn eriktorbjorn deleted the scumm-indy3-mac-missing-sound branch November 12, 2023 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants