SCI: Convert Object to use Common::Array for SCI3

In SCI3, index-to-selector tables no longer exist in compiled
object data (instead, the SCI3 VM uses selectors directly and
object data contains a bit map of valid selectors). In ScummVM,
the table is generated by Object::initSelectorsSci3 for
compatibility with the design of the ScummVM SCI VM. For
consistency, _baseVars is converted to use a standard container,
which works for all SCI versions.

The table for SCI3 property offsets is also changed to use a
standard container instead of manually managing the memory with
malloc/free.

4 files changed, 71 insertions, 63 deletions

diff --git a/engines/sci/engine/object.cpp b/engines/sci/engine/object.cpp
index 621810d856..47d551980e 100644
--- a/engines/sci/engine/object.cpp
+++ b/engines/sci/engine/object.cpp
@@ -62,20 +62,42 @@ void Object::init(const SciSpan<const byte> &buf, reg_t obj_pos, bool initVariab
 	if (getSciVersion() <= SCI_VERSION_1_LATE) {
 		const SciSpan<const byte> header = buf.subspan(obj_pos.getOffset() - kOffsetHeaderSize);
 		_variables.resize(header.getUint16LEAt(kOffsetHeaderSelectorCounter));
-		_baseVars = _baseObj.subspan<const uint16>(_variables.size() * sizeof(uint16));
+
+		// Non-class objects do not have a baseVars section
+		const uint16 infoSelector = data.getUint16SEAt((_offset + 2) * sizeof(uint16));
+		if (infoSelector & kInfoFlagClass) {
+			_baseVars.reserve(_variables.size());
+			uint baseVarsOffset = _variables.size() * sizeof(uint16);
+			for (uint i = 0; i < _variables.size(); ++i) {
+				_baseVars.push_back(data.getUint16SEAt(baseVarsOffset));
+				baseVarsOffset += sizeof(uint16);
+			}
+		}
+
 		_methodCount = data.getUint16LEAt(header.getUint16LEAt(kOffsetHeaderFunctionArea) - 2);
 		for (int i = 0; i < _methodCount * 2 + 2; ++i) {
 			_baseMethod.push_back(data.getUint16SEAt(header.getUint16LEAt(kOffsetHeaderFunctionArea) + i * 2));
 		}
 	} else if (getSciVersion() >= SCI_VERSION_1_1 && getSciVersion() <= SCI_VERSION_2_1_LATE) {
 		_variables.resize(data.getUint16SEAt(2));
-		_baseVars = buf.subspan<const uint16>(data.getUint16SEAt(4), _variables.size() * sizeof(uint16));
+
+		// Non-class objects do not have a baseVars section
+		const uint16 infoSelector = data.getUint16SEAt((_offset + 2) * sizeof(uint16));
+		if (infoSelector & kInfoFlagClass) {
+			_baseVars.reserve(_variables.size());
+			uint baseVarsOffset = data.getUint16SEAt(4);
+			for (uint i = 0; i < _variables.size(); ++i) {
+				_baseVars.push_back(buf.getUint16SEAt(baseVarsOffset));
+				baseVarsOffset += sizeof(uint16);
+			}
+		}
+
 		_methodCount = buf.getUint16SEAt(data.getUint16SEAt(6));
 		for (int i = 0; i < _methodCount * 2 + 3; ++i) {
 			_baseMethod.push_back(buf.getUint16SEAt(data.getUint16SEAt(6) + i * 2));
 		}
 	} else if (getSciVersion() == SCI_VERSION_3) {
-		initSelectorsSci3(buf);
+		initSelectorsSci3(buf, initVariables);
 	}
 
 	if (initVariables) {
@@ -83,7 +105,7 @@ void Object::init(const SciSpan<const byte> &buf, reg_t obj_pos, bool initVariab
 			for (uint i = 0; i < _variables.size(); i++)
 				_variables[i] = make_reg(0, data.getUint16SEAt(i * 2));
 		} else {
-			_infoSelectorSci3 = make_reg(0, _baseObj.getUint16SEAt(10));
+			_infoSelectorSci3 = make_reg(0, data.getUint16SEAt(10));
 		}
 	}
 }
