- Moved the Sierra game ID conversion code inside game.cpp, so that any game-specific workarounds are tested against ScummVM IDs

- Added detection for the EcoQuest 1 demo in the fallback detector
- Partially reverted my previous "fix" for EcoQuest 1 CD, and turned it into a script-specific hack for that game, for now
- Added handling of kDisposeScript calls made with 2 parameters, e.g. in KQ5CD and others (still untested, haven't found a test case)

svn-id: r43887

6 files changed, 166 insertions, 141 deletions

diff --git a/engines/sci/detection.cpp b/engines/sci/detection.cpp
index d2615e8b4e..6f9e5a1c1b 100644
--- a/engines/sci/detection.cpp
+++ b/engines/sci/detection.cpp
@@ -29,6 +29,7 @@
 #include "sci/sci.h"
 #include "sci/engine/kernel.h"
 #include "sci/engine/seg_manager.h"
+#include "sci/engine/vm.h"		// for convertSierraGameId
 
 namespace Sci {
 
@@ -163,136 +164,6 @@ public:
 	const ADGameDescription *fallbackDetect(const Common::FSList &fslist) const;
 };
 
-struct OldNewIdTableEntry {
-	const char *oldId;
-	const char *newId;
-	bool demo;
-};
-
-static const OldNewIdTableEntry s_oldNewTable[] = {
-	{ "demo",		"christmas1988",	false },
-	// iceman is the same
-	{ "icedemo",	"iceman",			true },
-	// longbow is the same
-	{ "rh",			"longbow",			true },
-	{ "eco",		"ecoquest",			false },
-	{ "eco2",		"ecoquest2",		true },		// EcoQuest 2 demo
-	{ "rain",		"ecoquest2",		false },	// EcoQuest 2 full
-	{ "fp",			"freddypharkas",	false },
-	{ "emc",		"funseeker",		false },
-	{ "gk",			"gk1",				false },
-	{ "hoyledemo",	"hoyle1",			true },
-	{ "cardgames",	"hoyle1",			false },
-	{ "solitare",	"hoyle2",			false },
-	// hoyle3 is the same
-	// hoyle4 is the same
-	{ "demo000",	"kq1sci",			true },
-	{ "kq1",		"kq1sci",			false },
-	{ "kq4",		"kq4sci",			false },
-	{ "ll1",		"lsl1sci",			true },
-	{ "lsl1",		"lsl1sci",			false },
-	// lsl2 is the same
-	{ "ll5",		"lsl5",				true },
-	// lsl5 is the same
-	// lsl6 is the same
-	{ "mg",			"mothergoose",		false },
-	{ "cb1",		"laurabow",			false },
-	{ "lb2",		"laurabow2",		false },
-	{ "twisty",		"pepper",			false },
-	{ "pq",			"pq2",				false },
-	{ "trial",		"qfg2",				false },
-	{ "hq2demo",	"qfg2",				true },
-	{ "thegame",	"slater",			false },
-	{ "sq1demo",	"sq1sci",			true },
-	{ "sq1",		"sq1sci",			false },
-	// sq5 is the same
-
-	{ 0, 0, 0 }
-};
-
-static const char *convertSierraGameId(const char *gameName, uint32 *gameFlags) {
-	// Convert the id to lower case, so that we match all upper/lower case variants.
-	Common::String sierraId = gameName;
-	sierraId.toLowercase();
-
-	// TODO: SCI32 IDs
-
-	for (const OldNewIdTableEntry *cur = s_oldNewTable; cur->oldId != 0; ++cur) {
-		if (sierraId == cur->oldId) {
-			if (cur->demo)
-				*gameFlags |= ADGF_DEMO;
-			return cur->newId;
-		}
-	}
-
-	if (sierraId == "card") {
-		// This could either be christmas1990 or christmas1992
-		// christmas1990 has a "resource.001" file, whereas 
-		// christmas1992 has a "resource.000" file
-		return (Common::File::exists("resource.001")) ? "christmas1990" : "christmas1992";
-	}
-	if (sierraId == "arthur") {
-		if (!Common::File::exists("resource.002"))
-			*gameFlags |= ADGF_DEMO;
-		return "camelot";
-	}
-	if (sierraId == "brain") {
-		// This could either be The Castle of Dr. Brain, or The Island of Dr. Brain
-		// castlebrain has resource.001, whereas islandbrain doesn't
-		return (Common::File::exists("resource.001")) ? "castlebrain" : "islandbrain";
-	}
-	if (sierraId == "lsl3") {
-		if (!Common::File::exists("resource.003"))
-			*gameFlags |= ADGF_DEMO;
-		return "lsl3";
-	}
-	// TODO: lslcasino
-	if (sierraId == "tales") {
-		if (!Common::File::exists("resource.002"))
-			*gameFlags |= ADGF_DEMO;
-		return "fairytales";
-	}
-	// TODO: pq1sci (its resources can't be read)
-	if (sierraId == "pq3") {
-		// The pq3 demo comes with resource.000 and resource.001
-		// The full version was released with several resource.* files,
-		// or one big resource.000 file
-		if (Common::File::exists("resource.000") && Common::File::exists("resource.001") &&
-			!Common::File::exists("resource.002"))
-			*gameFlags |= ADGF_DEMO;
-		return "pq3";
-	}
-	if (sierraId == "glory" || sierraId == "hq") {
-		// This could either be qfg1 or qfg3 or qfg4
-		// qfg3 has resource.aud, qfg4 has resource.sfx
-		if (Common::File::exists("resource.aud"))
-			return "qfg3";
-		else if (Common::File::exists("resource.sfx"))
-			return "qfg4";
-		else
-			return "qfg1";
-	}
-	// TODO: qfg1 VGA (its resources can't be read)
-	if (sierraId == "sq3") {
-		// Both SQ3 and the separately released subgame, Astro Chicken,
-		// have internal ID "sq3", but Astro Chicken only has "resource.map"
-		// and "resource.001". Detect if it's SQ3 by the existence of
-		// "resource.002"
-		return (Common::File::exists("resource.002")) ? "sq3" : "astrochicken";
-	}
-	if (sierraId == "sq4") {
-		// Both SQ4 and the separately released subgame, Ms. Astro Chicken,
-		// have internal ID "sq4", but Astro Chicken only has "resource.map"
-		// and "resource.001". Detect if it's SQ4 by the existence of
-		// "resource.000" (which exists in both SQ4 floppy and CD, but not in
-		// the subgame)
-		return (Common::File::exists("resource.000")) ? "sq4" : "msastrochicken";
-	}
-
-	// FIXME: Evil use of strdup here (we are leaking that memory, too)
-	return strdup(sierraId.c_str());
-}
-
 Common::Language charToScummVMLanguage(const char c) {
 	switch (c) {
 	case 'F':
diff --git a/engines/sci/engine/game.cpp b/engines/sci/engine/game.cpp
index cad73f8681..16a0e3320a 100644
--- a/engines/sci/engine/game.cpp
+++ b/engines/sci/engine/game.cpp
@@ -37,6 +37,140 @@
 
 namespace Sci {
 
+struct OldNewIdTableEntry {
+	const char *oldId;
+	const char *newId;
+	bool demo;
+};
+
+static const OldNewIdTableEntry s_oldNewTable[] = {
+	{ "demo",		"christmas1988",	false },
+	// iceman is the same
+	{ "icedemo",	"iceman",			true },
+	// longbow is the same
+	{ "rh",			"longbow",			true },
+	{ "eco2",		"ecoquest2",		true },		// EcoQuest 2 demo
+	{ "rain",		"ecoquest2",		false },	// EcoQuest 2 full
+	{ "fp",			"freddypharkas",	false },
+	{ "emc",		"funseeker",		false },
+	{ "gk",			"gk1",				false },
+	{ "hoyledemo",	"hoyle1",			true },
+	{ "cardgames",	"hoyle1",			false },
+	{ "solitare",	"hoyle2",			false },
+	// hoyle3 is the same
+	// hoyle4 is the same
+	{ "demo000",	"kq1sci",			true },
+	{ "kq1",		"kq1sci",			false },
+	{ "kq4",		"kq4sci",			false },
+	{ "ll1",		"lsl1sci",			true },
+	{ "lsl1",		"lsl1sci",			false },
+	// lsl2 is the same
+	{ "ll5",		"lsl5",				true },
+	// lsl5 is the same
+	// lsl6 is the same
+	{ "mg",			"mothergoose",		false },
+	{ "cb1",		"laurabow",			false },
+	{ "lb2",		"laurabow2",		false },
+	{ "twisty",		"pepper",			false },
+	{ "pq",			"pq2",				false },
+	{ "trial",		"qfg2",				false },
+	{ "hq2demo",	"qfg2",				true },
+	{ "thegame",	"slater",			false },
+	{ "sq1demo",	"sq1sci",			true },
+	{ "sq1",		"sq1sci",			false },
+	// sq5 is the same
+
+	{ 0, 0, 0 }
+};
+
+const char *convertSierraGameId(const char *gameName, uint32 *gameFlags) {
+	// Convert the id to lower case, so that we match all upper/lower case variants.
+	Common::String sierraId = gameName;
+	sierraId.toLowercase();
+
+	// TODO: SCI32 IDs
+
+	for (const OldNewIdTableEntry *cur = s_oldNewTable; cur->oldId != 0; ++cur) {
+		if (sierraId == cur->oldId) {
+			if (cur->demo)
+				*gameFlags |= ADGF_DEMO;
+			return cur->newId;
+		}
+	}
+
+	if (sierraId == "card") {
+		// This could either be christmas1990 or christmas1992
+		// christmas1990 has a "resource.001" file, whereas 
+		// christmas1992 has a "resource.000" file
+		return (Common::File::exists("resource.001")) ? "christmas1990" : "christmas1992";
+	}
+	if (sierraId == "arthur") {
+		if (!Common::File::exists("resource.002"))
+			*gameFlags |= ADGF_DEMO;
+		return "camelot";
+	}
+	if (sierraId == "brain") {
+		// This could either be The Castle of Dr. Brain, or The Island of Dr. Brain
+		// castlebrain has resource.001, whereas islandbrain doesn't
+		return (Common::File::exists("resource.001")) ? "castlebrain" : "islandbrain";
+	}
+	if (sierraId == "eco") {
+		if (!Common::File::exists("resource.000"))
+			*gameFlags |= ADGF_DEMO;
+		return "ecoquest";
+	}
+	if (sierraId == "lsl3") {
+		if (!Common::File::exists("resource.003"))
+			*gameFlags |= ADGF_DEMO;
+		return "lsl3";
+	}
+	// TODO: lslcasino
+	if (sierraId == "tales") {
+		if (!Common::File::exists("resource.002"))
+			*gameFlags |= ADGF_DEMO;
+		return "fairytales";
+	}
+	// TODO: pq1sci (its resources can't be read)
+	if (sierraId == "pq3") {
+		// The pq3 demo comes with resource.000 and resource.001
+		// The full version was released with several resource.* files,
+		// or one big resource.000 file
+		if (Common::File::exists("resource.000") && Common::File::exists("resource.001") &&
+			!Common::File::exists("resource.002"))
+			*gameFlags |= ADGF_DEMO;
+		return "pq3";
+	}
+	if (sierraId == "glory" || sierraId == "hq") {
+		// This could either be qfg1 or qfg3 or qfg4
+		// qfg3 has resource.aud, qfg4 has resource.sfx
+		if (Common::File::exists("resource.aud"))
+			return "qfg3";
+		else if (Common::File::exists("resource.sfx"))
+			return "qfg4";
+		else
+			return "qfg1";
+	}
+	// TODO: qfg1 VGA (its resources can't be read)
+	if (sierraId == "sq3") {
+		// Both SQ3 and the separately released subgame, Astro Chicken,
+		// have internal ID "sq3", but Astro Chicken only has "resource.map"
+		// and "resource.001". Detect if it's SQ3 by the existence of
+		// "resource.002"
+		return (Common::File::exists("resource.002")) ? "sq3" : "astrochicken";
+	}
+	if (sierraId == "sq4") {
+		// Both SQ4 and the separately released subgame, Ms. Astro Chicken,
+		// have internal ID "sq4", but Astro Chicken only has "resource.map"
+		// and "resource.001". Detect if it's SQ4 by the existence of
+		// "resource.000" (which exists in both SQ4 floppy and CD, but not in
+		// the subgame)
+		return (Common::File::exists("resource.000")) ? "sq4" : "msastrochicken";
+	}
+
+	// FIXME: Evil use of strdup here (we are leaking that memory, too)
+	return strdup(sierraId.c_str());
+}
+
 int _reset_graphics_input(EngineState *s) {
 	Resource *resource;
 	int font_nr;
@@ -285,7 +419,7 @@ void script_free_breakpoints(EngineState *s) {
 /*************************************************************/
 
 int game_init(EngineState *s) {
-	// FIXME Use new VM instantiation code all over the place"
+	// FIXME Use new VM instantiation code all over the place
 	DataStack *stack;
 
 	stack = s->segmentManager->allocateStack(VM_STACK_SIZE, &s->stack_segment);
@@ -327,7 +461,8 @@ int game_init(EngineState *s) {
 //	script_dissect(0, s->_selectorNames);
 	// The first entry in the export table of script 0 points to the game object
 	s->game_obj = script_lookup_export(s->segmentManager, 0, 0);
-	s->_gameName = obj_get_name(s->segmentManager, s->game_obj);
+	uint32 gameFlags = 0;	// unused
+	s->_gameName = convertSierraGameId(obj_get_name(s->segmentManager, s->game_obj), &gameFlags);
 
 	debug(2, " \"%s\" at %04x:%04x", s->_gameName.c_str(), PRINT_REG(s->game_obj));
 
diff --git a/engines/sci/engine/kgraphics.cpp b/engines/sci/engine/kgraphics.cpp
index d77bb219e4..aa2567c525 100644
--- a/engines/sci/engine/kgraphics.cpp
+++ b/engines/sci/engine/kgraphics.cpp
@@ -2391,7 +2391,7 @@ reg_t kSetPort(EngineState *s, int funct_nr, int argc, reg_t *argv) {
 		// LSL6 calls kSetPort to extend the screen to draw the GUI. If we free all resources
 		// here, the background picture is freed too, and this makes everything a big mess.
 		// FIXME/TODO: This code really needs to be rewritten to conform to the original behavior
-		if (s->_gameName != "LSL6") {
+		if (s->_gameName != "lsl6") {
 			s->gfx_state->pic_port_bounds = gfx_rect(argv[5].toUint16(), argv[4].toUint16(), argv[3].toUint16(), argv[2].toUint16());
 
 			// FIXME: Should really only invalidate all loaded pic resources here;
diff --git a/engines/sci/engine/kpathing.cpp b/engines/sci/engine/kpathing.cpp
index 0c88f1ddd9..b55cc88fc0 100644
--- a/engines/sci/engine/kpathing.cpp
+++ b/engines/sci/engine/kpathing.cpp
@@ -1236,7 +1236,7 @@ static Polygon *convert_polygon(EngineState *s, reg_t polygon) {
 
 	// WORKAROUND: broken polygon in LSL1VGA, room 350, after opening elevator
 	// Polygon has 17 points but size is set to 19
-	if ((size == 19) && (s->_gameName == "LSL1")) {
+	if ((size == 19) && (s->_gameName == "lsl1sci")) {
 		if ((s->currentRoomNumber() == 350)
 		&& (read_point(list, is_reg_t, 18) == Common::Point(108, 137))) {
 			debug(1, "Applying fix for broken polygon in LSL1VGA, room 350");
@@ -1245,21 +1245,21 @@ static Polygon *convert_polygon(EngineState *s, reg_t polygon) {
 	}
 
 	// WORKAROUND: self-intersecting polygons in ECO, rooms 221, 280 and 300
-	if ((size == 11) && (s->_gameName == "eco")) {
+	if ((size == 11) && (s->_gameName == "ecoquest")) {
 		if ((s->currentRoomNumber() == 300)
 		&& (read_point(list, is_reg_t, 10) == Common::Point(221, 0))) {
 			debug(1, "Applying fix for self-intersecting polygon in ECO, room 300");
 			size = 10;
 		}
 	}
-	if ((size == 12) && (s->_gameName == "eco")) {
+	if ((size == 12) && (s->_gameName == "ecoquest")) {
 		if ((s->currentRoomNumber() == 280)
 		&& (read_point(list, is_reg_t, 11) == Common::Point(238, 189))) {
 			debug(1, "Applying fix for self-intersecting polygon in ECO, room 280");
 			size = 10;
 		}
 	}
-	if ((size == 16) && (s->_gameName == "eco")) {
+	if ((size == 16) && (s->_gameName == "ecoquest")) {
 		if ((s->currentRoomNumber() == 221)
 		&& (read_point(list, is_reg_t, 1) == Common::Point(419, 175))) {
 			debug(1, "Applying fix for self-intersecting polygon in ECO, room 221");
@@ -1441,7 +1441,7 @@ static PathfindingState *convert_polygon_set(EngineState *s, reg_t poly_list, Co
 			return NULL;
 		}
 
-		if (s->_gameName == "Longbow" && s->currentRoomNumber() == 210)
+		if (s->_gameName == "longbow" && s->currentRoomNumber() == 210)
 				fixLongbowRoom210(pf_s, *new_start, *new_end);
 
 		// Merge start and end points into polygon set
diff --git a/engines/sci/engine/kscripts.cpp b/engines/sci/engine/kscripts.cpp
index 2419130bfa..7df7256f70 100644
--- a/engines/sci/engine/kscripts.cpp
+++ b/engines/sci/engine/kscripts.cpp
@@ -301,14 +301,25 @@ reg_t kDisposeScript(EngineState *s, int funct_nr, int argc, reg_t *argv) {
 	Script *scr = s->segmentManager->getScriptIfLoaded(id);
 	if (scr) {
 		if (s->_executionStack.back().addr.pc.segment != id)
-			// Lockers must be > 1, otherwise it won't have any effect on script_uninstantiate() below,
-			// because it decreases the lockers by 1. This occurs for example at the beginning of EcoQuest CD
+			scr->setLockers(1);
+
+		// HACK for EcoQuest CD
+		if (s->_gameName == "ecoquest" && script == 821) {
+			warning("kDisposeScript hack for EcoQuest 1 CD: not disposing script 821");
 			scr->setLockers(2);
+		}
 	}
 
 	script_uninstantiate(s->segmentManager, script);
 	s->_executionStackPosChanged = true;
-	return s->r_acc;
+
+	if (argc != 2) {
+		return s->r_acc;
+	} else {
+		// This exists in the KQ5CD interpreter, but a test case hasn't been found yet
+		warning("kDisposeScript called with 2 parameters, still untested");
+		return argv[1];
+	}
 }
 
 int is_heap_object(EngineState *s, reg_t pos) {
diff --git a/engines/sci/engine/vm.h b/engines/sci/engine/vm.h
index f4bc0d142f..212b855b71 100644
--- a/engines/sci/engine/vm.h
+++ b/engines/sci/engine/vm.h
@@ -498,6 +498,14 @@ int script_instantiate(ResourceManager *resourceManager, SegManager *segManager,
 void script_uninstantiate(SegManager *segManager, int script_nr);
 
 /**
+ * Converts the builtin Sierra game IDs to the ones we use in ScummVM
+ * @param[in] gameName		The internal game name
+ * @param[in] gameFlags     The game's flags, which are adjusted accordingly for demos
+ * @return					The equivalent ScummVM game id
+ */
+const char *convertSierraGameId(const char *gameName, uint32 *gameFlags);
+
+/**
  * Initializes an SCI game
  * This function must be run before script_run() is executed. Graphics data
  * is initialized iff s->gfx_state != NULL.