[Scummvm-git-logs] scummvm master -> 74ec1e26b58709dd918c2c609ae5fb154a6a6672

[dwatteau]

This automated email contains information about 1 new commit which have been
pushed to the 'scummvm' repo located at https://api.github.com/repos/scummvm/scummvm .

Summary:
74ec1e26b5 SCUMM: Add some notes on the setOwnerOf() assert workarounds


Commit: 74ec1e26b58709dd918c2c609ae5fb154a6a6672
    https://github.com/scummvm/scummvm/commit/74ec1e26b58709dd918c2c609ae5fb154a6a6672
Author: Donovan Watteau (contrib at dwatteau.fr)
Date: 2025-05-22T02:40:47+02:00

Commit Message:
SCUMM: Add some notes on the setOwnerOf() assert workarounds

I've tested again the Passport to Adventure case (see Trac no. 3657).
Indeed, without this workaround, the end of the Indy3 demo would crash
(so, it deserves the `kEnhGameBreakingBugFixes` value property we use
to flag and find such differences).

I've tested the same demo in DOSBox/DREAMM, and it doesn't crash there,
though. And the similar Freddi2 case next to this workaround *might*
mean that the original interpreters do something else, when obj == 0?

So I wonder if this assert(), and the clearOwnerOf() one (both added in
commit 6419311a2e361c5e265ae3796ef425700694801f) are a good thing?

Changed paths:
    engines/scumm/object.cpp


diff --git a/engines/scumm/object.cpp b/engines/scumm/object.cpp
index de537c0c8af..e432850d54a 100644
--- a/engines/scumm/object.cpp
+++ b/engines/scumm/object.cpp
@@ -107,8 +107,12 @@ void ScummEngine::setOwnerOf(int obj, int owner) {
 
 	// WORKAROUND for bug #3657: Game crash when finishing Indy3 demo.
 	// Script 94 tries to empty the inventory but does so in a bogus way.
+	// (It clears the inventory while iterating it in ascending order.)
 	// This causes it to try to remove object 0 from the inventory.
-	if (_game.id == GID_PASS && obj == 0 && vm.slot[_currentScript].number == 94)
+	// This would then trigger the assert() below.
+	//
+	// Note that the original interpreter did NOT produce an error, here.
+	if (_game.id == GID_PASS && obj == 0 && vm.slot[_currentScript].number == 94 && enhancementEnabled(kEnhGameBreakingBugFixes))
 		return;
 
 	// WORKAROUND for bug #6802: assert() was triggered in freddi2.
@@ -120,6 +124,9 @@ void ScummEngine::setOwnerOf(int obj, int owner) {
 	if (_game.id == GID_HEGAME && obj == 0 && _currentRoom == 39 && vm.slot[_currentScript].number == 10)
 		return;
 
+	// TODO: Should the following assert(), and the ScummEngine::clearOwnerOf()
+	// one, really be used? It looks like some original interpreters let this
+	// through -- but what did they exactly do, in this case?
 	assert(obj > 0);
 
 	if (owner == 0) {
@@ -136,7 +143,7 @@ void ScummEngine::setOwnerOf(int obj, int owner) {
 		// The bad code:
 		//   if (ss->where == WIO_INVENTORY && _inventory[ss->number] == obj) {
 		// That check makes no sense at all: _inventory only contains 80 items,
-		// which are in the order the player picked up items. We can only
+		// which are in the order the player picked them up. We can only
 		// guess that the SCUMM coders meant to write
 		//   if (ss->where == WIO_INVENTORY && ss->number == obj) {
 		// which would ensure that an object script that nukes itself gets