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 LONGBOW Amiga pub error, bug #9688 #1437

Merged
merged 1 commit into from Dec 9, 2018

Conversation

Projects
None yet
4 participants
@sluicebox
Contributor

sluicebox commented Dec 5, 2018

Fixes a script bug which causes a kernel signature mismatch

@m-kiewitz

This comment has been minimized.

Contributor

m-kiewitz commented Dec 5, 2018

Fixes a script bug which causes a kernel signature mismatch

Did this not work at all in the original interpreter?
I mean the code itself. What exactly happened in the original interpreter?

@sluicebox

This comment has been minimized.

Contributor

sluicebox commented Dec 5, 2018

It's a weird one because there doesn't seem to be any point to this script that they added, even if it did work.

They overrode onMe in this one inconsequential Feature in the room and replicated a small bit of the work that Feature:onMe already does, but did it wrong, and passed an object for the x parameter and a non-existent parameter 2 for the y parameter. The interpreter either ignored this or treated them as integers (I don't know which the amiga interpreter is doing under the hood) but either way the result is that fDrunk:onMe didn't return the correct answer, but from my tests nothing actually depends on this return value.

onMe is supposed to accept either an x and y parameter or a single object with x and y properties. fDrunk:onMe assumes that only the first one will happen, but the second one happens on every click.

@sluicebox

This comment has been minimized.

Contributor

sluicebox commented Dec 5, 2018

This is fDrunk:onMe:

(method (onMe param1 param2)
	(return (& onMeCheck (OnControl 4 param1 param2)))
)
@m-kiewitz

This comment has been minimized.

Contributor

m-kiewitz commented Dec 5, 2018

Well, you may also try to simply add a workaround entry for this one, making it skip the call.
If that works out, and makes it work properly as well, then I guess that may be better than a full script patch.
If it was broken in the original interpreter because of this script bug, there would be a reason to of course patch it out properly.

@sluicebox

This comment has been minimized.

Contributor

sluicebox commented Dec 5, 2018

I think the advantage of the script patch in this case is that it restores onMe to returning the correct value as it did in DOS before they overrode it with a broken one. Feature:onMe handles both integers and object parameters and does the right thing.

I'm 99% sure that just skipping the kernel call would be fine too, but if I understand that option right then onMe would still be returning the wrong answer (which seems like it doesn't matter), but it seems like making it work is less likely to have side effects.

@sluicebox

This comment has been minimized.

Contributor

sluicebox commented Dec 5, 2018

Which workaround table/type should I try for this? I've only done uninit reads.

@Vhati

This comment has been minimized.

Contributor

Vhati commented Dec 5, 2018

@sluicebox:

I've only done uninit reads.

OT question: should I add workarounds for every "uninit read" warning I see, or are those workarounds only meant to deal with noticeable bugs?

@m-kiewitz

This comment has been minimized.

Contributor

m-kiewitz commented Dec 6, 2018

Which workaround table/type should I try for this? I've only done uninit reads.

kOnMe currently does not have one.
I would have to add it.
Let me look into this during the upcoming weekend.

OT question: should I add workarounds for every "uninit read" warning I see, or are those workarounds only meant to deal with noticeable bugs?

Please only add workaround entries when our interpreter error()s out. I think for some uninitialized reads it doesn't error() out. And there are ways to combine entries in various situations.

@Vhati

This comment has been minimized.

Contributor

Vhati commented Dec 6, 2018

@m-kiewitz:

only add workaround entries when our interpreter error()s out. I think for some uninitialized reads it doesn't error() out.

I see these in my status window, no noticeable ill effects. Do they qualify?
Or do you mean when the debugger drops down and ends the game?

WARNING: Uninitialized read for parameter 1 from method hero::changeGait (room 770, script 28, localCall ffffffff)!

WARNING: Uninitialized read for parameter 3 from method gloryMessager::nextMsg (room 790, script 64924, localCall ffffffff)!
@sluicebox

