SCUMM: Get rid of the MemBlkHeader hack

This uncovered at least one potentially serious bug in the inventory
code, which still needs to be investigated and fixed.

10 files changed, 109 insertions, 83 deletions

diff --git a/engines/scumm/he/script_v72he.cpp b/engines/scumm/he/script_v72he.cpp
index b63feeb580..96ffa2af3b 100644
--- a/engines/scumm/he/script_v72he.cpp
+++ b/engines/scumm/he/script_v72he.cpp
@@ -621,7 +621,7 @@ void ScummEngine_v72he::o72_getArrayDimSize() {
 }
 
 void ScummEngine_v72he::o72_getNumFreeArrays() {
-	byte **addr = _res->_types[rtString].address;
+	byte **addr = _res->_types[rtString]._address;
 	int i, num = 0;
 
 	for (i = 1; i < _numArray; i++) {
@@ -629,7 +629,7 @@ void ScummEngine_v72he::o72_getNumFreeArrays() {
 			num++;
 	}
 
-	push (num);
+	push(num);
 }
 
 void ScummEngine_v72he::o72_roomOps() {
diff --git a/engines/scumm/imuse/imuse.cpp b/engines/scumm/imuse/imuse.cpp
index e6b134a7d8..4a7ddd05fc 100644
--- a/engines/scumm/imuse/imuse.cpp
+++ b/engines/scumm/imuse/imuse.cpp
@@ -112,12 +112,12 @@ byte *IMuseInternal::findStartOfSound(int sound) {
 	}
 
 	// Check for old-style headers first, like 'RO'
-	if (ptr[4] == 'R' && ptr[5] == 'O'&& ptr[6] != 'L')
+	if (ptr[0] == 'R' && ptr[1] == 'O'&& ptr[2] != 'L')
+		return ptr;
+	if (ptr[4] == 'S' && ptr[5] == 'O')
 		return ptr + 4;
-	if (ptr[8] == 'S' && ptr[9] == 'O')
-		return ptr + 8;
 
-	ptr += 8;
+	ptr += 4;
 	size = READ_BE_UINT32(ptr);
 	ptr += 4;
 
@@ -145,7 +145,7 @@ bool IMuseInternal::isMT32(int sound) {
 	if (ptr == NULL)
 		return false;
 
-	tag = READ_BE_UINT32(ptr + 4);
+	tag = READ_BE_UINT32(ptr);
 	switch (tag) {
 	case MKTAG('A','D','L',' '):
 	case MKTAG('A','S','F','X'): // Special AD class for old AdLib sound effects
@@ -164,17 +164,17 @@ bool IMuseInternal::isMT32(int sound) {
 
 	case MKTAG('M','I','D','I'):	// Occurs in Sam & Max
 		// HE games use Roland music
-		if (ptr[12] == 'H' && ptr[13] == 'S')
+		if (ptr[8] == 'H' && ptr[9] == 'S')
 			return true;
 		else
 			return false;
 	}
 
 	// Old style 'RO' has equivalent properties to 'ROL'
-	if (ptr[4] == 'R' && ptr[5] == 'O')
+	if (ptr[0] == 'R' && ptr[1] == 'O')
 		return true;
 	// Euphony tracks show as 'SO' and have equivalent properties to 'ADL'
-	if (ptr[8] == 'S' && ptr[9] == 'O')
+	if (ptr[4] == 'S' && ptr[5] == 'O')
 		return false;
 
 	error("Unknown music type: '%c%c%c%c'", (char)tag >> 24, (char)tag >> 16, (char)tag >> 8, (char)tag);
@@ -192,7 +192,7 @@ bool IMuseInternal::isMIDI(int sound) {
 	if (ptr == NULL)
 		return false;
 
-	tag = READ_BE_UINT32(ptr + 4);
+	tag = READ_BE_UINT32(ptr);
 	switch (tag) {
 	case MKTAG('A','D','L',' '):
 	case MKTAG('A','S','F','X'): // Special AD class for old AdLib sound effects
@@ -212,11 +212,11 @@ bool IMuseInternal::isMIDI(int sound) {
 	}
 
 	// Old style 'RO' has equivalent properties to 'ROL'
-	if (ptr[4] == 'R' && ptr[5] == 'O')
+	if (ptr[0] == 'R' && ptr[1] == 'O')
 		return true;
 	// Euphony tracks show as 'SO' and have equivalent properties to 'ADL'
 	// FIXME: Right now we're pretending it's GM.
-	if (ptr[8] == 'S' && ptr[9] == 'O')
+	if (ptr[4] == 'S' && ptr[5] == 'O')
 		return true;
 
 	error("Unknown music type: '%c%c%c%c'", (char)tag >> 24, (char)tag >> 16, (char)tag >> 8, (char)tag);
diff --git a/engines/scumm/object.cpp b/engines/scumm/object.cpp
index 457e2c717c..8b0e22d33e 100644
--- a/engines/scumm/object.cpp
+++ b/engines/scumm/object.cpp
@@ -192,8 +192,11 @@ void ScummEngine::clearOwnerOf(int obj) {
 					if (!_inventory[i] && _inventory[i+1]) {
 						_inventory[i] = _inventory[i+1];
 						_inventory[i+1] = 0;
-						_res->_types[rtInventory].address[i] = _res->_types[rtInventory].address[i + 1];
-						_res->_types[rtInventory].address[i + 1] = NULL;
+						// FIXME FIXME FIXME: This is incomplete, as we do not touch flags, status... BUG
+						_res->_types[rtInventory]._address[i] = _res->_types[rtInventory]._address[i + 1];
+						_res->_types[rtInventory]._size[i] = _res->_types[rtInventory]._size[i + 1];
+						_res->_types[rtInventory]._address[i + 1] = NULL;
+						_res->_types[rtInventory]._size[i + 1] = 0;
 					}
 				}
 				break;
@@ -1796,7 +1799,7 @@ int ScummEngine::findLocalObjectSlot() {
 int ScummEngine::findFlObjectSlot() {
 	int i;
 	for (i = 1; i < _numFlObject; i++) {
-		if (_res->_types[rtFlObject].address[i] == NULL)
+		if (_res->_types[rtFlObject]._address[i] == NULL)
 			return i;
 	}
 	error("findFlObjectSlot: Out of FLObject slots");
diff --git a/engines/scumm/resource.cpp b/engines/scumm/resource.cpp
index a00630f1ec..009fd5c11d 100644
--- a/engines/scumm/resource.cpp
+++ b/engines/scumm/resource.cpp
@@ -537,9 +537,10 @@ void ResourceManager::allocResTypeData(int id, uint32 tag, int num, const char *
 	_types[id].num = num;
 	_types[id].tag = tag;
 	_types[id].name = name;
-	_types[id].address = (byte **)calloc(num, sizeof(void *));
+	_types[id]._address = (byte **)calloc(num, sizeof(byte *));
+	_types[id]._size = (uint32 *)calloc(num, sizeof(uint32));
 	_types[id].flags = (byte *)calloc(num, sizeof(byte));
-	_types[id].status = (byte *)calloc(num, sizeof(byte));
+	_types[id]._status = (byte *)calloc(num, sizeof(byte));
 
 	if (mode) {
 		_types[id].roomno = (byte *)calloc(num, sizeof(byte));
@@ -584,8 +585,6 @@ void ScummEngine::nukeCharset(int i) {
 }
 
 void ScummEngine::ensureResourceLoaded(int type, int i) {
-	void *addr = NULL;
-
 	debugC(DEBUG_RESOURCE, "ensureResourceLoaded(%s,%d)", resTypeFromId(type), i);
 
 	if ((type == rtRoom) && i > 0x7F && _game.version < 7 && _game.heversion <= 71) {
@@ -606,10 +605,7 @@ void ScummEngine::ensureResourceLoaded(int type, int i) {
 	if (type != rtCharset && i == 0)
 		return;
 
-	if (i <= _res->_types[type].num)
-		addr = _res->_types[type].address[i];
-
-	if (addr)
+	if (i <= _res->_types[type].num && _res->_types[type]._address[i])
 		return;
 
 	loadResource(type, i);
@@ -715,9 +711,7 @@ uint32 ScummEngine_v70he::getResourceRoomOffset(int type, int idx) {
 int ScummEngine::getResourceSize(int type, int idx) {
 	byte *ptr = getResourceAddress(type, idx);
 	assert(ptr);
-	MemBlkHeader *hdr = (MemBlkHeader *)(ptr - sizeof(MemBlkHeader));
-
-	return hdr->size;
+	return _res->_types[type]._size[idx];
 }
 
 byte *ScummEngine::getResourceAddress(int type, int idx) {
@@ -729,17 +723,17 @@ byte *ScummEngine::getResourceAddress(int type, int idx) {
 	if (!_res->validateResource("getResourceAddress", type, idx))
 		return NULL;
 
-	if (!_res->_types[type].address) {
-		debugC(DEBUG_RESOURCE, "getResourceAddress(%s,%d), _res->_types[type].address == NULL", resTypeFromId(type), idx);
+	if (!_res->_types[type]._address) {
+		debugC(DEBUG_RESOURCE, "getResourceAddress(%s,%d), _res->_types[type]._address == NULL", resTypeFromId(type), idx);
 		return NULL;
 	}
 
 	// If the resource is missing, but loadable from the game data files, try to do so.
-	if (!_res->_types[type].address[idx] && _res->_types[type]._mode != kDynamicResTypeMode) {
+	if (!_res->_types[type]._address[idx] && _res->_types[type]._mode != kDynamicResTypeMode) {
 		ensureResourceLoaded(type, idx);
 	}
 
-	ptr = (byte *)_res->_types[type].address[idx];
+	ptr = (byte *)_res->_types[type]._address[idx];
 	if (!ptr) {
 		debugC(DEBUG_RESOURCE, "getResourceAddress(%s,%d) == NULL", resTypeFromId(type), idx);
 		return NULL;
@@ -747,8 +741,8 @@ byte *ScummEngine::getResourceAddress(int type, int idx) {
 
 	_res->setResourceCounter(type, idx, 1);
 
-	debugC(DEBUG_RESOURCE, "getResourceAddress(%s,%d) == %p", resTypeFromId(type), idx, ptr + sizeof(MemBlkHeader));
-	return ptr + sizeof(MemBlkHeader);
+	debugC(DEBUG_RESOURCE, "getResourceAddress(%s,%d) == %p", resTypeFromId(type), idx, ptr);
+	return ptr;
 }
 
 byte *ScummEngine::getStringAddress(int i) {
@@ -807,29 +801,29 @@ byte *ResourceManager::createResource(int type, int idx, uint32 size) {
 		// cases. For instance, Zak tries to reload the intro music
 		// while it's playing. See bug #1253171.
 
-		if (_types[type].address[idx] && (type == rtSound || type == rtScript || type == rtCostume))
-			return _types[type].address[idx] + sizeof(MemBlkHeader);
+		if (_types[type]._address[idx] && (type == rtSound || type == rtScript || type == rtCostume))
+			return _types[type]._address[idx];
 	}
 
 	nukeResource(type, idx);
 
 	expireResources(size);
 
-	void *ptr = calloc(size + sizeof(MemBlkHeader) + SAFETY_AREA, 1);
+	byte *ptr = (byte *)calloc(size + SAFETY_AREA, 1);
 	if (ptr == NULL) {
 		error("createResource(%s,%d): Out of memory while allocating %d", resTypeFromId(type), idx, size);
 	}
 
 	_allocatedSize += size;
 
-	_types[type].address[idx] = (byte *)ptr;
-	((MemBlkHeader *)ptr)->size = size;
+	_types[type]._address[idx] = ptr;
+	_types[type]._size[idx] = size;
 	setResourceCounter(type, idx, 1);
-	return (byte *)ptr + sizeof(MemBlkHeader);	/* skip header */
+	return ptr;
 }
 
 ResourceManager::ResTypeData::ResTypeData() {
-	memset(this, 0, sizeof(this));
+	memset(this, 0, sizeof(*this));
 }
 
 ResourceManager::ResTypeData::~ResTypeData() {
@@ -864,18 +858,19 @@ bool ResourceManager::validateResource(const char *str, int type, int idx) const
 void ResourceManager::nukeResource(int type, int idx) {
 	byte *ptr;
 
-	if (!_types[type].address)
+	if (!_types[type]._address)
 		return;
 
 	assert(idx >= 0 && idx < _types[type].num);
 
-	ptr = _types[type].address[idx];
+	ptr = _types[type]._address[idx];
 	if (ptr != NULL) {
 		debugC(DEBUG_RESOURCE, "nukeResource(%s,%d)", resTypeFromId(type), idx);
-		_types[type].address[idx] = 0;
+		_types[type]._address[idx] = 0;
+		_types[type]._size[idx] = 0;
 		_types[type].flags[idx] = 0;
-		_types[type].status[idx] &= ~RS_MODIFIED;
-		_allocatedSize -= ((MemBlkHeader *)ptr)->size;
+		_types[type]._status[idx] &= ~RS_MODIFIED;
+		_allocatedSize -= _types[type]._size[idx];
 		free(ptr);
 	}
 }
@@ -957,13 +952,13 @@ bool ScummEngine::isResourceInUse(int type, int i) const {
 void ResourceManager::setModified(int type, int i) {
 	if (!validateResource("Modified", type, i))
 		return;
-	_types[type].status[i] |= RS_MODIFIED;
+	_types[type]._status[i] |= RS_MODIFIED;
 }
 
 bool ResourceManager::isModified(int type, int i) const {
 	if (!validateResource("isModified", type, i))
 		return false;
-	return (_types[type].status[i] & RS_MODIFIED) != 0;
+	return (_types[type]._status[i] & RS_MODIFIED) != 0;
 }
 
 void ResourceManager::expireResources(uint32 size) {
@@ -993,7 +988,7 @@ void ResourceManager::expireResources(uint32 size) {
 				// so we can potentially unload them to free memory.
 				for (j = _types[i].num; --j >= 0;) {
 					flag = _types[i].flags[j];
-					if (!(flag & RF_LOCK) && flag >= best_counter && _types[i].address[j] && !_vm->isResourceInUse(i, j)) {
+					if (!(flag & RF_LOCK) && flag >= best_counter && _types[i]._address[j] && !_vm->isResourceInUse(i, j)) {
 						best_counter = flag;
 						best_type = i;
 						best_res = j;
@@ -1018,9 +1013,10 @@ void ResourceManager::freeResources() {
 			if (isResourceLoaded(i, j))
 				nukeResource(i, j);
 		}
-		free(_types[i].address);
+		free(_types[i]._address);
+		free(_types[i]._size);
 		free(_types[i].flags);
-		free(_types[i].status);
+		free(_types[i]._status);
 		free(_types[i].roomno);
 		free(_types[i].roomoffs);
 
@@ -1055,7 +1051,7 @@ void ScummEngine::loadPtrToResource(int type, int resindex, const byte *source)
 bool ResourceManager::isResourceLoaded(int type, int idx) const {
 	if (!validateResource("isResourceLoaded", type, idx))
 		return false;
-	return _types[type].address[idx] != NULL;
+	return _types[type]._address[idx] != NULL;
 }
 
 void ResourceManager::resourceStats() {
@@ -1066,8 +1062,8 @@ void ResourceManager::resourceStats() {
 	for (i = rtFirst; i <= rtLast; i++)
 		for (j = _types[i].num; --j >= 0;) {
 			flag = _types[i].flags[j];
-			if (flag & RF_LOCK && _types[i].address[j]) {
-				lockedSize += ((MemBlkHeader *)_types[i].roomoffs[j])->size;
+			if (flag & RF_LOCK && _types[i]._address[j]) {
+				lockedSize += _types[i]._size[j];
 				lockedNum++;
 			}
 		}
diff --git a/engines/scumm/resource.h b/engines/scumm/resource.h
index f1bcc14cb6..dd314cb0c8 100644
--- a/engines/scumm/resource.h
+++ b/engines/scumm/resource.h
@@ -88,14 +88,50 @@ public:
 		uint16 num;
 		uint32 tag;
 		const char *name;
-		byte **address;
+
+		/**
+		 * Array of size num containing pointers to each resource of this type.
+		 */
+		byte **_address;
+
+		/**
+		 * Array of size num containing the sizes of each resource of this type.
+		 */
+		uint32 *_size;
 	protected:
+		/**
+		 * Array of size num containing TODO of each resource of this type.
+		 */
 		byte *flags;
-		byte *status;
+
+		/**
+		 * Array of size num containing the status of each resource of this type.
+		 * This is a bitfield of which currently only one bit is used, which indicates
+		 * whether the resource is modified.
+		 */
+		byte *_status;
 	public:
+		/**
+		 * Array of size num containing for each resource of this type the
+		 * id of the room (resp. the disk) the resource is contained in.
+		 */
 		byte *roomno;
+
+		/**
+		 * Array of size num containing room offsets of each resource of this type.
+		 * That is the offset (in bytes) where the data for this resources
+		 * can be found in the game data file(s), relative to the start
+		 * of the room the resource is contained in.
+		 *
+		 * A value of RES_INVALID_OFFSET indicates a resources that is not contained
+		 * in the game data files.
+		 */
 		uint32 *roomoffs;
-		uint32 *globsize;	///!< Occurs in HE 70+, but we don't use it for anything.
+
+		/**
+		 * Array of size num. Occurs in HE 70+, but we don't use it for anything.
+		 */
+		uint32 *globsize;
 
 	public:
 		ResTypeData();
diff --git a/engines/scumm/saveload.cpp b/engines/scumm/saveload.cpp
index cd48feee54..0ca16482ce 100644
--- a/engines/scumm/saveload.cpp
+++ b/engines/scumm/saveload.cpp
@@ -1247,7 +1247,7 @@ void ScummEngine::saveOrLoad(Serializer *s) {
 					s->saveUint16(type);	// Save the res type...
 					for (idx = 0; idx < _res->_types[type].num; idx++) {
 						// Only save resources which actually exist...
-						if (_res->_types[type].address[idx]) {
+						if (_res->_types[type]._address[idx]) {
 							s->saveUint16(idx);	// Save the index of the resource
 							saveResource(s, type, idx);
 						}
@@ -1663,14 +1663,14 @@ void ScummEngine::loadResourceOLD(Serializer *ser, int type, int idx) {
 }
 
 void ScummEngine::saveResource(Serializer *ser, int type, int idx) {
-	assert(_res->_types[type].address[idx]);
+	assert(_res->_types[type]._address[idx]);
 
 	if (_res->_types[type]._mode == kDynamicResTypeMode) {
-		byte *ptr = _res->_types[type].address[idx];
-		uint32 size = ((MemBlkHeader *)ptr)->size;
+		byte *ptr = _res->_types[type]._address[idx];
+		uint32 size = _res->_types[type]._size[idx];
 
 		ser->saveUint32(size);
-		ser->saveBytes(ptr + sizeof(MemBlkHeader), size);
+		ser->saveBytes(ptr, size);
 
 		if (type == rtInventory) {
 			ser->saveUint16(_inventory[idx]);
diff --git a/engines/scumm/script.cpp b/engines/scumm/script.cpp
index 4630ec1a5c..7f279a3f0c 100644
--- a/engines/scumm/script.cpp
+++ b/engines/scumm/script.cpp
@@ -390,7 +390,7 @@ void ScummEngine::getScriptBaseAddress() {
 				break;
 		_scriptOrgPointer = getResourceAddress(rtInventory, idx);
 		assert(idx < _numInventory);
-		_lastCodePtr = &_res->_types[rtInventory].address[idx];
+		_lastCodePtr = &_res->_types[rtInventory]._address[idx];
 		break;
 
 	case WIO_LOCAL:
@@ -398,18 +398,18 @@ void ScummEngine::getScriptBaseAddress() {
 		if (_game.version == 8) {
 			_scriptOrgPointer = getResourceAddress(rtRoomScripts, _roomResource);
 			assert(_roomResource < _res->_types[rtRoomScripts].num);
-			_lastCodePtr = &_res->_types[rtRoomScripts].address[_roomResource];
+			_lastCodePtr = &_res->_types[rtRoomScripts]._address[_roomResource];
 		} else {
 			_scriptOrgPointer = getResourceAddress(rtRoom, _roomResource);
 			assert(_roomResource < _numRooms);
-			_lastCodePtr = &_res->_types[rtRoom].address[_roomResource];
+			_lastCodePtr = &_res->_types[rtRoom]._address[_roomResource];
 		}
 		break;
 
 	case WIO_GLOBAL:							/* global script */
 		_scriptOrgPointer = getResourceAddress(rtScript, ss->number);
 		assert(ss->number < _numScripts);
-		_lastCodePtr = &_res->_types[rtScript].address[ss->number];
+		_lastCodePtr = &_res->_types[rtScript]._address[ss->number];
 		break;
 
 	case WIO_FLOBJECT:						/* flobject script */
@@ -418,7 +418,7 @@ void ScummEngine::getScriptBaseAddress() {
 		idx = _objs[idx].fl_object_index;
 		_scriptOrgPointer = getResourceAddress(rtFlObject, idx);
 		assert(idx < _numFlObject);
-		_lastCodePtr = &_res->_types[rtFlObject].address[idx];
+		_lastCodePtr = &_res->_types[rtFlObject]._address[idx];
 		break;
 	default:
 		error("Bad type while getting base address");
@@ -445,7 +445,7 @@ void ScummEngine::resetScriptPointer() {
  * collected by ResourceManager::expireResources.
  */
 void ScummEngine::refreshScriptPointer() {
-	if (*_lastCodePtr + sizeof(MemBlkHeader) != _scriptOrgPointer) {
+	if (*_lastCodePtr != _scriptOrgPointer) {
 		long oldoffs = _scriptPointer - _scriptOrgPointer;
 		getScriptBaseAddress();
 		_scriptPointer = _scriptOrgPointer + oldoffs;
diff --git a/engines/scumm/script_v6.cpp b/engines/scumm/script_v6.cpp
index afc1eb6909..138da3f921 100644
--- a/engines/scumm/script_v6.cpp
+++ b/engines/scumm/script_v6.cpp
@@ -357,7 +357,7 @@ void ScummEngine_v6::nukeArray(int a) {
 }
 
 int ScummEngine_v6::findFreeArrayId() {
-	byte **addr = _res->_types[rtString].address;
+	byte **addr = _res->_types[rtString]._address;
 	int i;
 
 	for (i = 1; i < _numArray; i++) {
diff --git a/engines/scumm/scumm.cpp b/engines/scumm/scumm.cpp
index e89100688a..22736973f7 100644
--- a/engines/scumm/scumm.cpp
+++ b/engines/scumm/scumm.cpp
@@ -1185,7 +1185,7 @@ Common::Error ScummEngine::init() {
 	resetScummVars();
 
 	if (_imuse) {
-		_imuse->setBase(_res->_types[rtSound].address);
+		_imuse->setBase(_res->_types[rtSound]._address);
 	}
 
 	if (_game.version >= 5 && _game.version <= 7)
@@ -2462,7 +2462,7 @@ void ScummEngine::restart() {
 	resetScummVars();
 
 	if (_imuse) {
-		_imuse->setBase(_res->_types[rtSound].address);
+		_imuse->setBase(_res->_types[rtSound]._address);
 	}
 
 	// Reinit sound engine
diff --git a/engines/scumm/scumm.h b/engines/scumm/scumm.h
index 768c976388..9d64202ce8 100644
--- a/engines/scumm/scumm.h
+++ b/engines/scumm/scumm.h
@@ -168,17 +168,6 @@ enum {
 	DEBUG_SMUSH	=	1 << 10		// Track SMUSH
 };
 
-/**
- * Internal header for any memory block allocated by the resource manager.
- *
- * @todo Hide MemBlkHeader; no code outside the resource manager should
- * have to use it, ever. Currently script code needs it to detect whether
- * some scripts have moved (in fetchScriptByte()).
- */
-struct MemBlkHeader {
-	uint32 size;
-};
-
 struct VerbSlot;
 struct ObjectData;
 
@@ -622,9 +611,11 @@ protected:
 protected:
 	/* Script VM - should be in Script class */
 	uint32 _localScriptOffsets[1024];
-	const byte *_scriptPointer, *_scriptOrgPointer;
-	byte _opcode, _currentScript;
+	const byte *_scriptPointer;
+	const byte *_scriptOrgPointer;
 	const byte * const *_lastCodePtr;
+	byte _opcode;
+	byte _currentScript;
 	int _scummStackPos;
 	int _vmStack[150];