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 LB2 Yvette/Tut missing sync resource #1373

Merged
merged 1 commit into from Dec 2, 2018

Conversation

Projects
None yet
3 participants
@sluicebox
Contributor

sluicebox commented Nov 2, 2018

Fixes a missing resource in the game. bug #9956

SCI: Fix LB2 Yvette/Tut missing sync resource
Fixes a missing resource in the game. bug #9956

@digitall digitall requested review from bluegr and m-kiewitz Nov 2, 2018

@@ -469,6 +469,12 @@ int ResourceManager::readAudioMapSCI11(IntMapResourceSource *map) {
// FIXME: The sync36 resource seems to be two bytes too big in KQ6CD
// (bytes taken from the RAVE resource right after it)
if (syncSize > 0) {
// LB2CD is missing the sync resource for message 1885 1 6 30 2 but it's a duplicate
// of 1885 1 6 16 2 which does have a sync resource so use that for both. bug #9956
if (g_sci->getGameId() == GID_LAURABOW2 && map->_mapNumber == 1885 && n == 0x01061002) {

This comment has been minimized.

@m-kiewitz

m-kiewitz Nov 2, 2018

Contributor

Please add "g_sci->isCD()" to make sure it's the CD version. Well in case it's really CD version only.

Overall I haven't looked into this. Is a script patch impossible? I would prefer a script patch in these cases, if possible.
I think at one point we should add some mechanic to replace resources/fix resource data in a way. That way we could also fix the other broken lip sync data.

This comment has been minimized.

@bluegr

bluegr Nov 28, 2018

Member

Only the CD version has sync resources, so adding a CD check here is unnecessary

@sluicebox

This comment has been minimized.

Contributor

sluicebox commented Nov 2, 2018

This is such a funny message, it's so simple but they started with one bug which they fixed while introducing two others, so this is disproportionately confusing. I'm also torn because the voice acting is so awful that I don't really want to put effort into encouraging people to use it, but at this point I'm too deeply invested in this silly message! =)

The script is playing the correct message, which was added in the CD version, and it's just that of the message's 3 sequences the 2nd one doesn't have a sync resource. There's also a 5-sequence message whose first three sequences are identical to this one, which does have a sync resource for seq 2, so my goal is to just re-use it.

I didn't think this could be script patched since it's doing the right thing but now, given the existing fixes I've got in place for these two messages, I can think of a couple ways to do it, and there's a lot of room for patching in that script so it wouldn't be hard. I'm going to try changing this to a script patch.

@m-kiewitz

This comment has been minimized.

Contributor

m-kiewitz commented Nov 2, 2018

I didn't think this could be script patched since it's doing the right thing but now, given the existing fixes I've got in place for these two messages, I can think of a couple ways to do it, and there's a lot of room for patching in that script so it wouldn't be hard. I'm going to try changing this to a script patch.

As I said, if it's not possible let's keep this for now.
But I'm planning for some functionality so that we can patch/modify/add resource data on-the-fly as well and thus fixing things like this. We can also create lip sync data by ourselves, will take some time, but this would then fix King's Quest 6 bugs similar to these ones.

@sluicebox

This comment has been minimized.

Contributor

sluicebox commented Nov 2, 2018

I've got a script patch working, can you eyeball to see if this is preferable to the c++ change? https://gist.github.com/sluicebox/d3d76ff76802624eea4364264b5d0cd8

If so I'll continue and comment it up and get this PR switched over to it.

The patch changes the script to use the message that does have sync data, but since that message has two more sequences than the correct one, it sets the messager's lastSequence property so that it only plays the first three.

@m-kiewitz

This comment has been minimized.

Contributor

m-kiewitz commented Nov 2, 2018

Hmmm, let me think a bit about this.
It's a difficult decision. When we got functionality for direct resource patching, I guess maybe that method would be the best solution for this case.
Are you definitely sure that the "ask about ernie" is definitely impossible?

@sluicebox

This comment has been minimized.

Contributor

sluicebox commented Nov 2, 2018

100% sure. Ernie is only added to the notebook during the opening sequence of act 3 which introduces him and this is an act 2 script, but even if you add him manually with the debugger and ask about him you'll get the right response without the patched-out code. All that handler does is test the flag to see if he's dead so that it can override the default response and show a different message, and since he's not dead since he hasn't been introduced... there's a couple layers of redundancy. There's a lot of inaccessible code in this script that was copied from later acts.

@bluegr

This comment has been minimized.

Member

bluegr commented Nov 28, 2018

Any updates on this one?

@sluicebox

This comment has been minimized.

Contributor

sluicebox commented Nov 28, 2018

It's @m-kiewitz 's call how to proceed. Between the current options of the c++ workaround in the loader vs the script patch, I think the workaround is preferable. They both function equally well, I don't have any doubts about the script patch, but the script patch seems a disproportionately clever solution for the problem.

readAudioMapSCI11() already has workarounds for known broken resources which I think this is similar to. If there's better functionality for patching these kinds of things in the future then the c++ workaround will be more straight forward to understand and easier to recognize. The clever script patch requires a bit of game knowledge to recognize what's going on.

I can qualify that c++ code with an isCD check if that helps clarify. (Though since this is a sync36 resource it won't be in any other version)

@bluegr

This comment has been minimized.

Member

bluegr commented Nov 28, 2018

We can just go with the workaround in the interpreter for now, and when we have a working script patch in the future, we can remove it.

@m-kiewitz, are you OK with this? We need to replace game-specific checks in other parts of the SCI code as well, so IMHO changing all of them into script patches is a more long-term goal.

@sluicebox

This comment has been minimized.

Contributor

sluicebox commented Nov 30, 2018

Right, now I do already have a working script patch for this, I linked to a gist with it above, so we do have both in hand to choose from. I'm saying that since the bug is a resource bug and the original script is correct and the script patch is a little too clever, the simpler interpreter workaround is preferable.

As I understand it the longer term goal is to have a mechanism other than script patches for handling known broken resources, in this case a missing resource which happens to have an identical copy that can be used for both. We just saw a GK1 message whose audio had the wrong message tuple, and there are a couple reported bugs for QFG4CD messages with the wrong audio, and I bet more will be found.

@bluegr

This comment has been minimized.

Member

bluegr commented Dec 1, 2018

I agree with the rationale. We should create a mechanism for such cases, like the code we have for workarounds and script patches. Until we have such a mechanism, this patch should suffice.

Please, add a TODO to create a mechanism for such cases in your commit, and we can merge it

@bluegr

This comment has been minimized.

Member

bluegr commented Dec 2, 2018

Update: I'll merge this and add a relevant TODO

@bluegr bluegr merged commit 6284676 into scummvm:master Dec 2, 2018

1 check passed

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