[Scummvm-cvs-logs] scummvm master -> 00b37f8552962c26412eda347024db80de3d99ac

bluegr md5 at scummvm.org
Thu Mar 3 18:39:57 CET 2011


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

Summary:
1aed9a1f34 SCI: Applied save/load dialog patching to all SCI0-SCI2.1 early games
51437ba5e6 SCI: Fixed path finding in Amiga SCI1 games
00b37f8552 SCI: Simplified arithmetic reg_t operations, based on fingolfin's comments


Commit: 1aed9a1f3401477d4d56e278fec63cd90fafd81d
    https://github.com/scummvm/scummvm/commit/1aed9a1f3401477d4d56e278fec63cd90fafd81d
Author: md5 (md5 at scummvm.org)
Date: 2011-03-03T09:31:47-08:00

Commit Message:
SCI: Applied save/load dialog patching to all SCI0-SCI2.1 early games

Games that have the newer SCI2.1 kernel functions (i.e. kSave instead of
kSaveGame/kRestoreGame) aren't supported yet

Changed paths:
    engines/sci/sci.cpp



diff --git a/engines/sci/sci.cpp b/engines/sci/sci.cpp
index 4155fd2..657a7b9 100644
--- a/engines/sci/sci.cpp
+++ b/engines/sci/sci.cpp
@@ -444,9 +444,8 @@ static void patchGameSaveRestoreCode(SegManager *segMan, reg_t methodAddress, by
 	byte *patchPtr = const_cast<byte *>(script->getBuf(methodAddress.offset));
 	if (getSciVersion() <= SCI_VERSION_1_1)
 		memcpy(patchPtr, patchGameRestoreSave, sizeof(patchGameRestoreSave));
-	else if (getSciVersion() == SCI_VERSION_2)
+	else	// SCI2+
 		memcpy(patchPtr, patchGameRestoreSaveSci2, sizeof(patchGameRestoreSaveSci2));
-	// TODO: SCI21/SCI3
 	patchPtr[8] = id;
 }
 