This comment has been minimized.

Contributor

sluicebox commented Dec 6, 2018

Okay, now I see what's going on, this bug does have an effect on the game. It breaks messages in Amiga and the script patch is necessary to fix those. I'll update the comments.

I was confused because I only saw one drunk man at the pub at the table, and his messages work, but there's also a second one next to him who is difficult to see because he's just some dark feet on the floor sticking out from behind a wall. I never noticed him before! They have similar coordinates and are named tDrunk and fDrunk. fDrunk is the one on the floor that's causing problems. Too many drunks.

The fDrunk:onMe they added breaks hit testing since they're calling kOnControl with an object for an x and an uninitialized param for y. fDrunk:onMe would then always return false and so you wouldn't get any messages when clicking on the feet. I suppose if you were really unlucky it might return true when clicking somewhere else and then you'd get the wrong message. The script patch avoids the illegal kernel call and fixes the messages by restoring working hit testing.

@sluicebox sluicebox force-pushed the sluicebox:longbowamigapubfix branch from 11ca78d to b5f9f57 Dec 6, 2018

@sluicebox

This comment has been minimized.

Contributor

sluicebox commented Dec 6, 2018

Easy to see how a tester would miss that they'd broken the messages on this guy...

scummvm00020

0x39, 0x03, // pushi 03
0x39, 0x04, // pushi 04
0x8f, 0x01, // lsp 01
0x8f, 0x02, // lsp 02

This comment has been minimized.

@bluegr

bluegr Dec 6, 2018

Member

This looks to be quite generic code. Could it also erroneously match with scripts of the PC version?

This comment has been minimized.

@sluicebox

sluicebox Dec 7, 2018

Contributor

I've tested against PC and the sequence is not there. Those instructions are indeed generic but they come after pTos onMeCheck. I can include the next instruction, callk OnControl 6, to seal the deal. =)

@m-kiewitz

This comment has been minimized.

Contributor

m-kiewitz commented Dec 6, 2018

I see these in my status window, no noticeable ill effects. Do they qualify?
Or do you mean when the debugger drops down and ends the game?

I mean the latter.
We added the warnings later. Those are uninitialized parameters.
This may still cause problems, but happens quite often and until now I think we had no situation where those caused any issues. I may be wrong on that.

The ones where the debugger activates and an actual error gets thrown, those are definitely problematic ones that caused tons of issues. At first we did not even detect those and used zero all the time. The original interpreter though reused memory. So in a few cases, the scripts really need some special number to be inside the uninitialized memory like 1 or 5 and when that's not in there the scripts go crazy.

That's why we added those workarounds later so that we can specify the number.

The warning for subroutine parameters is simply there to notice those too. They do not need fixing, unless they cause some script bug.

@Vhati

This comment has been minimized.

Contributor

Vhati commented Dec 6, 2018

@m-kiewitz:
Understood. Thanks for clarifying.

@m-kiewitz

This comment has been minimized.

Contributor

m-kiewitz commented Dec 6, 2018

@m-kiewitz:
Understood. Thanks for clarifying.

Oh I see that I didn't specify the error() ones.
That's uninitialized variables, especially local variables.
Local variable block is defined, but some are not set. Sierra didn't zero out the local variable block, but kept whatever was in that memory area. We do not do that. We set zeroes, well in this case a special value, so that we can detect uninitialized reads.

The other ones are as I said parameters for subroutines.

SCI: Fix LONGBOW Amiga pub error, bug #9688
Fixes a script bug which causes a kernel signature mismatch

@sluicebox sluicebox force-pushed the sluicebox:longbowamigapubfix branch from b5f9f57 to b4f3560 Dec 7, 2018

@bluegr

This comment has been minimized.

Member

bluegr commented Dec 9, 2018

Thanks for your work! Merging

@bluegr bluegr merged commit d95b8e2 into scummvm:master Dec 9, 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