Opened 13 years ago

Closed 13 years ago

#2826 closed defect (fixed)

MI2: Inventory object cloning

Reported by: SF/dvitas Owned by: eriktorbjorn
Priority: normal Component: Engine: SCUMM
Keywords: Cc:
Game: Monkey Island 2

Description

1) ScummVM version (scummvm -v): ScummVM 0.9.0 (Jun
22 2006 09:26:48)
Features compiled in: Vorbis FLAC MP3 zLib MPEG2

2) Bug details, including instructions on
reproducing: When on Dinkey Island try take the
action "Pull" on object "rope" in inventory. This
rope will clone;)

3) Language of game : English

4) Version of game : I'm not sure... ScummVM GUI
shows it as (English (US)/DOS)

5) Platform and Compiler: platform = WinXP. Compiler
= I'm not sure... I have downloaded WinXP binaries
from SourceForge.

6) save game attached. Just "pull" the "rope" in
inventory.

Ticket imported from: #1555938. Ticket imported from: bugs/2826.

Attachments (2)

monkey2.s20 (52.5 KB ) - added by SF/dvitas 13 years ago.
save with "cloning rope" bug
mi2-rope.diff (996 bytes ) - added by eriktorbjorn 13 years ago.
Experimental patch

Download all attachments as: .zip

Change History (13)

by SF/dvitas, 13 years ago

Attachment: monkey2.s20 added

save with "cloning rope" bug

comment:1 by eriktorbjorn, 13 years ago

That's a strange bug. And as far as I can tell, it does not
happen with the original interpreter.

I think it's at least partly related to that you can pull
the rope off the chest to pick it up. Sometimes when I tried
it in a different room, ScummVM would also draw the chest -
sans rope - as the rope was cloned. Other times, ScummVM
would just crash with a "findObjectInRoom: Object 1047 not
found in room ..." error.

So it seems like the rope's script to remove itself from the
chest is run again (and again, and again), but I don't
understand the handling of verbs well enough to figure out
what's really happening here.

By the way, I can reproduce the error using my own old
savegames, or boot params, so it's probably not just some
once-in-a-blue-moon glitch.

comment:2 by eriktorbjorn, 13 years ago

I don't see any special case to stop the player from pulling
the rope a second time, though I could very well be missing
something.

If so, then perhaps there should be safeguards in
o5_drawObject() and o5_pickupObject(), but does that really
match the original behaviour? I thought MI2 was among the
first - perhaps *the* first - SCUMM game to be supported by
ScummVM, so it's hard to imagine there being a bug like that
still there.

Interestingly, there's an o5_pickupObjectOld() which does
guard against picking the same object up twice... but I
couldn't find any trace of it in ScummVM 0.0.1, so I guess
it's for different games. Maybe.

comment:3 by Kirben, 13 years ago

Summary: Inventory object cloning in MI2MI2: Inventory object cloning

comment:4 by fingolfin, 13 years ago

o5_pickupObjectOld is for old games, aye.

I wonder if this is an old bug or a regression. Would be interesting to try older
ScummVM versions to see if they also suffer from this bug. Is there a good boot
param one can use to reproduce this?

comment:5 by eriktorbjorn, 13 years ago

Boot param 997 is pretty close. I've only tried SVN, 0.9.0
and 0.8.0, and it happens with all of those, at least.

At one time, I used to have all stable releases of ScummVM
on my computer, all the way back to 0.0.1. But then the
soname of one of the libraries I linked against changed, and
I upgraded to GCC 4.x which makes it difficult to compile at
least some of the early versions. (The change to
o5_jumpRelative() is easy to backport, of course, but I seem
to recall there were other difficulties as well.)

comment:6 by eriktorbjorn, 13 years ago

Oh, and to try it in DOSbox:

* Type "monkeyspit"
* Press Ctrl-D
* Press Ctrl-L
* Enter "997"

Actually, the bug seems to be *partly* there in the original
as well. In most rooms, nothing happened when I pulled the
rope. But in one case, the game did draw the image of the
rope-less chest, even though the chest wasn't really in the
room. That could indicate that ScummVM is running the
correct script. Good. That simplifies things.

But unlike ScummVM, the original never crashed complaining
that it couldn't find the object in the room. Nor did it
"clone" the rope.

Kirben assured me the other day that our o5_pickupObject()
matches the original disassembly. But of course, that opcode
is implemented in terms of a number of other functions, and
I don't think he extended the comparision to cover those as
well.

comment:7 by eriktorbjorn, 13 years ago

I guess we can tentatively assume that the right scripts are
executed. I think it's roomobj-97-1047, which is pretty
short. In fact, I'm guessing that the part that handles
pulling the rope is simply:

[001E] (05) drawObject(1045, setImage(2));
[0024] (5D) setClass(1045,[6])
[002B] (25) pickupObject(1047,0)
[002F] (00) stopObjectCode()

I'm furthermore guessing that 1045 is the chest object, and
that the first two lines are simply to redraw it and tell it
that the rope is now gone from it.

Kirben assures me that the pickupObject() opcode is
identical to the original, but does that also extend to the
functions that o5_pickupObject() calls? Specifically, what
about addObjectToInventory()? If I change it to a no-op when
whereIsObject() returns WIO_INVENTORY, both the cloning and
the "object not in room" crashes seem to go away.

The chest glitch remains, but that's as in the original.

Though I don't see any corresponding check in ScummVM 0.0.1...

by eriktorbjorn, 13 years ago

Attachment: mi2-rope.diff added

Experimental patch

comment:8 by fingolfin, 13 years ago

This is definitely a script bug. In fact, I can't reproduce it, since all version of
MI2 I have fix it. In my german PC version, the code in roomobj-97-1047 for
event 6 (pull) looks as follows:

[001E] (90) VAR_RESULT = getObjectOwner(VAR_ME)
[0023] (48) if (VAR_RESULT == 15) {
[002A] (05) drawObject(1045, setImage(2));
[0030] (5D) setClass(1045,[6])
[0037] (25) pickupObject(1047,0)
[003B] (**) }
[003B] (00) stopObjectCode()

And in the english mac version I own, they simply don't allow you to "pull" the
rope at all...

So, we should implement a workaround. One way would be to simply disallow
"pull" on this object. Another would be to provide a patched script. Or we
could add checks to both drawObject and pickupObject for this particular
case...

comment:9 by fingolfin, 13 years ago

I'd try to fix this myself, but since I own no version of MI2 exhibiting the
problem, I have a hard time doing that.

Some more thoughts: To disallow "pull" on the rope, one could add a mod to
getVerbEntrypoint() to make it fail to find the entrypoint 6 for object 1047, if the
gameid is MI2... But only do that if the object is already in the inventory
(mimicking the german PC version fix).

comment:10 by eriktorbjorn, 13 years ago

Owner: set to eriktorbjorn
Resolution: fixed
Status: newclosed

comment:11 by eriktorbjorn, 13 years ago

The verb entry workaround works for me. Committed, after
discussing with Fingolfin.

Note: See TracTickets for help on using tickets.