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:
8a595e7
5484f0b
cc7cbfe

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) {

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;
 }
 

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
 			}
 		}
 	}

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;

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