SCI32: Change storage type of int16 arrays to hold reg_ts instead

Memory references and integers in SSCI are both 16-bit numbers,
so game scripts frequently (incorrectly) use an IntArray instead
of an IDArray for holding references. Since references in ScummVM
are 32-bit reg_ts, IntArray entries must be large enough to hold
reg_ts in order to be compatible with game scripts that store
references in integer arrays.

The alternative solution is to find and patch all incorrect use of
IntArray across all games. This is possible, but a bit risky from
a save game stability perspective, since incorrect IntArray usage
is sometimes not apparent until well after the array is
instantiated (like GK1's global interview array).

This change invalidates existing SCI32 save games.

9 files changed, 59 insertions, 156 deletions

diff --git a/engines/sci/console.cpp b/engines/sci/console.cpp
index 34109fff39..39fa95a956 100644
--- a/engines/sci/console.cpp
+++ b/engines/sci/console.cpp
@@ -2812,7 +2812,7 @@ bool Console::cmdViewReference(int argc, const char **argv) {
 							arrayType = "byte";
 							break;
 						case kArrayTypeInt16:
-							arrayType = "int16";
+							arrayType = "int16 (as reg_t)";
 							break;
 						case kArrayTypeString:
 							arrayType = "string";
@@ -2823,24 +2823,16 @@ bool Console::cmdViewReference(int argc, const char **argv) {
 					}
 					debugPrintf("SCI32 %s array (%u entries):\n", arrayType, array->size());
 					switch (array->getType()) {
-					case kArrayTypeID:
+					case kArrayTypeInt16:
+					case kArrayTypeID: {
 						hexDumpReg((const reg_t *)array->getRawData(), array->size(), 4, 0, true);
 						break;
+					}
 					case kArrayTypeByte:
 					case kArrayTypeString: {
 						Common::hexdump((const byte *)array->getRawData(), array->size(), 16, 0);
 						break;
 					}
-					case kArrayTypeInt16: {
-						const int16 *data = (const int16 *)array->getRawData();
-						for (int i = 0; i < array->size(); ++i) {
-							debugN("% 6d ", *data++);
-							if ((i % 8) == 0) {
-								debugN("\n");
-							}
-						}
-						break;
-					}
 					default:
 						break;
 					}
diff --git a/engines/sci/engine/kevent.cpp b/engines/sci/engine/kevent.cpp
index c4aa4c5e46..a00630622e 100644
--- a/engines/sci/engine/kevent.cpp
+++ b/engines/sci/engine/kevent.cpp
@@ -383,10 +383,10 @@ reg_t kSetHotRectangles(EngineState *s, int argc, reg_t *argv) {
 	rects.resize(numRects);
 
 	for (int16 i = 0; i < numRects; ++i) {
-		rects[i].left   = hotRects.int16At(i * 4);
-		rects[i].top    = hotRects.int16At(i * 4 + 1);
-		rects[i].right  = hotRects.int16At(i * 4 + 2) + 1;
-		rects[i].bottom = hotRects.int16At(i * 4 + 3) + 1;
+		rects[i].left   = hotRects.getAsInt16(i * 4);
+		rects[i].top    = hotRects.getAsInt16(i * 4 + 1);
+		rects[i].right  = hotRects.getAsInt16(i * 4 + 2) + 1;
+		rects[i].bottom = hotRects.getAsInt16(i * 4 + 3) + 1;
 	}
 
 	g_sci->getEventManager()->setHotRectanglesActive(true);
diff --git a/engines/sci/engine/kfile.cpp b/engines/sci/engine/kfile.cpp
index 386c4bcdfa..6aad256664 100644
--- a/engines/sci/engine/kfile.cpp
+++ b/engines/sci/engine/kfile.cpp
@@ -1327,11 +1327,11 @@ reg_t kGetSaveFiles32(EngineState *s, int argc, reg_t *argv) {
 		const SavegameDesc &save = saves[i];
 		char *target = &descriptions.charAt(SCI_MAX_SAVENAME_LENGTH * i);
 		Common::strlcpy(target, save.name, SCI_MAX_SAVENAME_LENGTH);
-		saveIds.int16At(i) = save.id - kSaveIdShift;
+		saveIds.setFromInt16(i, save.id - kSaveIdShift);
 	}
 
 	descriptions.charAt(SCI_MAX_SAVENAME_LENGTH * saves.size()) = '\0';
-	saveIds.int16At(saves.size()) = 0;
+	saveIds.setFromInt16(saves.size(), 0);
 
 	return make_reg(0, saves.size());
 }
diff --git a/engines/sci/engine/kpathing.cpp b/engines/sci/engine/kpathing.cpp
index 667f77f126..937b1cfc2f 100644
--- a/engines/sci/engine/kpathing.cpp
+++ b/engines/sci/engine/kpathing.cpp
@@ -285,13 +285,6 @@ struct PathfindingState {
 static Common::Point readPoint(SegmentRef list_r, int offset) {
 	Common::Point point;
 
-#ifdef ENABLE_SCI32
-	if (getSciVersion() >= SCI_VERSION_2) {
-		point.x = READ_UINT16(list_r.raw + offset * POLY_POINT_SIZE + 0);
-		point.y = READ_UINT16(list_r.raw + offset * POLY_POINT_SIZE + 2);
-	} else
-#endif
-
 	if (list_r.isRaw) {	// dynmem blocks are raw
 		point.x = (int16)READ_SCIENDIAN_UINT16(list_r.raw + offset * POLY_POINT_SIZE);
 		point.y = (int16)READ_SCIENDIAN_UINT16(list_r.raw + offset * POLY_POINT_SIZE + 2);
@@ -303,12 +296,6 @@ static Common::Point readPoint(SegmentRef list_r, int offset) {
 }
 
 static void writePoint(SegmentRef ref, int offset, const Common::Point &point) {
-#ifdef ENABLE_SCI32
-	if (getSciVersion() >= SCI_VERSION_2) {
-		WRITE_UINT16(ref.raw + offset * POLY_POINT_SIZE + 0, point.x);
-		WRITE_UINT16(ref.raw + offset * POLY_POINT_SIZE + 2, point.y);
-	} else
-#endif
 	if (ref.isRaw) {	// dynmem blocks are raw
 		WRITE_SCIENDIAN_UINT16(ref.raw + offset * POLY_POINT_SIZE, point.x);
 		WRITE_SCIENDIAN_UINT16(ref.raw + offset * POLY_POINT_SIZE + 2, point.y);
diff --git a/engines/sci/engine/savegame.cpp b/engines/sci/engine/savegame.cpp
index f01e2a677e..2e4da46b70 100644
--- a/engines/sci/engine/savegame.cpp
+++ b/engines/sci/engine/savegame.cpp
@@ -685,20 +685,16 @@ void SciArray::saveLoadWithSerializer(Common::Serializer &s) {
 	}
 
 	switch (_type) {
-	case kArrayTypeByte:
-	case kArrayTypeString:
-		s.syncBytes((byte *)_data, savedSize);
-		break;
 	case kArrayTypeInt16:
-		for (int i = 0; i < savedSize; ++i) {
-			s.syncAsUint16LE(((int16 *)_data)[i]);
-		}
-		break;
 	case kArrayTypeID:
 		for (int i = 0; i < savedSize; ++i) {
 			syncWithSerializer(s, ((reg_t *)_data)[i]);
 		}
 		break;
+	case kArrayTypeByte:
+	case kArrayTypeString:
+		s.syncBytes((byte *)_data, savedSize);
+		break;
 	default:
 		error("Attempt to sync invalid SciArray type %d", _type);
 	}
diff --git a/engines/sci/engine/script_patches.cpp b/engines/sci/engine/script_patches.cpp
index 4773646b57..f2f290fa9e 100644
--- a/engines/sci/engine/script_patches.cpp
+++ b/engines/sci/engine/script_patches.cpp
@@ -995,32 +995,8 @@ static const uint16 gk1PatchDay10GabrielDressUp[] = {
 	PATCH_END
 };
 
-// GK1 initializes the global interrogation array with an int16 array, which
-// happens to work in SSCI because object references are int16s, but in ScummVM
-// object references are reg_ts, so this array needs to be created as an IDArray
-// instead
-static const uint16 gk1SignatureInterrogationArray1[] = {
-	0x38, SIG_SELECTOR16(new), // pushi new
-	0x78,                      // push1
-	SIG_MAGICDWORD,
-	0x39, 0x0f,                // pushi 15
-	0x51, 0x0a,                // class IntArray
-	SIG_END
-};
-
-static const uint16 gk1PatchInterrogationArray1[] = {
-	PATCH_ADDTOOFFSET(+3),  // pushi new
-	PATCH_ADDTOOFFSET(+1),  // push1
-	PATCH_ADDTOOFFSET(+2),  // pushi 15
-	0x51, 0x0b,             // class IDArray
-	PATCH_END
-};
-
 //          script, description,                                      signature                         patch
 static const SciScriptPatcherEntry gk1Signatures[] = {
-	{  true,    10, "fix interrogation array type 1/3",            1, gk1SignatureInterrogationArray1,  gk1PatchInterrogationArray1 },
-	{  true,    50, "fix interrogation array type 2/3",            1, gk1SignatureInterrogationArray1,  gk1PatchInterrogationArray1 },
-	{  true,    93, "fix interrogation array type 3/3",            1, gk1SignatureInterrogationArray1,  gk1PatchInterrogationArray1 },
 	{  true,    51, "interrogation bug",                           1, gk1SignatureInterrogationBug,     gk1PatchInterrogationBug },
 	{  true,   212, "day 5 drum book dialogue error",              1, gk1SignatureDay5DrumBookDialogue, gk1PatchDay5DrumBookDialogue },
 	{  true,   212, "day 5 phone freeze",                          1, gk1SignatureDay5PhoneFreeze,      gk1PatchDay5PhoneFreeze },
@@ -4560,30 +4536,10 @@ static const SciScriptPatcherEntry sq5Signatures[] = {
 #pragma mark -
 #pragma mark Space Quest 6
 
-// When pressing number buttons on the Holocabana controls, View objects are
-// put in an int16 array. This happens to work in SSCI, but ScummVM requires a
-// proper IDArray because reg_t is larger than 16 bits.
-static const uint16 sq6HoloIntArraySignature[] = {
-	0x38, SIG_SELECTOR16(new), // pushi new
-	0x76,                      // push0
-	0x51, 0x0b,                // class IntArray
-	SIG_MAGICDWORD,
-	0x4a, SIG_UINT16(0x04),    // send 4
-	0xa3, 0x06,                // sal local[6]
-	SIG_END
-};
-
-static const uint16 sq6HoloIntArrayPatch[] = {
-	PATCH_ADDTOOFFSET(+4),      // pushi new; push0
-	0x51, 0x0c,                 // class IDArray
-	PATCH_END
-};
-
 //          script, description,                                      signature                        patch
 static const SciScriptPatcherEntry sq6Signatures[] = {
 	{  true,    15, "invalid array construction",                  1, sci21IntArraySignature,          sci21IntArrayPatch },
 	{  true,    22, "invalid array construction",                  1, sci21IntArraySignature,          sci21IntArrayPatch },
-	{  true,   400, "invalid array type",                          1, sq6HoloIntArraySignature,        sq6HoloIntArrayPatch },
 	{  true,   460, "invalid array construction",                  1, sci21IntArraySignature,          sci21IntArrayPatch },
 	{  true,   510, "invalid array construction",                  1, sci21IntArraySignature,          sci21IntArrayPatch },
 	{  true, 64990, "increase number of save games",               1, sci2NumSavesSignature1,          sci2NumSavesPatch1 },
@@ -4595,30 +4551,8 @@ static const SciScriptPatcherEntry sq6Signatures[] = {
 #pragma mark -
 #pragma mark Torins Passage
 
-// Torin initializes the inventory with an int16 array, which happens to work
-// in SSCI because object references are int16s, but in ScummVM object
-// references are reg_ts, so this array needs to be created as an IDArray
-// instead
-static const uint16 torinInventItemSlotsSignature[] = {
-	0x38, SIG_SELECTOR16(new), // pushi new
-	0x78,                      // push1
-	SIG_MAGICDWORD,
-	0x67, 0x2e,                // pTos $2e (invSlotsTot)
-	0x51, 0x0b,                // class IntArray
-	SIG_END
-};
-
-static const uint16 torinInventItemSlotsPatch[] = {
-	PATCH_ADDTOOFFSET(+3),  // pushi new
-	PATCH_ADDTOOFFSET(+1),  // push1
-	PATCH_ADDTOOFFSET(+2),  // pTos $2e (invSlotsTot)
-	0x51, 0x0c,             // class IDArray
-	PATCH_END
-};
-
 //          script, description,                                      signature                         patch
 static const SciScriptPatcherEntry torinSignatures[] = {
-	{  true, 64895, "fix inventory array type",                    1, torinInventItemSlotsSignature,    torinInventItemSlotsPatch },
 	{  true, 64990, "increase number of save games",               1, sci2NumSavesSignature1,           sci2NumSavesPatch1 },
 	{  true, 64990, "increase number of save games",               1, sci2NumSavesSignature2,           sci2NumSavesPatch2 },
 	SCI_SIGNATUREENTRY_TERMINATOR
diff --git a/engines/sci/engine/segment.cpp b/engines/sci/engine/segment.cpp
index 11331f0599..32f16148dc 100644
--- a/engines/sci/engine/segment.cpp
+++ b/engines/sci/engine/segment.cpp
@@ -253,10 +253,10 @@ SegmentRef ArrayTable::dereference(reg_t pointer) {
 	SegmentRef ret;
 
 	SciArray &array = at(pointer.getOffset());
-	const bool isRaw = array.getType() != kArrayTypeID;
+	const bool isRaw = array.getType() == kArrayTypeByte || array.getType() == kArrayTypeString;
 
 	ret.isRaw = isRaw;
-	ret.maxSize = isRaw ? array.byteSize() : array.size();
+	ret.maxSize = array.byteSize();
 	if (isRaw) {
 		ret.raw = (byte *)array.getRawData();
 	} else {
diff --git a/engines/sci/engine/segment.h b/engines/sci/engine/segment.h
index 6ecfbc7881..e62b4fa8eb 100644
--- a/engines/sci/engine/segment.h
+++ b/engines/sci/engine/segment.h
@@ -475,15 +475,13 @@ public:
 	/**
 	 * Sets the type of this array. The type of the array may only be set once.
 	 */
-	void setType(SciArrayType type) {
+	void setType(const SciArrayType type) {
 		assert(_type == kArrayTypeInvalid);
 		switch(type) {
+		case kArrayTypeInt16:
 		case kArrayTypeID:
 			_elementSize = sizeof(reg_t);
 			break;
-		case kArrayTypeInt16:
-			_elementSize = sizeof(int16);
-			break;
 		case kArrayTypeString:
 			_elementSize = sizeof(char);
 			break;
@@ -551,12 +549,11 @@ public:
 
 		switch(_type) {
 		case kArrayTypeInt16:
-			return make_reg(0, ((int16 *)_data)[index]);
+		case kArrayTypeID:
+			return ((reg_t *)_data)[index];
 		case kArrayTypeByte:
 		case kArrayTypeString:
 			return make_reg(0, ((byte *)_data)[index]);
-		case kArrayTypeID:
-			return ((reg_t *)_data)[index];
 		default:
 			error("Invalid array type %d", _type);
 		}
@@ -574,26 +571,23 @@ public:
 
 		switch(_type) {
 		case kArrayTypeInt16:
-			((int16 *)_data)[index] = value.toSint16();
+		case kArrayTypeID:
+			((reg_t *)_data)[index] = value;
 			break;
 		case kArrayTypeByte:
 		case kArrayTypeString:
 			((byte *)_data)[index] = value.toSint16();
 			break;
-		case kArrayTypeID:
-			((reg_t *)_data)[index] = value;
-			break;
 		default:
 			error("Invalid array type %d", _type);
 		}
 	}
 
 	/**
-	 * Returns a reference to the byte at the given index. Only valid for
-	 * string and byte arrays.
+	 * Gets the value at the given index as an int16.
 	 */
-	byte &byteAt(const uint16 index) {
-		assert(_type == kArrayTypeString || _type == kArrayTypeByte);
+	int16 getAsInt16(const uint16 index) {
+		assert(_type == kArrayTypeInt16);
 
 		if (getSciVersion() >= SCI_VERSION_3) {
 			resize(index);
@@ -601,14 +595,31 @@ public:
 			assert(index < _size);
 		}
 
-		return ((byte *)_data)[index];
+		const reg_t value = ((reg_t *)_data)[index];
+		assert(value.isNumber());
+		return value.toSint16();
 	}
 
 	/**
-	 * Returns a reference to the char at the given index. Only valid for
+	 * Sets the value at the given index from an int16.
+	 */
+	void setFromInt16(const uint16 index, const int16 value) {
+		assert(_type == kArrayTypeInt16);
+
+		if (getSciVersion() >= SCI_VERSION_3) {
+			resize(index + 1);
+		} else {
+			assert(index < _size);
+		}
+
+		((reg_t *)_data)[index] = make_reg(0, value);
+	}
+
+	/**
+	 * Returns a reference to the byte at the given index. Only valid for
 	 * string and byte arrays.
 	 */
-	char &charAt(const uint16 index) {
+	byte &byteAt(const uint16 index) {
 		assert(_type == kArrayTypeString || _type == kArrayTypeByte);
 
 		if (getSciVersion() >= SCI_VERSION_3) {
@@ -617,15 +628,15 @@ public:
 			assert(index < _size);
 		}
 
-		return ((char *)_data)[index];
+		return ((byte *)_data)[index];
 	}
 
 	/**
-	 * Returns a reference to the int16 at the given index. Only valid for int16
-	 * arrays.
+	 * Returns a reference to the char at the given index. Only valid for
+	 * string and byte arrays.
 	 */
-	int16 &int16At(const uint16 index) {
-		assert(_type == kArrayTypeInt16);
+	char &charAt(const uint16 index) {
+		assert(_type == kArrayTypeString || _type == kArrayTypeByte);
 
 		if (getSciVersion() >= SCI_VERSION_3) {
 			resize(index);
@@ -633,15 +644,15 @@ public:
 			assert(index < _size);
 		}
 
-		return ((int16 *)_data)[index];
+		return ((char *)_data)[index];
 	}
 
 	/**
 	 * Returns a reference to the reg_t at the given index. Only valid for ID
-	 * arrays.
+	 * and int16 arrays.
 	 */
 	reg_t &IDAt(const uint16 index) {
-		assert(_type == kArrayTypeID);
+		assert(_type == kArrayTypeID || _type == kArrayTypeInt16);
 
 		if (getSciVersion() >= SCI_VERSION_3) {
 			resize(index);
@@ -660,18 +671,7 @@ public:
 		resize(index + count);
 
 		switch (_type) {
-		case kArrayTypeInt16: {
-			const reg_t *source = values;
-			int16 *target = (int16 *)_data + index;
-			while (count--) {
-				if (!source->isNumber()) {
-					error("Non-number %04x:%04x sent to int16 array", PRINT_REG(*source));
-				}
-				*target++ = source->toSint16();
-				++source;
-			}
-			break;
-		}
+		case kArrayTypeInt16:
 		case kArrayTypeID: {
 			const reg_t *source = values;
 			reg_t *target = (reg_t *)_data + index;
@@ -714,14 +714,7 @@ public:
 		resize(index + count);
 
 		switch (_type) {
-		case kArrayTypeInt16: {
-			const int16 fillValue = value.toSint16();
-			int16 *target = (int16 *)_data + index;
-			while (count--) {
-				*target++ = fillValue;
-			}
-			break;
-		}
+		case kArrayTypeInt16:
 		case kArrayTypeID: {
 			reg_t *target = (reg_t *)_data + index;
 			while (count--) {
@@ -1129,8 +1122,9 @@ public:
 		const int length = getWidth() * getHeight();
 		uint8 *pixel = getPixels();
 		for (int i = 0; i < length; ++i) {
-			uint8 color = clut.int16At(*pixel);
-			*pixel++ = color;
+			const int16 color = clut.getAsInt16(*pixel);
+			assert(color >= 0 && color <= 255);
+			*pixel++ = (uint8)color;
 		}
 	}
 };
diff --git a/engines/sci/graphics/transitions32.cpp b/engines/sci/graphics/transitions32.cpp
index bffbd546b3..ee230f50a8 100644
--- a/engines/sci/graphics/transitions32.cpp
+++ b/engines/sci/graphics/transitions32.cpp
@@ -280,7 +280,7 @@ void GfxTransitions32::kernelSetShowStyle(const uint16 argc, const reg_t planeOb
 				if (rangeCount > 0) {
 					entry->fadeColorRanges = new uint16[rangeCount];
 					for (size_t i = 0; i < rangeCount; ++i) {
-						entry->fadeColorRanges[i] = table.int16At(i);
+						entry->fadeColorRanges[i] = table.getAsInt16(i);
 					}
 				}
 			}