Added experimental (i.e. not tested) resource dumping code.

Added tentative workaround for the bug (a script bug, I think) that causes
the game to hang when examining the lift at the top of the pyramid.

And, of course, some misc. cleanup.

svn-id: r11359

8 files changed, 218 insertions, 215 deletions

diff --git a/sword2/header.h b/sword2/header.h
index 9c6f3b4f78..95f106254d 100644
--- a/sword2/header.h
+++ b/sword2/header.h
@@ -50,46 +50,47 @@ struct _standardHeader {
 	uint8 name[NAME_LEN];		// Name of object
 } GCC_PACK;
 
-//----------------------------------------------------------
 // fileType
 
-// 0 something's wrong!
-
-#define ANIMATION_FILE		1	// All normal animations & sprites
+enum {
+	// 0 something's wrong!
+	ANIMATION_FILE		= 1,	// All normal animations & sprites
 					// including mega-sets & font files
 					// which are the same format (but all
 					// frames always uncompressed)
-#define	SCREEN_FILE		2	// Each contains background, palette,
+	SCREEN_FILE		= 2,	// Each contains background, palette,
 					// layer sprites, parallax layers &
 					// shading mask
-#define	GAME_OBJECT		3	// Each contains object hub +
+	GAME_OBJECT		= 3,	// Each contains object hub +
 					// structures + script data
-#define	WALK_GRID_FILE		4	// Walk-grid data
-#define	GLOBAL_VAR_FILE		5	// All the global script variables in
+	WALK_GRID_FILE		= 4,	// Walk-grid data
+	GLOBAL_VAR_FILE		= 5,	// All the global script variables in
 					// one file; "there can be only one"
-#define PARALLAX_FILE_null	6	// NOT USED
-#define	RUN_LIST		7	// Each contains a list of object
+	PARALLAX_FILE_null	= 6,	// NOT USED
+	RUN_LIST		= 7,	// Each contains a list of object
 					// resource id's
-#define	TEXT_FILE		8	// Each contains all the lines of text
+	TEXT_FILE		= 8,	// Each contains all the lines of text
 					// for a location or a character's
 					// conversation script
-#define	SCREEN_MANAGER		9	// One for each location; this contains
+	SCREEN_MANAGER		= 9,	// One for each location; this contains
 					// special startup scripts
-#define MOUSE_FILE		10	// Mouse pointers and luggage icons
+	MOUSE_FILE		= 10,	// Mouse pointers and luggage icons
 					// (sprites in General / Mouse pointers
 					// & Luggage icons)
-#define WAV_FILE		11	// WAV file
-#define	ICON_FILE		12	// Menu icon (sprites in General \ Menu
-					// icons
-#define PALETTE_FILE		13	// separate palette file (see also
+	WAV_FILE		= 11,	// WAV file
+	ICON_FILE		= 12,	// Menu icon (sprites in General / Menu
+					// icons)
+	PALETTE_FILE		= 13	// separate palette file (see also
 					// _paletteHeader)
+};
 
-//----------------------------------------------------------
 // compType
 
-#define NO_COMPRESSION		0
-#define FILE_COMPRESSION	1	// standard whole-file compression
+enum {
+	NO_COMPRESSION		= 0,
+	FILE_COMPRESSION	= 1	// standard whole-file compression
 					// (not yet devised!)
+};
 
 //----------------------------------------------------------
 // (1) ANIMATION FILES
