[Scummvm-git-logs] scummvm master -> 9795144183d77637f264aa7a8aae278b1e02292b
bluegr
noreply at scummvm.org
Thu Mar 21 08:02:06 UTC 2024
This automated email contains information about 1 new commit which have been
pushed to the 'scummvm' repo located at https://github.com/scummvm/scummvm .
Summary:
9795144183 AGI: Implement motion/cycler overwrite behavior
Commit: 9795144183d77637f264aa7a8aae278b1e02292b
https://github.com/scummvm/scummvm/commit/9795144183d77637f264aa7a8aae278b1e02292b
Author: sluicebox (22204938+sluicebox at users.noreply.github.com)
Date: 2024-03-21T10:02:02+02:00
Commit Message:
AGI: Implement motion/cycler overwrite behavior
- Fixes Black Cauldron witches not disappearing at end of game
- Properly fixes Donald Duck's Playground intro, bug #14170
- Continues to fix KQ1 eagle jump, bug #7046
Big thanks to @AGKorson for providing detailed information on this
interpreter behavior, script analysis, and maintaining excellent
AGI documentation in WinAGI.
See also:
8a595e7771aa89d06876e13d7ab6751e26da8982
5484f0bc58b77bf7dd28debf1b7a53bd138ba28c
cc7cbfe6266ee5d875d9c96d7ecf50c9098f58ca
Changed paths:
engines/agi/motion.cpp
engines/agi/op_cmd.cpp
engines/agi/saveload.cpp
engines/agi/view.cpp
engines/agi/view.h
diff --git a/engines/agi/motion.cpp b/engines/agi/motion.cpp
index e250203f45b..1a2878dd345 100644
--- a/engines/agi/motion.cpp
+++ b/engines/agi/motion.cpp
@@ -60,36 +60,40 @@ void AgiEngine::changePos(ScreenObjEntry *screenObj) {
}
// WORKAROUND:
-// A motion was just activated, check if "end.of.loop"/"reverse.loop" is currently active for the same screen object
-// If this is the case, it would result in some random flag getting overwritten in original AGI after the loop was
-// completed, because in original AGI loop_flag + wander_count/follow_stepSize/move_X shared the same memory location.
-// This is basically an implementation error in the original interpreter.
-// Happens in at least:
-// - BC: right at the end when the witches disappear at least on Apple IIgs (room 12, screen object 13, view 84)
-// - KQ1: when grabbing the eagle (room 22).
+// Overwrite cycler state with motion data as original AGI did, almost.
+//
+// The original AGI interpreter stored motion and cycler data in the same four
+// bytes of the screen object struct. If a script activated a motion while a
+// cycler is active, or the opposite, then the previous action's state was
+// overwritten. Some game scripts rely on this behavior, although probably
+// unintentionally. We store motion and cycler data in separate fields, so
+// in order to produce the original behavior that games depend on, we implement
+// the overwrite behavior.
+//
+// However, we make an exception: when cycler data is overwritten, the original
+// would set an unintended game flag when it completed. It doesn't seem like
+// anything good can come from setting an unintended flag, so we do not set any
+// flag when an overwritten cycler completes.
+//
+// This affects at least:
+// - KQ1: when the eagle grabs ego (room 22), Bug #7046
+// - BC: when the witches disappear at the end of the game (room 12, screen object 13)
+// - DDP: introduction when the ducks jump, Bug #14170
// - KQ2: happened somewhere in the game, LordHoto couldn't remember exactly where
-// FIXME: This workaround prevents the DDP introduction from animating the three
-// jumping ducks while they move from left to right. Bug #14170
-// For now, this game is excluded from the workaround, but a proper fix
-// is needed, or at least an explanation for why blocking this behavior
-// is the right thing to do when at least one game relies on it.
void AgiEngine::motionActivated(ScreenObjEntry *screenObj) {
- // Exclude DDP from workaround; see above
- if (getGameID() == GID_DDP) {
- return;
- }
-
- if (screenObj->flags & fCycling) {
- // Cycling active too
+ if (screenObj->flags & fCycling) { // Is a cycler active?
switch (screenObj->cycle) {
case kCycleEndOfLoop: // "end.of.loop"
- case kCycleRevLoop: // "reverse.loop"
- // Disable it
- screenObj->flags &= ~fCycling;
- screenObj->cycle = kCycleNormal;
-
- warning("Motion activated for screen object %d, but cycler also active", screenObj->objectNr);
- warning("This would have resulted in flag corruption in original AGI. Cycler disabled.");
+ case kCycleRevLoop: // "reverse.loop"
+ // This would have overwritten the cycler's flag with wander_count
+ // or follow_stepSize or move_x, and that would cause the cycler
+ // to set an unintended flag if it completes.
+ // We skip setting the unintended flag with ignoreLoopFlag.
+ // Jumping at the eagle in KQ1 room 22 depends on the overwritten
+ // flag not being set. Bug #7046
+ screenObj->ignoreLoopFlag = true;
+ warning("Motion activated for screen object %d while cycler is active", screenObj->objectNr);
+ warning("Original AGI would overwrite flag %d, we skip setting it", screenObj->loop_flag);
break;
default:
break;
@@ -98,29 +102,39 @@ void AgiEngine::motionActivated(ScreenObjEntry *screenObj) {
}
// WORKAROUND:
+// Overwrite motion state with cycler data as original AGI did.
+// This is necessary because we use a different data structure than the
+// original, but games relied on undefined behavior when activating a
+// cycler while a motion was in progress.
// See comment for motionActivated()
-// This way no flag would have been overwritten, but certain other variables of the motions.
void AgiEngine::cyclerActivated(ScreenObjEntry *screenObj) {
+ const char *fieldName;
+ uint8 previousValue;
switch (screenObj->motionType) {
case kMotionWander:
- // this would have resulted in wander_count to get corrupted
- // We don't stop it.
+ // Overwrite wander count with the cycler's flag.
+ fieldName = "wander_count";
+ previousValue = screenObj->wander_count;
+ screenObj->wander_count = screenObj->loop_flag;
break;
case kMotionFollowEgo:
- // this would have resulted in follow_stepSize to get corrupted
- // do not stop motion atm - screenObj->direction = 0;
- // do not stop motion atm - screenObj->motionType = kMotionNormal;
+ // Overwrite follow step size with the cycler's flag.
+ fieldName = "follow_stepSize";
+ previousValue = screenObj->follow_stepSize;
+ screenObj->follow_stepSize = screenObj->loop_flag;
break;
case kMotionMoveObj:
- // this would have resulted in move_x to get corrupted
- // do not stop motion atm - motionMoveObjStop(screenObj);
+ // Overwrite move_x with the cycler's flag.
+ // Required for witches to disappear at the end of Black Cauldron, room 12.
+ fieldName = "move_x";
+ previousValue = screenObj->move_x;
+ screenObj->move_x = screenObj->loop_flag;
break;
default:
return;
- break;
}
- warning("Cycler activated for screen object %d, but motion also active", screenObj->objectNr);
- warning("This would have resulted in corruption in original AGI. Motion disabled.");
+ warning("Cycler activated for screen object %d while motion is active", screenObj->objectNr);
+ warning("Overwriting %s: %d with flag number %d, as original AGI did", fieldName, previousValue, screenObj->loop_flag);
}
void AgiEngine::motionWander(ScreenObjEntry *screenObj) {
diff --git a/engines/agi/op_cmd.cpp b/engines/agi/op_cmd.cpp
index 9bce60be4dc..cf050e79fe3 100644
--- a/engines/agi/op_cmd.cpp
+++ b/engines/agi/op_cmd.cpp
@@ -1535,7 +1535,7 @@ void cmdReverseLoop(AgiGame *state, AgiEngine *vm, uint8 *parameter) {
debugC(4, kDebugLevelScripts, "o%d, f%d", objectNr, loopFlag);
screenObj->cycle = kCycleRevLoop;
screenObj->flags |= (fDontUpdate | fUpdate | fCycling);
- screenObj->loop_flag = loopFlag;
+ screenObj->setLoopFlag(loopFlag);
vm->setFlag(screenObj->loop_flag, false);
vm->cyclerActivated(screenObj);
@@ -1550,7 +1550,7 @@ void cmdReverseLoopV1(AgiGame *state, AgiEngine *vm, uint8 *parameter) {
screenObj->cycle = kCycleRevLoop;
vm->setCel(screenObj, 0);
screenObj->flags |= (fDontUpdate | fUpdate | fCycling);
- screenObj->loop_flag = loopFlag;
+ screenObj->setLoopFlag(loopFlag);
//screenObj->parm3 = 0;
}
@@ -1562,7 +1562,7 @@ void cmdEndOfLoop(AgiGame *state, AgiEngine *vm, uint8 *parameter) {
debugC(4, kDebugLevelScripts, "o%d, f%d", objectNr, loopFlag);
screenObj->cycle = kCycleEndOfLoop;
screenObj->flags |= (fDontUpdate | fUpdate | fCycling);
- screenObj->loop_flag = loopFlag;
+ screenObj->setLoopFlag(loopFlag);
vm->setFlag(screenObj->loop_flag, false);
vm->cyclerActivated(screenObj);
@@ -1577,7 +1577,7 @@ void cmdEndOfLoopV1(AgiGame *state, AgiEngine *vm, uint8 *parameter) {
screenObj->cycle = kCycleEndOfLoop;
vm->setCel(screenObj, 0);
screenObj->flags |= (fDontUpdate | fUpdate | fCycling);
- screenObj->loop_flag = loopFlag;
+ screenObj->setLoopFlag(loopFlag);
//screenObj->parm3 = 0;
}
diff --git a/engines/agi/saveload.cpp b/engines/agi/saveload.cpp
index b1e566b0af9..1e612c834ea 100644
--- a/engines/agi/saveload.cpp
+++ b/engines/agi/saveload.cpp
@@ -645,7 +645,7 @@ int AgiEngine::loadGame(const Common::String &fileName, bool checkId) {
screenObj->cycle = (CycleType)in->readByte();
if (saveVersion >= 11) {
// Version 11+: loop_flag, was previously vt.parm1
- screenObj->loop_flag = in->readByte();
+ screenObj->setLoopFlag(in->readByte());
}
screenObj->priority = in->readByte();
@@ -688,10 +688,10 @@ int AgiEngine::loadGame(const Common::String &fileName, bool checkId) {
if (saveVersion < 7) {
// Recreate loop_flag from motion-type (was previously vt.parm1)
// vt.parm1 was shared for multiple uses
- screenObj->loop_flag = oldLoopFlag;
+ screenObj->setLoopFlag(oldLoopFlag);
} else {
// for Version 7-10 we can't really do anything, it was not saved
- screenObj->loop_flag = 0; // set it to 0
+ screenObj->setLoopFlag(0); // set it to 0
}
}
}
diff --git a/engines/agi/view.cpp b/engines/agi/view.cpp
index 8d5bac73d2e..c8e3fbdda77 100644
--- a/engines/agi/view.cpp
+++ b/engines/agi/view.cpp
@@ -46,7 +46,11 @@ void AgiEngine::updateView(ScreenObjEntry *screenObj) {
if (++celNr != lastCelNr)
break;
}
- setFlag(screenObj->loop_flag, true);
+ if (!screenObj->ignoreLoopFlag) {
+ setFlag(screenObj->loop_flag, true);
+ } else {
+ warning("kCycleEndOfLoop: skip setting flag %d", screenObj->loop_flag);
+ }
screenObj->flags &= ~fCycling;
screenObj->direction = 0;
screenObj->cycle = kCycleNormal;
@@ -57,7 +61,11 @@ void AgiEngine::updateView(ScreenObjEntry *screenObj) {
if (celNr)
break;
}
- setFlag(screenObj->loop_flag, true);
+ if (!screenObj->ignoreLoopFlag) {
+ setFlag(screenObj->loop_flag, true);
+ } else {
+ warning("kCycleRevLoop: skip setting flag %d", screenObj->loop_flag);
+ }
screenObj->flags &= ~fCycling;
screenObj->direction = 0;
screenObj->cycle = kCycleNormal;
diff --git a/engines/agi/view.h b/engines/agi/view.h
index 26af4c9d85d..004e7209c0b 100644
--- a/engines/agi/view.h
+++ b/engines/agi/view.h
@@ -141,9 +141,15 @@ struct ScreenObjEntry {
uint8 wander_count;
// end of motion related variables
uint8 loop_flag;
+ bool ignoreLoopFlag;
void reset() { memset(this, 0, sizeof(ScreenObjEntry)); }
ScreenObjEntry() { reset(); }
+
+ void setLoopFlag(uint8 flag) {
+ loop_flag = flag;
+ ignoreLoopFlag = false;
+ }
}; // struct vt_entry
} // End of namespace Agi
More information about the Scummvm-git-logs
mailing list