SCI: Added a custom modulo reg_t operator

This version only handles signed integers. The modulo operator was changed in
SCI0 late (Iceman, and perhaps all SCI0 0.000.685 and later) so that it handles
negative numbers as well. We need to see if there really is a need to keep two
different modulo operators (which will only be necessary if any SCI0 game asks
for the modulo of a negative number by mistake, or a number larger than 32767).
Thus, error out in such a case for SCI0, so that this can be investigated
properly

3 files changed, 27 insertions, 21 deletions

diff --git a/engines/sci/engine/vm.cpp b/engines/sci/engine/vm.cpp
index a6677ee633..69d046da7d 100644
--- a/engines/sci/engine/vm.cpp
+++ b/engines/sci/engine/vm.cpp
@@ -961,28 +961,10 @@ void run_vm(EngineState *s) {
 			s->r_acc = POP32() / s->r_acc;
 			break;
 
-		case op_mod: { // 0x05 (05)
-			if (getSciVersion() <= SCI_VERSION_0_LATE) {
-				uint16 modulo = s->r_acc.requireUint16();
-				uint16 value = POP32().requireUint16();
-				uint16 result = (modulo != 0 ? value % modulo : 0);
-				s->r_acc = make_reg(0, result);
-			} else {
-				// In Iceman (and perhaps from SCI0 0.000.685 onwards in general),
-				// handling for negative numbers was added. Since Iceman doesn't
-				// seem to have issues with the older code, we exclude it for now
-				// for simplicity's sake and use the new code for SCI01 and newer
-				// games. Fixes the battlecruiser mini game in SQ5 (room 850),
-				// bug #3035755
-				int16 modulo = ABS(s->r_acc.requireSint16());
-				int16 value = POP32().requireSint16();
-				int16 result = (modulo != 0 ? value % modulo : 0);
-				if (result < 0)
-					result += modulo;
-				s->r_acc = make_reg(0, result);
-			}
+		case op_mod: // 0x05 (05)
+			// we check for division by 0 inside the custom reg_t modulo operator
+			s->r_acc = POP32() % s->r_acc;
 			break;
-		}
 
 		case op_shr: // 0x06 (06)
 			// Shift right logical
diff --git a/engines/sci/engine/vm_types.cpp b/engines/sci/engine/vm_types.cpp
index a165bf7cc3..c5b106d212 100644
--- a/engines/sci/engine/vm_types.cpp
+++ b/engines/sci/engine/vm_types.cpp
@@ -113,6 +113,29 @@ reg_t reg_t::operator/(const reg_t right) const {
 		return lookForWorkaround(right);
 }
 
+reg_t reg_t::operator%(const reg_t right) const {
+	if (isNumber() && right.isNumber()) {
+		// 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 if we do find such a case, error out on purpose here,
+		// so that we address it properly, as the result is probably
+		// unpredictable. Such a case would indicate either a script bug, or
+		// a modulo on an unsigned integer larger than 32767. In any case,
+		// such a case should be investigated, instead of being silently
+		// accepted.
+		if (getSciVersion() <= SCI_VERSION_0_LATE && (toSint16() < 0 || right.toSint16() < 0))
+			error("Modulo of a negative number has been requested for SCI0. "
+				  "Please report this error to the ScummVM team");
+		int16 value = toSint16();
+		int16 modulo = ABS(right.toSint16());
+		int16 result = (modulo != 0 ? value % modulo : 0);
+		if (result < 0)
+			result += modulo;
+		return make_reg(0, result);
+	} else
+		return lookForWorkaround(right);
+}
+
 reg_t reg_t::operator>>(const reg_t right) const {
 	if (isNumber() && right.isNumber())
 		return make_reg(0, toUint16() >> right.toUint16());
diff --git a/engines/sci/engine/vm_types.h b/engines/sci/engine/vm_types.h
index 0187906670..ac23bbe7dc 100644
--- a/engines/sci/engine/vm_types.h
+++ b/engines/sci/engine/vm_types.h
@@ -108,6 +108,7 @@ struct reg_t {
 	reg_t operator-(const reg_t right) const;
 	reg_t operator*(const reg_t right) const;
 	reg_t operator/(const reg_t right) const;
+	reg_t operator%(const reg_t right) const;
 	reg_t operator>>(const reg_t right) const;
 	reg_t operator<<(const reg_t right) const;