SCUMM: Fix bug #6679 - INDY3 DOS-EGA: book of maps graphic glitch

[rrebello]

This fixes the issue described in bug #6679 (purple box covering part of the screen while looking at the book of maps). A more detailed explanation of what goes on under the hood has been provided in the bugtracker:

http://sourceforge.net/p/scummvm/bugs/6679/

I've done some testing by playing through the game and this change doesn't seem to have introduced any side effects.

[digitall]

Any SCUMM engine developers want to review this? @sev- @Kirben @lordhoto

It looks to be a fairly small correction, but should this be limited to just INDY3 by gameid or is it more widely applicable in SCUMM?

[Kirben]

I can't see anything like that code change, in the original. So it would be best to mark as a work around via comments, and limit only the the specific versions of Indy3 which require the work around.

[rrebello]

I see. Just out of curiosity, what exactly do you mean by "in the original"? Do you guys actually have access to the original source code of SCUMM games? Or are you referring to the code obtained via reverse engineering?

Anyway, I had decided not to limit the change to any specific game id as it seemed, at first glance at least, to be reasonably safe. If you look at ScummEngine::setCameraAtEx or ScummEngine::panCameraTo, which are parts where "camera._mode" is changed to a value other than kFollowActorCameraMode, you'll see the state variable "camera._movingToActor" is always reset to false. That makes sense as it stops the following block, currently found in lines 144-147 in engines/scumm/camera.cpp, from setting the camera destination to the actor's position (the camera shouldn't follow the actor if it's not in Follow Actor mode):

if (camera._movingToActor) {
    a = derefActor(camera._follows, "moveCamera(2)");
    camera._dest.x = a->getPos().x;
}

Well, since the root cause of this bug is actually the wrong room width value stored in the data files of a specific version of Indy3 DOS/EGA, as I've explained in the bugtracker, I've decided to propose an alternate and maybe more appropriate solution, which consists in simply forcing the correct value for the room in question in ScummEngine_v3old::setupRoomSubBlocks().

I've intentionally made a new commit for the change instead of amending the first one since I'm not sure which solution you'll consider more appropriate. I'll certainly fix that as soon as a decision is made and create a single commit.

[bluegr]

We do not have access to the original source of the SCUMM engine, so we are referring to the original executable. Your original change, although small, is likely to introduce regressions, because we will deviate from the behavior of the original executables.

IMHO, your second commit is the right way to go: it fixes the specific bug with that room, without causing any potentially unwanted side-effects in other games.

[rrebello]

Alright then. I've squashed the second commit into the first one and fixed the commit message.

[rrebello]

OK. I've done as you suggested and amended my last commit.

bluegr:

You could just do:

_roomWidth = READ_LE_UINT16(&(rmhd->old.width));
if (_game.id == GID_INDY3 && _roomResource == 64 && _roomWidth == 1793)
    _roomWidth = 320;

This looks a bit cleaner, IMHO, and can handle the same issue in all the other Indy 3 versions, if it exists

[bluegr]

Great work, thanks! I can confirm this fixes the issue. Merging :)