@@ -93,20 +115,20 @@ const Object *Object::getClass(SegManager *segMan) const {
 }
 
 int Object::locateVarSelector(SegManager *segMan, Selector slc) const {
-	SciSpan<const byte> buf;
+	const Common::Array<uint16> *buf;
 	uint varnum = 0;
 
 	if (getSciVersion() <= SCI_VERSION_2_1_LATE) {
 		const Object *obj = getClass(segMan);
 		varnum = getSciVersion() <= SCI_VERSION_1_LATE ? getVarCount() : obj->getVariable(1).toUint16();
-		buf = obj->_baseVars.subspan<const byte>(0);
+		buf = &obj->_baseVars;
 	} else if (getSciVersion() == SCI_VERSION_3) {
 		varnum = _variables.size();
-		buf = _baseVars.subspan<const byte>(0);
+		buf = &_baseVars;
 	}
 
 	for (uint i = 0; i < varnum; i++)
-		if (buf.getUint16SEAt(i << 1) == slc) // Found it?
+		if ((*buf)[i] == slc) // Found it?
 			return i; // report success
 
 	return -1; // Failed
@@ -117,7 +139,7 @@ bool Object::relocateSci0Sci21(SegmentId segment, int location, size_t scriptSiz
 }
 
 bool Object::relocateSci3(SegmentId segment, uint32 location, int offset, size_t scriptSize) {
-	assert(_propertyOffsetsSci3);
+	assert(_propertyOffsetsSci3.size());
 
 	for (uint i = 0; i < _variables.size(); ++i) {
 		if (location == _propertyOffsetsSci3[i]) {
@@ -147,7 +169,7 @@ int Object::propertyOffsetToId(SegManager *segMan, int propertyOffset) const {
 		if (!isClass())
 			obj = segMan->getObject(getSuperClassSelector());
 
-		return obj->_baseVars.subspan<const byte>(0).getUint16SEAt(propertyOffset);
+		return obj->_baseVars[propertyOffset >> 1];
 	}
 }
 
@@ -272,7 +294,7 @@ bool Object::mustSetViewVisible(const int index) const {
 }
 #endif
 
-void Object::initSelectorsSci3(const SciSpan<const byte> &buf) {
+void Object::initSelectorsSci3(const SciSpan<const byte> &buf, const bool initVariables) {
 	const SciSpan<const byte> groupInfo = _baseObj.subspan(16);
 	const SciSpan<const byte> selectorBase = groupInfo.subspan(EXTRA_GROUPS * 32 * 2);
 	int groups = g_sci->getKernel()->getSelectorNamesSize()/32;
@@ -315,9 +337,9 @@ void Object::initSelectorsSci3(const SciSpan<const byte> &buf) {
 	}
 
 	_variables.resize(properties);
-	uint16 *propertyIds = (uint16 *)malloc(sizeof(uint16) * properties);
+	_propertyOffsetsSci3.resize(properties);
+	_baseVars.resize(properties);
 //	uint16 *methodOffsets = (uint16 *)malloc(sizeof(uint16) * 2 * methods);
-	uint32 *propertyOffsets = (uint32 *)malloc(sizeof(uint32) * properties);
 	int propertyCounter = 0;
 	int methodCounter = 0;
 
@@ -335,18 +357,12 @@ void Object::initSelectorsSci3(const SciSpan<const byte> &buf) {
 			for (int bit = 2; bit < 32; ++bit) {
 				int value = seeker.getUint16SEAt(bit * 2);
 				if (typeMask & (1 << bit)) { // Property
-
-					// FIXME: We really shouldn't be doing endianness
-					// conversion here; instead, propertyIds should be converted
-					// to a Common::Array, like _baseMethod already is
-					// This interim solution fixes playing SCI3 PC games
-					// on Big Endian platforms
-
-					WRITE_SCI11ENDIAN_UINT16(&propertyIds[propertyCounter],
-					                         groupBaseId + bit);
-					_variables[propertyCounter] = make_reg(0, value);
+					_baseVars[propertyCounter] = groupBaseId + bit;
+					if (initVariables) {
+						_variables[propertyCounter] = make_reg(0, value);
+					}
 					uint32 propertyOffset = (seeker + bit * 2) - buf;
-					propertyOffsets[propertyCounter] = propertyOffset;
+					_propertyOffsetsSci3[propertyCounter] = propertyOffset;
 					++propertyCounter;
 				} else if (value != 0xffff) { // Method
 					_baseMethod.push_back(groupBaseId + bit);
@@ -361,12 +377,11 @@ void Object::initSelectorsSci3(const SciSpan<const byte> &buf) {
 		}
 	}
 
-	_speciesSelectorSci3 = make_reg(0, _baseObj.getUint16SEAt(4));
-	_superClassPosSci3 = make_reg(0, _baseObj.getUint16SEAt(8));
-
-	_baseVars = SciSpan<const uint16>(propertyIds, properties);
+	if (initVariables) {
+		_speciesSelectorSci3 = make_reg(0, _baseObj.getUint16SEAt(4));
+		_superClassPosSci3 = make_reg(0, _baseObj.getUint16SEAt(8));
+	}
 	_methodCount = methods;
-	_propertyOffsetsSci3 = propertyOffsets;
 	//_methodOffsetsSci3 = methodOffsets;
 }
 
diff --git a/engines/sci/engine/object.h b/engines/sci/engine/object.h
index 20b1e01776..8957a61799 100644
--- a/engines/sci/engine/object.h
+++ b/engines/sci/engine/object.h
@@ -73,24 +73,13 @@ enum ObjectOffsets {
 
 class Object {
 public:
-	Object() {
-		_offset = getSciVersion() < SCI_VERSION_1_1 ? 0 : 5;
-		_flags = 0;
-		_baseObj.clear();
-		_baseVars.clear();
-		_methodCount = 0;
-		_propertyOffsetsSci3 = nullptr;
-	}
-
-	~Object() {
-		if (getSciVersion() == SCI_VERSION_3) {
-			// TODO: This is super gross
-			free(const_cast<uint16 *>(_baseVars.data()));
-			_baseVars.clear();
-			free(_propertyOffsetsSci3);
-			_propertyOffsetsSci3 = nullptr;
-		}
-	}
+	Object() :
+		_offset(getSciVersion() < SCI_VERSION_1_1 ? 0 : 5),
+		_flags(0),
+		_baseObj(),
+		_baseVars(),
+		_methodCount(0),
+		_propertyOffsetsSci3() {}
 
 	Object &operator=(const Object &other) {
 		_baseObj = other._baseObj;
@@ -100,21 +89,14 @@ public:
 		_flags = other._flags;
 		_offset = other._offset;
 		_pos = other._pos;
+		_baseVars = other._baseVars;
 
 		if (getSciVersion() == SCI_VERSION_3) {
-			uint16 *baseVars = (uint16 *)malloc(other._baseVars.byteSize());
-			other._baseVars.unsafeCopyDataTo(baseVars);
-			_baseVars = SciSpan<const uint16>(baseVars, other._baseVars.size());
-
-			_propertyOffsetsSci3 = (uint32 *)malloc(sizeof(uint32) * _variables.size());
-			memcpy(_propertyOffsetsSci3, other._propertyOffsetsSci3, sizeof(uint32) * _variables.size());
-
+			_propertyOffsetsSci3 = other._propertyOffsetsSci3;
 			_superClassPosSci3 = other._superClassPosSci3;
 			_speciesSelectorSci3 = other._speciesSelectorSci3;
 			_infoSelectorSci3 = other._infoSelectorSci3;
 			_mustSetViewVisible = other._mustSetViewVisible;
-		} else {
-			_baseVars = other._baseVars;
 		}
 
 		return *this;
@@ -225,7 +207,7 @@ public:
 			error("setClassScriptSelector called for SCI3");
 	}
 
-	Selector getVarSelector(uint16 i) const { return _baseVars.getUint16SEAt(i); }
+	Selector getVarSelector(uint16 i) const { return _baseVars[i]; }
 
 	reg_t getFunction(uint16 i) const {
 		uint16 offset = (getSciVersion() < SCI_VERSION_1_1) ? _methodCount + 1 + i : i * 2 + 2;
@@ -282,7 +264,7 @@ public:
 	void cloneFromObject(const Object *obj) {
 		_baseObj = obj ? obj->_baseObj : SciSpan<const byte>();
 		_baseMethod = obj ? obj->_baseMethod : Common::Array<uint16>();
-		_baseVars = obj ? obj->_baseVars : SciSpan<const uint16>();
+		_baseVars = obj ? obj->_baseVars : Common::Array<uint16>();
 	}
 
 	bool relocateSci0Sci21(SegmentId segment, int location, size_t scriptSize);
@@ -300,12 +282,12 @@ public:
 #endif
 
 private:
-	void initSelectorsSci3(const SciSpan<const byte> &buf);
+	void initSelectorsSci3(const SciSpan<const byte> &buf, const bool initVariables);
 
 	SciSpan<const byte> _baseObj; /**< base + object offset within base */
-	SciSpan<const uint16> _baseVars; /**< Pointer to the varselector area for this object */
-	Common::Array<uint16> _baseMethod; /**< Pointer to the method selector area for this object */
-	uint32 *_propertyOffsetsSci3; /**< This is used to enable relocation of property values in SCI3 */
+	Common::Array<uint16> _baseVars; /**< The varselector area for this object */
+	Common::Array<uint16> _baseMethod; /**< The method selector area for this object */
+	Common::Array<uint32> _propertyOffsetsSci3; /**< Enables relocation of property values in SCI3 */
 
 	Common::Array<reg_t> _variables;
 	uint16 _methodCount;
diff --git a/engines/sci/engine/savegame.cpp b/engines/sci/engine/savegame.cpp
index 4c16770fc5..a82499adc4 100644
--- a/engines/sci/engine/savegame.cpp
+++ b/engines/sci/engine/savegame.cpp
@@ -103,6 +103,10 @@ void syncWithSerializer(Common::Serializer &s, Node &obj) {
 	syncWithSerializer(s, obj.value);
 }
 
+void syncWithSerializer(Common::Serializer &s, bool &obj) {
+	s.syncAsByte(obj);
+}
+
 #pragma mark -
 
 // By default, sync using syncWithSerializer, which in turn can easily be overloaded.
@@ -415,6 +419,12 @@ void Object::saveLoadWithSerializer(Common::Serializer &s) {
 	s.syncAsSint32LE(_methodCount);		// that's actually a uint16
 
 	syncArray<reg_t>(s, _variables);
+	if (s.getVersion() >= 42 && getSciVersion() == SCI_VERSION_3) {
+		syncArray<bool>(s, _mustSetViewVisible);
+		syncWithSerializer(s, _superClassPosSci3);
+		syncWithSerializer(s, _speciesSelectorSci3);
+		syncWithSerializer(s, _infoSelectorSci3);
+	}
 }
 
 
diff --git a/engines/sci/engine/savegame.h b/engines/sci/engine/savegame.h
index b6a673b1c0..53377f0b4d 100644
--- a/engines/sci/engine/savegame.h
+++ b/engines/sci/engine/savegame.h
@@ -37,6 +37,7 @@ struct EngineState;
  *
  * Version - new/changed feature
  * =============================
+ *      42 - initial SCI3 support
  *      41 - palette support for newer SCI2.1 games; stable SCI2/2.1 save games
  *      40 - always store palvary variables
  *      39 - Accurate SCI32 arrays/strings, score metadata, avatar metadata
@@ -66,7 +67,7 @@ struct EngineState;
  */
 
 enum {
-	CURRENT_SAVEGAME_VERSION = 41,
+	CURRENT_SAVEGAME_VERSION = 42,
 	MINIMUM_SAVEGAME_VERSION = 14
 #ifdef ENABLE_SCI32
 	,