@@ -459,10 +458,6 @@ void SciEngine::patchGameSaveRestore() {
 	byte kernelIdRestore = 0;
 	byte kernelIdSave = 0;
 
-	// This feature is currently not supported in SCI21 or SCI3
-	if (getSciVersion() >= SCI_VERSION_2_1)
-		return;
-
 	switch (_gameId) {
 	case GID_MOTHERGOOSE256: // mother goose saves/restores directly and has no save/restore dialogs
 	case GID_JONES: // gets confused, when we patch us in, the game is only able to save to 1 slot, so hooking is not required
@@ -485,6 +480,11 @@ void SciEngine::patchGameSaveRestore() {
 			kernelIdSave = kernelNr;
 	}
 
+	// TODO: This feature does not yet work with the SCI2.1 middle and newer
+	// kernel functions (i.e. kSave)
+	if (!kernelIdRestore || !kernelIdSave)
+		return;
+
 	// Search for gameobject superclass ::restore
 	uint16 gameSuperObjectMethodCount = gameSuperObject->getMethodCount();
 	for (uint16 methodNr = 0; methodNr < gameSuperObjectMethodCount; methodNr++) {


Commit: 51437ba5e6e418d7e79cf341606bc6c1c50fd50b
    https://github.com/scummvm/scummvm/commit/51437ba5e6e418d7e79cf341606bc6c1c50fd50b
Author: md5 (md5 at scummvm.org)
Date: 2011-03-03T09:34:11-08:00

Commit Message:
SCI: Fixed path finding in Amiga SCI1 games

Added wrapper functions to read/write from dynmem segments, as these are
treated as BE in Amiga versions (as we treat them like raw data instead
of reg_t's), whereas the rest are LE. Thanks to waltervn and wjp for their
help on this

Changed paths:
    engines/sci/engine/kpathing.cpp
    engines/sci/util.cpp
    engines/sci/util.h



diff --git a/engines/sci/engine/kpathing.cpp b/engines/sci/engine/kpathing.cpp
index cb70cf9..c98d93c 100644
--- a/engines/sci/engine/kpathing.cpp
+++ b/engines/sci/engine/kpathing.cpp
@@ -264,9 +264,9 @@ struct PathfindingState {
 static Common::Point readPoint(SegmentRef list_r, int offset) {
 	Common::Point point;
 
-	if (list_r.isRaw) {
-		point.x = (int16)READ_LE_UINT16(list_r.raw + offset * POLY_POINT_SIZE);
-		point.y = (int16)READ_LE_UINT16(list_r.raw + offset * POLY_POINT_SIZE + 2);
+	if (list_r.isRaw) {	// dynmem blocks are raw
+		point.x = (int16)READ_SCI1ENDIAN_UINT16(list_r.raw + offset * POLY_POINT_SIZE);
+		point.y = (int16)READ_SCI1ENDIAN_UINT16(list_r.raw + offset * POLY_POINT_SIZE + 2);
 	} else {
 		point.x = list_r.reg[offset * 2].toUint16();
 		point.y = list_r.reg[offset * 2 + 1].toUint16();
@@ -275,9 +275,9 @@ static Common::Point readPoint(SegmentRef list_r, int offset) {
 }
 
 static void writePoint(SegmentRef ref, int offset, const Common::Point &point) {
-	if (ref.isRaw) {
-		WRITE_LE_UINT16(ref.raw + offset * POLY_POINT_SIZE, point.x);
-		WRITE_LE_UINT16(ref.raw + offset * POLY_POINT_SIZE + 2, point.y);
+	if (ref.isRaw) {	// dynmem blocks are raw
+		WRITE_SCI1ENDIAN_UINT16(ref.raw + offset * POLY_POINT_SIZE, point.x);
+		WRITE_SCI1ENDIAN_UINT16(ref.raw + offset * POLY_POINT_SIZE + 2, point.y);
 	} else {
 		ref.reg[offset * 2] = make_reg(0, point.x);
 		ref.reg[offset * 2 + 1] = make_reg(0, point.y);
diff --git a/engines/sci/util.cpp b/engines/sci/util.cpp
index f6a2465..6bc08e6 100644
--- a/engines/sci/util.cpp
+++ b/engines/sci/util.cpp
@@ -30,6 +30,20 @@
 
 namespace Sci {
 
+uint16 READ_SCI1ENDIAN_UINT16(const void *ptr) {
+	if (g_sci->getPlatform() == Common::kPlatformAmiga && getSciVersion() >= SCI_VERSION_1_EGA_ONLY && getSciVersion() <= SCI_VERSION_1_LATE)
+		return READ_BE_UINT16(ptr);
+
+	return READ_LE_UINT16(ptr);
+}
+
+void WRITE_SCI1ENDIAN_UINT16(void *ptr, uint16 val) {
+	if (g_sci->getPlatform() == Common::kPlatformAmiga && getSciVersion() >= SCI_VERSION_1_EGA_ONLY && getSciVersion() <= SCI_VERSION_1_LATE)
+		WRITE_BE_UINT16(ptr, val);
+
+	WRITE_LE_UINT16(ptr, val);
+}
+
 uint16 READ_SCI11ENDIAN_UINT16(const void *ptr) {
 	if (g_sci->getPlatform() == Common::kPlatformMacintosh && getSciVersion() >= SCI_VERSION_1_1)
 		return READ_BE_UINT16(ptr);
diff --git a/engines/sci/util.h b/engines/sci/util.h
index d9ced5c..d1f1c2f 100644
--- a/engines/sci/util.h
+++ b/engines/sci/util.h
@@ -30,6 +30,12 @@
 
 namespace Sci {
 
+// Wrappers for reading/writing integer values for SCI1 Amiga.
+// Amiga versions store big endian data in dynmem blocks, as
+// the game resources are in LE, but the actual system is BE.
+uint16 READ_SCI1ENDIAN_UINT16(const void *ptr);
+void WRITE_SCI1ENDIAN_UINT16(void *ptr, uint16 val);
+
 // Wrappers for reading integer values for SCI1.1+.
 // Mac versions have big endian data for some fields.
 uint16 READ_SCI11ENDIAN_UINT16(const void *ptr);


Commit: 00b37f8552962c26412eda347024db80de3d99ac
    https://github.com/scummvm/scummvm/commit/00b37f8552962c26412eda347024db80de3d99ac
Author: md5 (md5 at scummvm.org)
Date: 2011-03-03T09:38:30-08:00

Commit Message:
SCI: Simplified arithmetic reg_t operations, based on fingolfin's comments

- Folded all comparison operators in a single function, cmp()
- Simplified the + operator, and removed the SQ1 workaround, as it's not
needed anymore
- Removed the workaround for uninitialized variables in the * operator
- Removed division by zero workarounds in the / and % operators
- Added a better description of pointerComparisonWithInteger(), based
on fingolfin's description and comments. Also, changed the SCI versions
where this is used to SCI0-SCI1. The SCI1.1 case in QFG3 was a script
bug

Changed paths:
    engines/sci/engine/vm_types.cpp
    engines/sci/engine/vm_types.h



diff --git a/engines/sci/engine/vm_types.cpp b/engines/sci/engine/vm_types.cpp
index 85c83bb..c2119aa 100644
--- a/engines/sci/engine/vm_types.cpp
+++ b/engines/sci/engine/vm_types.cpp
@@ -35,7 +35,7 @@ reg_t reg_t::lookForWorkaround(const reg_t right) const {
 	SciTrackOriginReply originReply;
 	SciWorkaroundSolution solution = trackOriginAndFindWorkaround(0, arithmeticWorkarounds, &originReply);
 	if (solution.type == WORKAROUND_NONE)
-		error("arithmetic operation on non-integer (%04x:%04x, %04x:%04x) from method %s::%s (script %d, room %d, localCall %x)", 
+		error("Invalid arithmetic operation (params: %04x:%04x and %04x:%04x) from method %s::%s (script %d, room %d, localCall %x)", 
 		PRINT_REG(*this), PRINT_REG(right), originReply.objectName.c_str(), 
 		originReply.methodName.c_str(), originReply.scriptNr, g_sci->getEngineState()->currentRoomNumber(),
 		originReply.localCallOffset);
@@ -44,7 +44,7 @@ reg_t reg_t::lookForWorkaround(const reg_t right) const {
 }
 
 reg_t reg_t::operator+(const reg_t right) const {
-	if (isPointer()) {
+	if (isPointer() && right.isNumber()) {
 		// Pointer arithmetics. Only some pointer types make sense here
 		SegmentObj *mobj = g_sci->getEngineState()->_segMan->getSegmentObj(segment);
 
@@ -56,9 +56,6 @@ reg_t reg_t::operator+(const reg_t right) const {
 		case SEG_TYPE_SCRIPT:
 		case SEG_TYPE_STACK:
 		case SEG_TYPE_DYNMEM:
-			// Make sure that we are adding an offset to the pointer
-			if (!right.isNumber())
-				return lookForWorkaround(right);
 			return make_reg(segment, offset + right.toSint16());
 		default:
 			return lookForWorkaround(right);
@@ -66,19 +63,11 @@ reg_t reg_t::operator+(const reg_t right) const {
 	} else if (isNumber() && right.isPointer()) {
 		// Adding a pointer to a number, flip the order
 		return right + *this;
+	} else if (isNumber() && right.isNumber()) {
+		// Normal arithmetics
+		return make_reg(0, toSint16() + right.toSint16());
 	} else {
-		// Normal arithmetics. Make sure we're adding a number
-		if (right.isPointer())
-			return lookForWorkaround(right);
-		// If the current variable is uninitialized, it'll be set
-		// to zero in order to perform the operation. Such a case
-		// happens in SQ1, room 28, when throwing the water at Orat.
-		if (!isInitialized())
-			return make_reg(0, right.toSint16());
-		else if (!right.isInitialized())
-			return *this;
-		else
-			return make_reg(0, toSint16() + right.toSint16());
+		return lookForWorkaround(right);
 	}
 }
 
@@ -95,24 +84,19 @@ reg_t reg_t::operator-(const reg_t right) const {
 reg_t reg_t::operator*(const reg_t right) const {
 	if (isNumber() && right.isNumber())
 		return make_reg(0, toSint16() * right.toSint16());
-	else if (!isInitialized() || !right.isInitialized())
-		return NULL_REG;	// unitialized variables - always return 0
 	else
 		return lookForWorkaround(right);
 }
 
 reg_t reg_t::operator/(const reg_t right) const {
-	if (isNumber() && right.isNumber()) {
-		if (right.isNull())
-			return NULL_REG;	// division by zero
-		else
-			return make_reg(0, toSint16() / right.toSint16());
-	} else
+	if (isNumber() && right.isNumber() && !right.isNull())
+		return make_reg(0, toSint16() / right.toSint16());
+	else
 		return lookForWorkaround(right);
 }
 
 reg_t reg_t::operator%(const reg_t right) const {
-	if (isNumber() && right.isNumber()) {
+	if (isNumber() && right.isNumber() && !right.isNull()) {
 		// Support for negative numbers was added in Iceman, and perhaps in 
 		// SCI0 0.000.685 and later. Theoretically, this wasn't really used
 		// in SCI0, so the result is probably unpredictable. Such a case 
@@ -123,7 +107,7 @@ reg_t reg_t::operator%(const reg_t right) const {
 			warning("Modulo of a negative number has been requested for SCI0. This *could* lead to issues");
 		int16 value = toSint16();
 		int16 modulo = ABS(right.toSint16());
-		int16 result = (modulo != 0 ? value % modulo : 0);
+		int16 result = value % modulo;
 		if (result < 0)
 			result += modulo;
 		return make_reg(0, result);
@@ -157,6 +141,8 @@ uint16 reg_t::requireUint16() const {
 	if (isNumber())
 		return toUint16();
 	else
+		// The right parameter is NULL_REG because
+		// we're not comparing *this with anything here.
 		return lookForWorkaround(NULL_REG).toUint16();
 }
 
@@ -164,6 +150,8 @@ int16 reg_t::requireSint16() const {
 	if (isNumber())
 		return toSint16();
 	else
+		// The right parameter is NULL_REG because
+		// we're not comparing *this with anything here.
 		return lookForWorkaround(NULL_REG).toSint16();
 }
 
@@ -188,62 +176,35 @@ reg_t reg_t::operator^(const reg_t right) const {
 		return lookForWorkaround(right);
 }
 
-bool reg_t::operator>(const reg_t right) const {
-	if (isNumber() && right.isNumber())
-		return toSint16() > right.toSint16();
-	else if (isPointer() && segment == right.segment)
-		return toUint16() > right.toUint16();	// pointer comparison
-	else if (pointerComparisonWithInteger(right))
-		return true;
-	else if (right.pointerComparisonWithInteger(*this))
-		return false;
-	else
-		return lookForWorkaround(right).toSint16();
-}
-
-bool reg_t::operator<(const reg_t right) const {
-	if (isNumber() && right.isNumber())
-		return toSint16() < right.toSint16();
-	else if (isPointer() && segment == right.segment)
-		return toUint16() < right.toUint16();	// pointer comparison
-	else if (pointerComparisonWithInteger(right))
-		return false;
-	else if (right.pointerComparisonWithInteger(*this))
-		return true;
-	else
-		return lookForWorkaround(right).toSint16();
-}
-
-bool reg_t::gtU(const reg_t right) const {
-	if (isNumber() && right.isNumber())
-		return toUint16() > right.toUint16();
-	else if (isPointer() && segment == right.segment)
-		return toUint16() > right.toUint16();	// pointer comparison
-	else if (pointerComparisonWithInteger(right))
-		return true;
-	else if (right.pointerComparisonWithInteger(*this))
-		return false;
-	else
-		return lookForWorkaround(right).toSint16();
-}
-
-bool reg_t::ltU(const reg_t right) const {
-	if (isNumber() && right.isNumber())
-		return toUint16() < right.toUint16();
-	else if (isPointer() && segment == right.segment)
-		return toUint16() < right.toUint16();	// pointer comparison
-	else if (pointerComparisonWithInteger(right))
-		return false;
-	else if (right.pointerComparisonWithInteger(*this))
-		return true;
-	else
+int reg_t::cmp(const reg_t right, bool treatAsUnsigned) const {
+	if (segment == right.segment) { // can compare things in the same segment
+		if (treatAsUnsigned || !isNumber())
+			return toUint16() - right.toUint16();
+		else
+			return toSint16() - right.toSint16(); 
+	} else if (pointerComparisonWithInteger(right)) {
+		return 1;
+	} else if (right.pointerComparisonWithInteger(*this)) {
+		return -1;
+	} else
 		return lookForWorkaround(right).toSint16();
 }
 
 bool reg_t::pointerComparisonWithInteger(const reg_t right) const {
-	// SCI0 - SCI1.1 scripts use this to check whether a parameter is a pointer
-	// or a far text reference. It is used e.g. by the standard library Print
-	// function to distinguish two ways of calling it:
+	// This function handles the case where a script tries to compare a pointer
+	// to a number. Normally, we would not want to allow that. However, SCI0 -
+	// SCI1 scripts do this in order to distinguish pointers (to resources)
+	// from plain numbers. In our SCI implementation, such a check may seem
+	// pointless, as one can simply use the segment value to achieve this goal.
+	// But Sierra's SCI did not have the notion of segment IDs, so both pointer
+	// and numbers were simple integers.
+	// But for some things, scripts had (and have) to distinguish between
+	// numbers and pointers. Lacking the segment information, Sierra's
+	// developers resorted to a hack: If an integer is smaller than a certain
+	// bound, it can be assumed to be a number, otherwise it is assumed to be a
+	// pointer. This allowed them to implement polymorphic functions, such as
+	// the Print function, which can be called in two different ways, with a
+	// pointer or a far text reference:
 	//
 	// (Print "foo") // Pointer to a string
 	// (Print 420 5) // Reference to the fifth message in text resource 420
@@ -256,9 +217,8 @@ bool reg_t::pointerComparisonWithInteger(const reg_t right) const {
 	// Hoyle 3, Pachisi, when any opponent is about to talk
 	// SQ1, room 28, when throwing water at the Orat
 	// SQ1, room 58, when giving the ID card to the robot
-	// QFG3, room 440, when talking to Uhura
 	// Thus we check for all integers <= 2000
-	return (isPointer() && right.isNumber() && right.offset <= 2000 && getSciVersion() <= SCI_VERSION_1_1);
+	return (isPointer() && right.isNumber() && right.offset <= 2000 && getSciVersion() <= SCI_VERSION_1_LATE);
 }
 
 } // End of namespace Sci
diff --git a/engines/sci/engine/vm_types.h b/engines/sci/engine/vm_types.h
index 82c881d..b927df3 100644
--- a/engines/sci/engine/vm_types.h
+++ b/engines/sci/engine/vm_types.h
@@ -73,32 +73,38 @@ struct reg_t {
 		return (offset != x.offset) || (segment != x.segment);
 	}
 
-	bool operator>(const reg_t right) const;
+	bool operator>(const reg_t right) const {
+		return cmp(right, false) > 0;
+	}
+
 	bool operator>=(const reg_t right) const {
-		if (*this == right)
-			return true;
-		return *this > right;
+		return cmp(right, false) >= 0;
+	}
+
+	bool operator<(const reg_t right) const {
+		return cmp(right, false) < 0;
 	}
-	bool operator<(const reg_t right) const;
+
 	bool operator<=(const reg_t right) const {
-		if (*this == right)
-			return true;
-		return *this < right;
+		return cmp(right, false) <= 0;
 	}
 
 	// Same as the normal operators, but perform unsigned
 	// integer checking
-	bool gtU(const reg_t right) const;
+	bool gtU(const reg_t right) const {
+		return cmp(right, true) > 0;
+	}
+
 	bool geU(const reg_t right) const {
-		if (*this == right)
-			return true;
-		return gtU(right);
+		return cmp(right, true) >= 0;
+	}
+
+	bool ltU(const reg_t right) const {
+		return cmp(right, true) < 0;
 	}
-	bool ltU(const reg_t right) const;
+
 	bool leU(const reg_t right) const {
-		if (*this == right)
-			return true;
-		return ltU(right);
+		return cmp(right, true) <= 0;
 	}
 
 	// Arithmetic operators
@@ -124,6 +130,14 @@ struct reg_t {
 	reg_t operator^(const reg_t right) const;
 
 private:
+	/**
+	 * Compares two reg_t's.
+	 * Returns:
+	 * - a positive number if *this > right
+	 * - 0 if *this == right
+	 * - a negative number if *this < right
+	 */
+	int cmp(const reg_t right, bool treatAsUnsigned) const;
 	reg_t lookForWorkaround(const reg_t right) const;
 	bool pointerComparisonWithInteger(const reg_t right) const;
 };






More information about the Scummvm-git-logs mailing list