SCI32: Fix broken Remap implementation

Remap would crash SCI2.1early games with 19 remap slots, and
did not actually work in most cases in SCI2.1mid+ games.

The SCI16 implementation was moved to its own separate file but
was otherwise touched as little as possible, so may still have
similar problems to the SCI32 code.

1. Split SCI16 and SCI32 code into separate files
2. Use -32 prefixes for SCI32 code and no prefix for SCI16 code,
   where possible, to match other existing code
3. Avoid accidental corruption of values from the VM that may be
   valid when signed or larger than 8 bits
4. Added documentation
5. Add missing remap CelObj calls
6. Inline where possible in performance-critical code paths
7. Fix bad `matchColor` function, and move it from GfxPalette to
   GfxRemap32 since it is only used by GfxRemap32
8. Fix bad capitalisation in getCycleMap
9. Remove unnecessary initialisation of SingleRemaps
10. Update architecture to more closely mirror how SSCI worked
11. Clarify the purpose of each type of remap type (and
    associated variable names)
12. Split large `apply` function into smaller units
13. Fix buffer overrun when loading a SCI2.1early game with remap
14. Remove use of `#define` constants
15. Warn instead of crashing with an error on invalid input (to
    match SSCI more closely)
16. Change the collision avoidance mechanism between the RemapType
    enum and remap kernel functions
17. Add save/load function

6 files changed, 92 insertions, 50 deletions

diff --git a/engines/sci/engine/kernel.h b/engines/sci/engine/kernel.h
index b269e9789d..1202982986 100644
--- a/engines/sci/engine/kernel.h
+++ b/engines/sci/engine/kernel.h
@@ -412,7 +412,7 @@ reg_t kPlatform(EngineState *s, int argc, reg_t *argv);
 reg_t kTextColors(EngineState *s, int argc, reg_t *argv);
 reg_t kTextFonts(EngineState *s, int argc, reg_t *argv);
 reg_t kShow(EngineState *s, int argc, reg_t *argv);
-reg_t kRemapColors16(EngineState *s, int argc, reg_t *argv);
+reg_t kRemapColors(EngineState *s, int argc, reg_t *argv);
 reg_t kDummy(EngineState *s, int argc, reg_t *argv);
 reg_t kEmpty(EngineState *s, int argc, reg_t *argv);
 reg_t kStub(EngineState *s, int argc, reg_t *argv);
@@ -482,13 +482,13 @@ reg_t kScrollWindowDestroy(EngineState *s, int argc, reg_t *argv);
 
 reg_t kMulDiv(EngineState *s, int argc, reg_t *argv);
 
-reg_t kRemapColors(EngineState *s, int argc, reg_t *argv);
-reg_t kRemapOff(EngineState *s, int argc, reg_t *argv);
-reg_t kRemapByRange(EngineState *s, int argc, reg_t *argv);
-reg_t kRemapByPercent(EngineState *s, int argc, reg_t *argv);
-reg_t kRemapToGray(EngineState *s, int argc, reg_t *argv);
-reg_t kRemapToPercentGray(EngineState *s, int argc, reg_t *argv);
-reg_t kRemapSetNoMatchRange(EngineState *s, int argc, reg_t *argv);
+reg_t kRemapColors32(EngineState *s, int argc, reg_t *argv);
+reg_t kRemapColorsOff(EngineState *s, int argc, reg_t *argv);
+reg_t kRemapColorsByRange(EngineState *s, int argc, reg_t *argv);
+reg_t kRemapColorsByPercent(EngineState *s, int argc, reg_t *argv);
+reg_t kRemapColorsToGray(EngineState *s, int argc, reg_t *argv);
+reg_t kRemapColorsToPercentGray(EngineState *s, int argc, reg_t *argv);
+reg_t kRemapColorsBlockRange(EngineState *s, int argc, reg_t *argv);
 
 reg_t kAddScreenItem(EngineState *s, int argc, reg_t *argv);
 reg_t kUpdateScreenItem(EngineState *s, int argc, reg_t *argv);