@@ -103,7 +104,6 @@ struct _standardHeader {
 // a 16-byte colour table ONLY if (runTimeComp==RLE16)
 // a string of groups of (frame header + frame data)
 
-//----------------------------------------------------------
 // Animation Header
 
 struct _animHeader {
@@ -122,17 +122,17 @@ struct _animHeader {
 	uint16 blend;
 } GCC_PACK;
 
-//----------------------------------------------------------
 // runtimeComp - compression used on each frame of the anim
 
-#define NONE	0		// No frame compression
-#define RLE256	1		// James's RLE for 256-colour sprites
-#define RLE16	2		// James's RLE for 16- or 17-colour sprites
+enum {
+	NONE	= 0,		// No frame compression
+	RLE256	= 1,		// James's RLE for 256-colour sprites
+	RLE16	= 2		// James's RLE for 16- or 17-colour sprites
 				// (raw blocks have max 16 colours for 2 pixels
 				// per byte, so '0's are encoded only as FLAT
 				// for 17-colour sprites eg. George's mega-set)
+};
 
-//----------------------------------------------------------
 // CDT Entry
 
 struct _cdtEntry {
@@ -147,14 +147,16 @@ struct _cdtEntry {
 } GCC_PACK;
 
 // 'frameType' bit values
-#define FRAME_OFFSET	1	// Print at (feetX + x, feetY + y), with
+
+enum {
+	FRAME_OFFSET	= 1,	// Print at (feetX + x, feetY + y), with
 				// scaling according to feetY
-#define FRAME_FLIPPED	2	// Print the frame flipped Left->Right
-#define FRAME_256_FAST	4	// Frame has been compressed using Pauls fast
+	FRAME_FLIPPED	= 2,	// Print the frame flipped Left->Right
+	FRAME_256_FAST	= 4	// Frame has been compressed using Pauls fast
 				// RLE 256 compression.
+};
 
-//----------------------------------------------------------
-//Frame Header
+// Frame Header
 
 struct _frameHeader {
 	uint32 compSize;	// Compressed size of frame - NB. compression
@@ -178,7 +180,6 @@ struct _frameHeader {
 // a string of layer headers
 // a string of layer masks
 
-//----------------------------------------------------------
 // Multi screen header
 // Goes at the beginning of a screen file after the standard header.
 // Gives offsets from start of table of each of the components
@@ -193,7 +194,6 @@ struct _multiScreenHeader {
 	uint32 maskOffset;
 } GCC_PACK;
 
-//------------------------------------------------------------
 // Palette Data
 
 struct _palEntry {
@@ -203,7 +203,6 @@ struct _palEntry {
 	uint8 alpha;
 } GCC_PACK;
 
-//------------------------------------------------------------
 // Screen Header
 
 struct _screenHeader {
@@ -212,12 +211,10 @@ struct _screenHeader {
 	uint16 noLayers;	// number of layer areas
 } GCC_PACK;
 
-//------------------------------------------------------------
 // Layer Header
 
-// Note that all the layer headers are kept together,
-// rather than being placed before each layer mask,
-// in order to simplify the sort routine.
+// Note that all the layer headers are kept together, rather than being placed
+// before each layer mask, in order to simplify the sort routine.
 
 struct _layerHeader {
 	uint16 x;		// coordinates of top-left pixel of area
@@ -247,7 +244,6 @@ struct _layerHeader {
 // walk-grid file header
 // walk-grid data
 
-//----------------------------------------------------------
 // Walk-Grid Header - taken directly from old "header.h" in STD_INC
 
 struct _walkGridHeader {
@@ -264,8 +260,6 @@ struct _walkGridHeader {
 // 4 * 256 bytes of palette data
 // 256k palette match table
 
-//----------------------------------------------------------
-
 // an object hub - which represents all that remains of the compact concept
 
 #define	TREE_SIZE	3
@@ -293,8 +287,6 @@ struct _textHeader {
 //	line of text,0
 //	line of text,0
 
-// ----------------------------------------------------------
-
 #if !defined(__GNUC__)
 	#pragma END_PACK_STRUCTS
 #endif
diff --git a/sword2/interpreter.cpp b/sword2/interpreter.cpp
index d8550c9fe8..f6fde1b095 100644
--- a/sword2/interpreter.cpp
+++ b/sword2/interpreter.cpp
@@ -18,6 +18,7 @@
  */
 
 #include "common/stdafx.h"
+#include "common/util.h"
 #include "sword2/sword2.h"
 #include "sword2/interpreter.h"
 
@@ -193,6 +194,9 @@ void Logic::setupOpcodes(void) {
 
 int32 Logic::executeOpcode(int i, int32 *params) {
 	OpcodeProc op = _opcodes[i].proc;
+
+	debug(5, "%s", _opcodes[i].desc);
+
 	return (this->*op) (params);
 }
 
@@ -315,8 +319,6 @@ int Logic::runScript(char *scriptData, char *objectData, uint32 *offset) {
 			assert(parameter <= MAX_FN_NUMBER);
 			// amount to adjust stack by (no of parameters)
 			Read8ip(value);
-			debug(5, "Call mcode %d with stack = %x", parameter, stack2 + stackPointer2 - value);
-
 			retVal = executeOpcode(parameter, stack2 + stackPointer2 - value);
 
 			stackPointer2 -= value;
diff --git a/sword2/logic.cpp b/sword2/logic.cpp
index 3cf0818721..5a85ad7698 100644
--- a/sword2/logic.cpp
+++ b/sword2/logic.cpp
@@ -53,7 +53,7 @@ int Logic::processSession(void) {
 	// processing on the current list
 
 	while (_pc != 0xffffffff) {
-		head = (_standardHeader*) _vm->_resman->openResource(run_list);
+		head = (_standardHeader *) _vm->_resman->openResource(run_list);
 
 		if (head->fileType != RUN_LIST)
 			error("Logic_engine %d not a run_list", run_list);
@@ -77,7 +77,7 @@ int Logic::processSession(void) {
 			return 0;
 		}
 
-		head = (_standardHeader*) _vm->_resman->openResource(ID);
+		head = (_standardHeader *) _vm->_resman->openResource(ID);
 
 		if (head->fileType != GAME_OBJECT)
 			error("Logic_engine %d not an object", ID);
@@ -111,7 +111,7 @@ int Logic::processSession(void) {
 				// this is the script data
 				// raw_script_ad = (char *) (_curObjectHub + 1);
 
-				raw_script_ad = (char*) head;
+				raw_script_ad = (char *) head;
 
 				// script and data object are us/same
 				ret = runScript(raw_script_ad, raw_script_ad, &_curObjectHub->script_pc[LEVEL]);
@@ -121,19 +121,19 @@ int Logic::processSession(void) {
 
 				// get the foreign objects script data address
 
-				raw_data_ad = (char*) head;
+				raw_data_ad = (char *) head;
 
 				far_head = (_standardHeader*) _vm->_resman->openResource(script / SIZE);
 
 				if (far_head->fileType != GAME_OBJECT && far_head->fileType != SCREEN_MANAGER)
 					error("Logic_engine %d not a far object (its a %d)", script / SIZE, far_head->fileType);
 
-				// raw_script_ad = (char*) (head + 1) + sizeof(_standardHeader);
+				// raw_script_ad = (char *) (head + 1) + sizeof(_standardHeader);
 
 				// get our objects data address
-				// raw_data_ad = (char*) (_curObjectHub + 1);
+				// raw_data_ad = (char *) (_curObjectHub + 1);
 
-				raw_script_ad = (char*) far_head;
+				raw_script_ad = (char *) far_head;
 
 				ret = runScript(raw_script_ad, raw_data_ad, &_curObjectHub->script_pc[LEVEL]);
 
diff --git a/sword2/mouse.cpp b/sword2/mouse.cpp
index fc4f9ad98d..5cb6d784dd 100644
--- a/sword2/mouse.cpp
+++ b/sword2/mouse.cpp
@@ -331,7 +331,7 @@ void Sword2Engine::dragMouse(void) {
 
 		_logic->setPlayerActionEvent(CUR_PLAYER_ID, _mouseTouching);
 
-		debug(5, "Used \"%s\" on \"%s\"", fetchObjectName(OBJECT_HELD), fetchObjectName(CLICKED_ID));
+		debug(2, "Used \"%s\" on \"%s\"", fetchObjectName(OBJECT_HELD), fetchObjectName(CLICKED_ID));
 
 		// Hide menu - back to normal menu mode
 
@@ -647,7 +647,26 @@ void Sword2Engine::normalMouse(void) {
 		EXIT_CLICK_ID = 0;
 		EXIT_FADING = 0;
 
-		_logic->setPlayerActionEvent(CUR_PLAYER_ID, _mouseTouching);
+		// WORKAROUND: Examining the lift while at the top of the
+		// pyramid causes the game to hang. It looks like a script
+		// bug to me: the script hides the mouse cursor, checks if the
+		// player pressed the left mouse button and, if not, jumps to
+		// an end of script instruction.
+		//
+		// One idea would be to redirect the action to the elevator
+		// object at the bottom of the pyramid instead, but I don't
+		// know if that's a safe thing to do so for now I've disabled
+		// it. Maybe we could find a better workaround if we had a
+		// script decompiler...
+		//
+		// I'm checking the status of the left button rather than the
+		// right button because that's what I think the script does.
+
+		if (_mouseTouching == 2773 && !LEFT_BUTTON) {
+			warning("Ignoring action to work around script bug at pyramid top");
+			// _logic->setPlayerActionEvent(CUR_PLAYER_ID, 2737);
+		} else
+			_logic->setPlayerActionEvent(CUR_PLAYER_ID, _mouseTouching);
 
 		if (OBJECT_HELD)
 			debug(2, "Used \"%s\" on \"%s\"", fetchObjectName(OBJECT_HELD), fetchObjectName(CLICKED_ID));
diff --git a/sword2/resman.cpp b/sword2/resman.cpp
index 5fa8c9f9de..3b7a05f383 100644
--- a/sword2/resman.cpp
+++ b/sword2/resman.cpp
@@ -27,29 +27,16 @@
 namespace Sword2 {
 
 // ---------------------------------------------------------------------------
-// welcome to the easy resource manager - written in simple code for easy
+// Welcome to the easy resource manager - written in simple code for easy
 // maintenance
 // 
-// the resource compiler will create two files
+// The resource compiler will create two files
 //
 //	resource.inf which is a list of ascii cluster file names
 //	resource.tab which is a table which tells us which cluster a resource
 //      is located in and the number within the cluster
 // ---------------------------------------------------------------------------
 
-#define NONE	 0
-#define FETCHING 1
-
-#define BUFFERSIZE	4096
-
-// ---------------------------------------------------------------------------
-//
-//
-//	resman.
-//
-//
-// ---------------------------------------------------------------------------
-
 enum {
 	BOTH		= 0x0,		// Cluster is on both CDs
 	CD1		= 0x1,		// Cluster is on CD1 only
@@ -214,7 +201,7 @@ ResourceManager::~ResourceManager(void) {
 
 void convertEndian(uint8 *file, uint32 len) {
 	int i;
-	_standardHeader *hdr = (_standardHeader *)file;
+	_standardHeader *hdr = (_standardHeader *) file;
 	
 	file += sizeof(_standardHeader);
 
@@ -223,7 +210,7 @@ void convertEndian(uint8 *file, uint32 len) {
 
 	switch (hdr->fileType) {
 	case ANIMATION_FILE: {
-		_animHeader *animHead = (_animHeader *)file;
+		_animHeader *animHead = (_animHeader *) file;
 
 		SWAP16(animHead->noAnimFrames);
 		SWAP16(animHead->feetStartX);
@@ -360,7 +347,7 @@ void convertEndian(uint8 *file, uint32 len) {
 		}
 
 		uint16 *node = (uint16 *) (file + sizeof(_walkGridHeader) + walkGridHeader->numBars * sizeof(_barData));
-		for (i = 0; i < walkGridHeader->numNodes*2; i++) {
+		for (i = 0; i < walkGridHeader->numNodes * 2; i++) {
 			SWAP16(*node);
 			node++;
 		}
@@ -372,7 +359,7 @@ void convertEndian(uint8 *file, uint32 len) {
 	case PARALLAX_FILE_null:
 		break;
 	case RUN_LIST: {
-		uint32 *list = (uint32 *)file;
+		uint32 *list = (uint32 *) file;
 		while (*list) {
 			SWAP32(*list);
 			list++;
@@ -380,7 +367,7 @@ void convertEndian(uint8 *file, uint32 len) {
 		break;
 	}
 	case TEXT_FILE: {
-		_textHeader *textHeader = (_textHeader *)file;
+		_textHeader *textHeader = (_textHeader *) file;
 		SWAP32(textHeader->noOfLines);
 		break;
 	}
@@ -393,7 +380,7 @@ void convertEndian(uint8 *file, uint32 len) {
 	}
 }
 
-uint8 *ResourceManager::openResource(uint32 res) {
+uint8 *ResourceManager::openResource(uint32 res, bool dump) {
 	// returns ad of resource. Loads if not in memory
 	// retains a count
 	// resource can be aged out of memory if count = 0
@@ -407,10 +394,7 @@ uint8 *ResourceManager::openResource(uint32 res) {
 
 	uint32	table_offset;
 
-//#ifdef _SWORD2_DEBUG
-	if (res >= _totalResFiles)
-		error("open illegal resource %d (there are %d resources 0-%d)", res, _totalResFiles, _totalResFiles - 1);
-//#endif
+	assert(res < _totalResFiles);
 
 	// is the resource in memory already?
 	// if the file is not in memory then age should and MUST be 0
@@ -422,10 +406,7 @@ uint8 *ResourceManager::openResource(uint32 res) {
 		// points to the number of the ascii filename
 		parent_res_file = _resConvTable[res * 2];
 
-//#ifdef _SWORD2_DEBUG
-		if (parent_res_file == 0xffff)
-			error("open tried to open null & void resource number %d", res);
-//#endif
+		assert(parent_res_file != 0xffff);
 
 		// relative resource within the file
 		actual_res = _resConvTable[(res * 2) + 1];
@@ -438,9 +419,8 @@ uint8 *ResourceManager::openResource(uint32 res) {
 		// of the CDs, remember which one so that we can play the
 		// correct music.
 
-		if (!(_cdTab[parent_res_file] & LOCAL_PERM)) {
+		if (!(_cdTab[parent_res_file] & LOCAL_PERM))
 			_curCd = _cdTab[parent_res_file] & 3;
-		}
 
 		// Actually, as long as the file can be found we don't really
 		// care which CD it's on. But if we can't find it, keep asking
@@ -482,6 +462,69 @@ uint8 *ResourceManager::openResource(uint32 res) {
 		// hurray, load it in.
 		file.read(_resList[res]->ad, len);
 
+		if (dump) {
+			_standardHeader *header = (_standardHeader *) _resList[res]->ad;
+			char buf[256];
+			char tag[10];
+			File out;
+
+			switch (header->fileType) {
+			case ANIMATION_FILE:
+				strcpy(tag, "anim");
+				break;
+			case SCREEN_FILE:
+				strcpy(tag, "layer");
+				break;
+			case GAME_OBJECT:
+				strcpy(tag, "object");
+				break;
+			case WALK_GRID_FILE:
+				strcpy(tag, "walkgrid");
+				break;
+			case GLOBAL_VAR_FILE:
+				strcpy(tag, "globals");
+				break;
+			case PARALLAX_FILE_null:
+				strcpy(tag, "parallax");	// Not used!
+				break;
+			case RUN_LIST:
+				strcpy(tag, "runlist");
+				break;
+			case TEXT_FILE:
+				strcpy(tag, "text");
+				break;
+			case SCREEN_MANAGER:
+				strcpy(tag, "screen");
+				break;
+			case MOUSE_FILE:
+				strcpy(tag, "mouse");
+				break;
+			case ICON_FILE:
+				strcpy(tag, "icon");
+				break;
+			default:
+				strcpy(tag, "unknown");
+				break;
+			}
+
+#if defined(MACOS_CARBON)
+			sprintf(buf, ":dumps:%s-%d", tag, res);
+#else
+			sprintf(buf, "dumps/%s-%d", tag, res);
+#endif
+
+			out.open(buf, "");
+
+			if (!out.isOpen()) {
+				out.open(buf, "", File::kFileWriteMode);
+				if (out.isOpen())
+					out.write(_resList[res]->ad, len);
+			}
+
+			if (out.isOpen())
+				out.close();
+		}
+
 		// close the cluster
 		file.close();
 
@@ -489,7 +532,7 @@ uint8 *ResourceManager::openResource(uint32 res) {
 		convertEndian((uint8 *) _resList[res]->ad, len);
 #endif
 	} else {
-		debug(5, "RO %d, already open count=%d", res, _count[res]);
+		debug(9, "RO %d, already open count=%d", res, _count[res]);
 	}
 
 	// number of times opened - the file won't move in memory while count
@@ -531,16 +574,12 @@ void ResourceManager::nextCycle(void) {
 	// increment the cycle and calculate actual per-cycle memory useage
 
 #ifdef _SWORD2_DEBUG
-	uint32 j;
-#endif
-
-#ifdef _SWORD2_DEBUG
 	_currentMemoryUsage = 0;
 
-	for (j = 1; j < _totalResFiles; j++) {
+	for (int i = 1; i < _totalResFiles; i++) {
 		// was accessed last cycle
-		if (_age[j] == _resTime)
-			_currentMemoryUsage += _resList[j]->size;
+		if (_age[i] == _resTime)
+			_currentMemoryUsage += _resList[i]->size;
 	}
 #endif
 
@@ -566,14 +605,8 @@ void ResourceManager::closeResource(uint32 res) {
 	// decrements the count
 	// resource floats when count = 0
 
-//#ifdef _SWORD2_DEBUG
-	if (res >= _totalResFiles)
-		error("closing illegal resource %d (there are %d resources 0-%d)", res, _totalResFiles, _totalResFiles - 1);
-
-	//closing but isnt open?
-	if (!(_count[res]))
-		error("closeResource: closing %d but it isn't open", res);
-//#endif
+	assert(res < _totalResFiles);
+	assert(_count[res]);
 
 	//one less has it open
 	_count[res]--;
@@ -712,47 +745,12 @@ void ResourceManager::examine(int res) {
 		Debug_Printf("%d is a null & void resource number\n", res);
 	else {
 		// open up the resource and take a look inside!
-		file_header = (_standardHeader*) openResource(res);
+		file_header = (_standardHeader *) openResource(res);
 
 		// Debug_Printf("%d\n", file_header->fileType);
 		// Debug_Printf("%s\n", file_header->name);
 
-		//----------------------------------------------------
-		// resource types: (taken from header.h)
-
-		// 1:  ANIMATION_FILE
-		//      all normal animations & sprites including mega-sets &
-		//	font files which are the same format
-		// 2:  SCREEN_FILE
-		//      each contains background, palette, layer sprites,
-		//	parallax layers & shading mask
-		// 3:  GAME_OBJECT
-		//      each contains object hub + structures + script data
-		// 4:  WALK_GRID_FILE
-		//      walk-grid data
-		// 5:  GLOBAL_VAR_FILE
-		//      all the global script variables in one file; "there can
-		//	be only one"
-		// 6:  PARALLAX_FILE_null
-		//      NOT USED
-		// 7:  RUN_LIST
-		//      each contains a list of object resource ids
-		// 8:  TEXT_FILE
-		//      each contains all the lines of text for a location or a
-		//	character's conversation script
-		// 9:  SCREEN_MANAGER
-		//      one for each location; this contains special startup
-		//	scripts
-		// 10: MOUSE_FILE
-		//      mouse pointers and luggage icons (sprites in General,
-		//	Mouse pointers & Luggage icons)
-		// 11: WAV_FILE
-		//      NOT USED HERE
-		// 12: ICON_FILE
-		//      menu icon (sprites in General and Menu icons)
-		// 13: PALETTE_FILE
-		//      NOT USED HERE
-		//----------------------------------------------------
+		// Resource types. See header.h
 
 		switch (file_header->fileType) {
 		case ANIMATION_FILE:
diff --git a/sword2/resman.h b/sword2/resman.h
index 5738f4c78c..13b8a52756 100644
--- a/sword2/resman.h
+++ b/sword2/resman.h
@@ -36,7 +36,7 @@ public:
 	// The resource is locked while count != 0
 	// Resource floats when count = 0
 
-	uint8 *openResource(uint32 res);
+	uint8 *openResource(uint32 res, bool dump = false);
 	void closeResource(uint32 res);		// decrements the count
 
 	// returns '0' if resource out of range or null, otherwise '1' for ok
diff --git a/sword2/speech.cpp b/sword2/speech.cpp
index 2c1999bc81..56b584f344 100644
--- a/sword2/speech.cpp
+++ b/sword2/speech.cpp
@@ -87,15 +87,20 @@ int32 Logic::fnChoose(int32 *params) {
 	// the human is switched off so there will be no normal mouse engine
 
 	_mouseEvent *me;
-	uint32 j, hit;
+	uint32 i;
+	int hit;
 	uint8 *icon;
-	uint32 pos = 0;
 
 	AUTO_SELECTED = 0;	// see below
 
 	// new thing to intercept objects held at time of clicking on a person
 
 	if (OBJECT_HELD) {
+		// So that, if there is no match, the speech script uses the
+		// default text for objects that are not accounted for
+
+		int response = _defaultResponseId;
+
 		// If we are using a luggage icon on the person, scan the
 		// subject list to see if this icon would have been available
 		// at this time.
@@ -108,28 +113,20 @@ int32 Logic::fnChoose(int32 *params) {
 		// Note that we won't display the subject icons in this case!
 
 		// scan the subject list for a match with our 'object_held'
-		while (pos < IN_SUBJECT) {
-			if (_subjectList[pos].res == OBJECT_HELD) {
-				// if we've found a match, clear it so it
-				// doesn't keep happening!
-				OBJECT_HELD = 0;
-
-				// clear the subject list
-				IN_SUBJECT = 0;
 
-				// return special subject chosen code (same
+		for (i = 0; i < IN_SUBJECT; i++) {
+			if (_subjectList[i].res == OBJECT_HELD) {
+				// Return special subject chosen code (same
 				// as in normal chooser routine below)
-				return IR_CONT + (_subjectList[pos].ref << 3);
+				response = _subjectList[i].ref;
+				break;
 			}
-			pos++;
 		}
 
 		OBJECT_HELD = 0; // clear it so it doesn't keep happening!
 		IN_SUBJECT = 0;	 // clear the subject list
 
-		// so that the speech script uses the default text for
-		// objects that are not accounted for
-		return IR_CONT + (_defaultResponseId << 3);
+		return IR_CONT + (response << 3);
 	}
 
 	// new thing for skipping chooser with "nothing else to say" text
@@ -156,16 +153,16 @@ int32 Logic::fnChoose(int32 *params) {
 		// init top menu from master list
 		// all icons are highlighted / full colour
 
-		for (j = 0; j < 15; j++) {
-			if (j < IN_SUBJECT) {
-				debug(5, " ICON res %d for %d", _subjectList[j].res, j);
-				icon = _vm->_resman->openResource(_subjectList[j].res) + sizeof(_standardHeader) + RDMENU_ICONWIDE * RDMENU_ICONDEEP;
-				_vm->_graphics->setMenuIcon(RDMENU_BOTTOM, (uint8) j, icon);
-				_vm->_resman->closeResource(_subjectList[j].res);
+		for (i = 0; i < 15; i++) {
+			if (i < IN_SUBJECT) {
+				debug(5, " ICON res %d for %d", _subjectList[i].res, i);
+				icon = _vm->_resman->openResource(_subjectList[i].res) + sizeof(_standardHeader) + RDMENU_ICONWIDE * RDMENU_ICONDEEP;
+				_vm->_graphics->setMenuIcon(RDMENU_BOTTOM, (uint8) i, icon);
+				_vm->_resman->closeResource(_subjectList[i].res);
 			} else {
-				//no icon here
-				debug(5, " NULL for %d", j);
-				_vm->_graphics->setMenuIcon(RDMENU_BOTTOM, (uint8) j, NULL);
+				// no icon here
+				debug(5, " NULL for %d", i);
+				_vm->_graphics->setMenuIcon(RDMENU_BOTTOM, (uint8) i, NULL);
 			}
 		}
 
@@ -179,68 +176,63 @@ int32 Logic::fnChoose(int32 *params) {
 
 		// again next cycle
 		return IR_REPEAT;
-	} else {
-		// menu is there - we're just waiting for a click
-		debug(5, "choosing");
+	}
 
-		me = _vm->_input->mouseEvent();
+	// menu is there - we're just waiting for a click
+	debug(5, "choosing");
 
-		// we only care about left clicks
-		// we ignore mouse releases
+	me = _vm->_input->mouseEvent();
 
-		if (me && (me->buttons & RD_LEFTBUTTONDOWN)) {
-			// check for click on a menu
-			// if so then end the choose, highlight only the
-			// chosen, blank the mouse and return the ref code * 8
+	// we only care about left clicks
+	// we ignore mouse releases
 
-			if (_vm->_input->_mouseY > 399 && _vm->_input->_mouseX >= 24 && _vm->_input->_mouseX < 640 - 24) {
-				//which are we over?
-				hit = (_vm->_input->_mouseX - 24) / 40;
+	if (!me || !(me->buttons & RD_LEFTBUTTONDOWN) || _vm->_input->_mouseY < 400) {
+		debug(5, "end choose");
+		return IR_REPEAT;
+	}
 
-				//clicked on something - what button?
-				if (hit < IN_SUBJECT) {
-					debug(5, "Icons available:");
+	// Check for click on a menu. If so then end the choose, highlight only
+	// the chosen, blank the mouse and return the ref code * 8
 
-					// change icons
-					for (j = 0; j < IN_SUBJECT; j++) {
-						debug(5, "%s", _vm->fetchObjectName(_subjectList[j].res));
+	hit = _vm->menuClick(IN_SUBJECT);
 
-						// change all others to grey
-						if (j != hit) {
-							icon = _vm->_resman->openResource( _subjectList[j].res ) + sizeof(_standardHeader);
-							_vm->_graphics->setMenuIcon(RDMENU_BOTTOM, (uint8) j, icon);
-							_vm->_resman->closeResource(_subjectList[j].res);
-						}
-					}
+	if (hit < 0) {
+		debug(5, "end choose");
+		return IR_REPEAT;
+	}
 
+	debug(5, "Icons available:");
 
-					debug(5, "Selected: %s", _vm->fetchObjectName(_subjectList[hit].res));
+	// change icons
+	for (i = 0; i < IN_SUBJECT; i++) {
+		debug(5, "%s", _vm->fetchObjectName(_subjectList[i].res));
 
-					// this is our looping flag
-					_choosing = false;
+		// change all others to grey
+		if (i != (uint32) hit) {
+			icon = _vm->_resman->openResource(_subjectList[i].res) + sizeof(_standardHeader);
+			_vm->_graphics->setMenuIcon(RDMENU_BOTTOM, (uint8) i, icon);
+			_vm->_resman->closeResource(_subjectList[i].res);
+		}
+	}
 
-					IN_SUBJECT = 0;
+	debug(2, "Selected: %s", _vm->fetchObjectName(_subjectList[hit].res));
 
-					// blank mouse again
-					_vm->setMouse(0);
+	// this is our looping flag
+	_choosing = false;
 
-					debug(5, "hit %d - ref %d  ref*8 %d", hit, _subjectList[hit].ref, _subjectList[hit].ref * 8);
+	IN_SUBJECT = 0;
 
-					// for non-speech scripts that manually
-					// call the chooser
-					RESULT = _subjectList[hit].res;
+	// blank mouse again
+	_vm->setMouse(0);
 
-					// return special subject chosen code
-					return IR_CONT + (_subjectList[hit].ref << 3);
-				}
-			}
-		}
+	debug(5, "hit %d - ref %d  ref*8 %d", hit, _subjectList[hit].ref, _subjectList[hit].ref * 8);
 
-		debug(5, "end choose");
+	// for non-speech scripts that manually
+	// call the chooser
+	RESULT = _subjectList[hit].res;
 
-		// again next cycle
-		return IR_REPEAT;
-	}
+	// return special subject chosen code
+	return IR_CONT + (_subjectList[hit].ref << 3);
 }
 
 int32 Logic::fnStartConversation(int32 *params) {
@@ -380,7 +372,7 @@ int32 Logic::fnTheyDoWeWait(int32 *params) {
 	if (!INS_COMMAND && RESULT == 1 && ob_logic->looping == 0) {
 		// first time so set up targets command if target is waiting
 
-		debug(5, "FNtdww sending command to %d", target);
+		debug(5, "fnTheyDoWeWait sending command to %d", target);
 
 		SPEECH_ID = params[1];
 		INS_COMMAND = params[2];
@@ -413,7 +405,7 @@ int32 Logic::fnTheyDoWeWait(int32 *params) {
 
 	if (RESULT == 1) {
 		// its waiting now so we can be finished with all this
-		debug(5, "FNtdww finished");
+		debug(5, "fnTheyDoWeWait finished");
 
 		// not looping anymore
 		ob_logic->looping = 0;
@@ -424,7 +416,7 @@ int32 Logic::fnTheyDoWeWait(int32 *params) {
 		return IR_CONT;
 	}
 
-	debug(5, "FNtdww just waiting");
+	debug(5, "fnTheyDoWeWait just waiting");
 
 	// debug flag to indicate who we're waiting for - see debug.cpp
 	_speechScriptWaiting = target;
@@ -572,7 +564,7 @@ int32 Logic::fnSpeechProcess(int32 *params) {
 
 	debug(5, "  SP");
 
-	while(1) {
+	while (1) {
 		//we are currently running a command
 		switch (ob_speech->command) {
 		case 0:
diff --git a/sword2/sword2.h b/sword2/sword2.h
index 0839611c7d..ab639fcf50 100644
--- a/sword2/sword2.h
+++ b/sword2/sword2.h
@@ -114,8 +114,6 @@ private:
 	void startNewPalette(void);
 	void processLayer(uint32 layer_number);
 
-	int menuClick(int menu_items);
-
 	void getPlayerStructures(void);
 	void putPlayerStructures(void);
 
@@ -205,6 +203,8 @@ public:
 	menu_object _masterMenuList[TOTAL_engine_pockets];
 	uint32 _totalMasters;
 
+	int menuClick(int menu_items);
+
 	void buildMenu(void);
 	void buildSystemMenu(void);