diff --git a/engines/sci/engine/kernel_tables.h b/engines/sci/engine/kernel_tables.h
index 19bee359c5..dacaafe757 100644
--- a/engines/sci/engine/kernel_tables.h
+++ b/engines/sci/engine/kernel_tables.h
@@ -414,12 +414,12 @@ static const SciKernelMapSubEntry kList_subops[] = {
 
 //    version,         subId, function-mapping,                    signature,              workarounds
 static const SciKernelMapSubEntry kRemapColors_subops[] = {
-	{ SIG_SCI32,           0, MAP_CALL(RemapOff),                 "(i)",                  NULL },
-	{ SIG_SCI32,           1, MAP_CALL(RemapByRange),             "iiii(i)",              NULL },
-	{ SIG_SCI32,           2, MAP_CALL(RemapByPercent),           "ii(i)",                NULL },
-	{ SIG_SCI32,           3, MAP_CALL(RemapToGray),              "ii(i)",                NULL },
-	{ SIG_SCI32,           4, MAP_CALL(RemapToPercentGray),       "iii(i)",               NULL },
-	{ SIG_SCI32,           5, MAP_CALL(RemapSetNoMatchRange),     "ii",                   NULL },
+	{ SIG_SCI32,           0, MAP_CALL(RemapColorsOff),            "(i)",                  NULL },
+	{ SIG_SCI32,           1, MAP_CALL(RemapColorsByRange),        "iiii(i)",              NULL },
+	{ SIG_SCI32,           2, MAP_CALL(RemapColorsByPercent),      "ii(i)",                NULL },
+	{ SIG_SCI32,           3, MAP_CALL(RemapColorsToGray),         "ii(i)",                NULL },
+	{ SIG_SCI32,           4, MAP_CALL(RemapColorsToPercentGray),  "iii(i)",               NULL },
+	{ SIG_SCI32,           5, MAP_CALL(RemapColorsBlockRange),     "ii",                   NULL },
 	SCI_SUBOPENTRY_TERMINATOR
 };
 
@@ -634,9 +634,9 @@ static SciKernelMapEntry s_kernelMap[] = {
 	{ MAP_CALL(PriCoord),          SIG_EVERYWHERE,           "i",                     NULL,            NULL },
 	{ MAP_CALL(Random),            SIG_EVERYWHERE,           "i(i)(i)",               NULL,            NULL },
 	{ MAP_CALL(ReadNumber),        SIG_EVERYWHERE,           "r",                     NULL,            kReadNumber_workarounds },
-	{ "RemapColors", kRemapColors16,       SIG_SCI11, SIGFOR_ALL,    "i(i)(i)(i)(i)", NULL,            NULL },
+	{ MAP_CALL(RemapColors),       SIG_SCI11, SIGFOR_ALL,    "i(i)(i)(i)(i)",         NULL,            NULL },
 #ifdef ENABLE_SCI32
-	{ MAP_CALL(RemapColors),       SIG_SCI32, SIGFOR_ALL,    "i(i)(i)(i)(i)(i)",      kRemapColors_subops, NULL },
+	{ "RemapColors", kRemapColors32, SIG_SCI32, SIGFOR_ALL,  "i(i)(i)(i)(i)(i)",      kRemapColors_subops, NULL },
 #endif
 	{ MAP_CALL(ResCheck),          SIG_EVERYWHERE,           "ii(iiii)",              NULL,            NULL },
 	{ MAP_CALL(RespondsTo),        SIG_EVERYWHERE,           ".i",                    NULL,            NULL },
diff --git a/engines/sci/engine/kgraphics.cpp b/engines/sci/engine/kgraphics.cpp
index 73236b98ed..cae5a09789 100644
--- a/engines/sci/engine/kgraphics.cpp
+++ b/engines/sci/engine/kgraphics.cpp
@@ -1258,7 +1258,7 @@ reg_t kShow(EngineState *s, int argc, reg_t *argv) {
 }
 
 // Early variant of the SCI32 kRemapColors kernel function, used in the demo of QFG4
-reg_t kRemapColors16(EngineState *s, int argc, reg_t *argv) {
+reg_t kRemapColors(EngineState *s, int argc, reg_t *argv) {
 	uint16 operation = argv[0].toUint16();
 
 	switch (operation) {
diff --git a/engines/sci/engine/kgraphics32.cpp b/engines/sci/engine/kgraphics32.cpp
index 9270c81c02..019a06930c 100644
--- a/engines/sci/engine/kgraphics32.cpp
+++ b/engines/sci/engine/kgraphics32.cpp
@@ -54,6 +54,7 @@
 #include "sci/graphics/frameout.h"
 #include "sci/graphics/paint32.h"
 #include "sci/graphics/palette32.h"
+#include "sci/graphics/remap32.h"
 #include "sci/graphics/text32.h"
 #endif
 
@@ -925,57 +926,69 @@ reg_t kPalCycle(EngineState *s, int argc, reg_t *argv) {
 	return s->r_acc;
 }
 
-reg_t kRemapColors(EngineState *s, int argc, reg_t *argv) {
+reg_t kRemapColors32(EngineState *s, int argc, reg_t *argv) {
 	if (!s)
 		return make_reg(0, getSciVersion());
 	error("not supposed to call this");
 }
 
-reg_t kRemapOff(EngineState *s, int argc, reg_t *argv) {
-	byte color = (argc >= 1) ? argv[0].toUint16() : 0;
-	g_sci->_gfxRemap32->remapOff(color);
+reg_t kRemapColorsOff(EngineState *s, int argc, reg_t *argv) {
+	if (argc == 0) {
+		g_sci->_gfxRemap32->remapAllOff();
+	} else {
+		const uint8 color = argv[0].toUint16();
+		g_sci->_gfxRemap32->remapOff(color);
+	}
 	return s->r_acc;
 }
 
-reg_t kRemapByRange(EngineState *s, int argc, reg_t *argv) {
-	byte color = argv[0].toUint16();
-	byte from = argv[1].toUint16();
-	byte to = argv[2].toUint16();
-	byte base = argv[3].toUint16();
-	// The last parameter, depth, is unused
-	g_sci->_gfxRemap32->setRemappingRange(color, from, to, base);
+reg_t kRemapColorsByRange(EngineState *s, int argc, reg_t *argv) {
+	const uint8 color = argv[0].toUint16();
+	const int16 from = argv[1].toSint16();
+	const int16 to = argv[2].toSint16();
+	const int16 base = argv[3].toSint16();
+	// NOTE: There is an optional last parameter after `base`
+	// which was only used by the priority map debugger, which
+	// does not exist in release versions of SSCI
+	g_sci->_gfxRemap32->remapByRange(color, from, to, base);
 	return s->r_acc;
 }
 
-reg_t kRemapByPercent(EngineState *s, int argc, reg_t *argv) {
-	byte color = argv[0].toUint16();
-	byte percent = argv[1].toUint16();
-	// The last parameter, depth, is unused
-	g_sci->_gfxRemap32->setRemappingPercent(color, percent);
+reg_t kRemapColorsByPercent(EngineState *s, int argc, reg_t *argv) {
+	const uint8 color = argv[0].toUint16();
+	const int16 percent = argv[1].toSint16();
+	// NOTE: There is an optional last parameter after `percent`
+	// which was only used by the priority map debugger, which
+	// does not exist in release versions of SSCI
+	g_sci->_gfxRemap32->remapByPercent(color, percent);
 	return s->r_acc;
 }
 
-reg_t kRemapToGray(EngineState *s, int argc, reg_t *argv) {
-	byte color = argv[0].toUint16();
-	byte gray = argv[1].toUint16();
-	// The last parameter, depth, is unused
-	g_sci->_gfxRemap32->setRemappingToGray(color, gray);
+reg_t kRemapColorsToGray(EngineState *s, int argc, reg_t *argv) {
+	const uint8 color = argv[0].toUint16();
+	const int16 gray = argv[1].toSint16();
+	// NOTE: There is an optional last parameter after `gray`
+	// which was only used by the priority map debugger, which
+	// does not exist in release versions of SSCI
+	g_sci->_gfxRemap32->remapToGray(color, gray);
 	return s->r_acc;
 }
 
-reg_t kRemapToPercentGray(EngineState *s, int argc, reg_t *argv) {
-	byte color = argv[0].toUint16();
-	byte gray = argv[1].toUint16();
-	byte percent = argv[2].toUint16();
-	// The last parameter, depth, is unused
-	g_sci->_gfxRemap32->setRemappingToPercentGray(color, gray, percent);
+reg_t kRemapColorsToPercentGray(EngineState *s, int argc, reg_t *argv) {
+	const uint8 color = argv[0].toUint16();
+	const int16 gray = argv[1].toSint16();
+	const int16 percent = argv[2].toSint16();
+	// NOTE: There is an optional last parameter after `percent`
+	// which was only used by the priority map debugger, which
+	// does not exist in release versions of SSCI
+	g_sci->_gfxRemap32->remapToPercentGray(color, gray, percent);
 	return s->r_acc;
 }
 
-reg_t kRemapSetNoMatchRange(EngineState *s, int argc, reg_t *argv) {
-	byte from = argv[0].toUint16();
-	byte count = argv[1].toUint16();
-	g_sci->_gfxRemap32->setNoMatchRange(from, count);
+reg_t kRemapColorsBlockRange(EngineState *s, int argc, reg_t *argv) {
+	const uint8 from = argv[0].toUint16();
+	const uint8 count = argv[1].toUint16();
+	g_sci->_gfxRemap32->blockRange(from, count);
 	return s->r_acc;
 }
 
diff --git a/engines/sci/engine/savegame.cpp b/engines/sci/engine/savegame.cpp
index 302f046458..0972aec4a4 100644
--- a/engines/sci/engine/savegame.cpp
+++ b/engines/sci/engine/savegame.cpp
@@ -48,8 +48,9 @@
 #include "sci/sound/music.h"
 
 #ifdef ENABLE_SCI32
-#include "sci/graphics/palette32.h"
 #include "sci/graphics/frameout.h"
+#include "sci/graphics/palette32.h"
+#include "sci/graphics/remap32.h"
 #endif
 
 namespace Sci {
@@ -807,6 +808,33 @@ void GfxPalette32::saveLoadWithSerializer(Common::Serializer &s) {
 		}
 	}
 }
+
+void GfxRemap32::saveLoadWithSerializer(Common::Serializer &s) {
+	if (s.getVersion() < 35) {
+		return;
+	}
+
+	s.syncAsByte(_numActiveRemaps);
+	s.syncAsByte(_blockedRangeStart);
+	s.syncAsSint16LE(_blockedRangeCount);
+
+	for (uint i = 0; i < _remaps.size(); ++i) {
+		SingleRemap &singleRemap = _remaps[i];
+		s.syncAsByte(singleRemap._type);
+		if (s.isLoading() && singleRemap._type != kRemapNone) {
+			singleRemap.reset();
+		}
+		s.syncAsByte(singleRemap._from);
+		s.syncAsByte(singleRemap._to);
+		s.syncAsByte(singleRemap._delta);
+		s.syncAsByte(singleRemap._percent);
+		s.syncAsByte(singleRemap._gray);
+	}
+
+	if (s.isLoading()) {
+		_needsUpdate = true;
+	}
+}
 #endif
 
 void GfxPorts::saveLoadWithSerializer(Common::Serializer &s) {
diff --git a/engines/sci/engine/savegame.h b/engines/sci/engine/savegame.h
index 459e992e24..43909accf2 100644
--- a/engines/sci/engine/savegame.h
+++ b/engines/sci/engine/savegame.h
@@ -37,6 +37,7 @@ struct EngineState;
  *
  * Version - new/changed feature
  * =============================
+ *      35 - SCI32 remap
  *      34 - SCI32 palettes, and store play time in ticks
  *      33 - new overridePriority flag in MusicEntry
  *      32 - new playBed flag in MusicEntry
@@ -59,7 +60,7 @@ struct EngineState;
  */
 
 enum {
-	CURRENT_SAVEGAME_VERSION = 34,
+	CURRENT_SAVEGAME_VERSION = 35,
 	MINIMUM_SAVEGAME_VERSION = 14
 };