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

1 files changed, 73 insertions, 28 deletions

diff --git a/engines/sci/graphics/celobj32.cpp b/engines/sci/graphics/celobj32.cpp
index 77d333a717..af5ee3abbc 100644
--- a/engines/sci/graphics/celobj32.cpp
+++ b/engines/sci/graphics/celobj32.cpp
@@ -26,13 +26,12 @@
 #include "sci/graphics/celobj32.h"
 #include "sci/graphics/frameout.h"
 #include "sci/graphics/palette32.h"
-#include "sci/graphics/picture.h"
-#include "sci/graphics/remap.h"
+#include "sci/graphics/remap32.h"
 #include "sci/graphics/text32.h"
-#include "sci/graphics/view.h"
 
 namespace Sci {
 #pragma mark CelScaler
+
 CelScaler *CelObj::_scaler = nullptr;
 
 void CelScaler::activateScaleTables(const Ratio &scaleX, const Ratio &scaleY) {
@@ -323,6 +322,10 @@ public:
 #pragma mark -
 #pragma mark CelObj - Remappers
 
+/**
+ * Pixel mapper for a CelObj with transparent pixels and no
+ * remapping data.
+ */
 struct MAPPER_NoMD {
 	inline void draw(byte *target, const byte pixel, const uint8 skipColor) const {
 		if (pixel != skipColor) {
@@ -330,25 +333,49 @@ struct MAPPER_NoMD {
 		}
 	}
 };
+
+/**
+ * Pixel mapper for a CelObj with no transparent pixels and
+ * no remapping data.
+ */
 struct MAPPER_NoMDNoSkip {
 	inline void draw(byte *target, const byte pixel, const uint8) const {
 		*target = pixel;
 	}
 };
 
+/**
+ * Pixel mapper for a CelObj with transparent pixels,
+ * remapping data, and remapping enabled.
+ */
 struct MAPPER_Map {
 	inline void draw(byte *target, const byte pixel, const uint8 skipColor) const {
 		if (pixel != skipColor) {
+			// NOTE: For some reason, SSCI never checks if the source
+			// pixel is *above* the range of remaps.
 			if (pixel < g_sci->_gfxRemap32->getStartColor()) {
 				*target = pixel;
-			} else {
-				if (g_sci->_gfxRemap32->remapEnabled(pixel))
-					*target = g_sci->_gfxRemap32->remapColor(pixel, *target);
+			} else if (g_sci->_gfxRemap32->remapEnabled(pixel)) {
+				*target = g_sci->_gfxRemap32->remapColor(pixel, *target);
 			}
 		}
 	}
 };
 
+/**
+ * Pixel mapper for a CelObj with transparent pixels,
+ * remapping data, and remapping disabled.
+ */
+struct MAPPER_NoMap {
+	inline void draw(byte *target, const byte pixel, const uint8 skipColor) const {
+		// NOTE: For some reason, SSCI never checks if the source
+		// pixel is *above* the range of remaps.
+		if (pixel != skipColor && pixel < g_sci->_gfxRemap32->getStartColor()) {
+			*target = pixel;
+		}
+	}
+};
+
 void CelObj::draw(Buffer &target, const ScreenItem &screenItem, const Common::Rect &targetRect) const {
 	const Common::Point &scaledPosition = screenItem._scaledPosition;
 	const Ratio &scaleX = screenItem._ratioX;
@@ -523,6 +550,7 @@ void CelObj::submitPalette() const {
 
 #pragma mark -
 #pragma mark CelObj - Caching
+
 int CelObj::_nextCacheId = 1;
 CelCache *CelObj::_cache = nullptr;
 
@@ -624,33 +652,35 @@ void dummyFill(Buffer &target, const Common::Rect &targetRect) {
 }
 
 void CelObj::drawHzFlip(Buffer &target, const Common::Rect &targetRect, const Common::Point &scaledPosition) const {
-	debug("drawHzFlip");
-	dummyFill(target, targetRect);
+	render<MAPPER_NoMap, SCALER_NoScale<true, READER_Compressed> >(target, targetRect, scaledPosition);
 }
 
 void CelObj::drawNoFlip(Buffer &target, const Common::Rect &targetRect, const Common::Point &scaledPosition) const {
-	debug("drawNoFlip");
-	dummyFill(target, targetRect);
+	render<MAPPER_NoMap, SCALER_NoScale<false, READER_Compressed> >(target, targetRect, scaledPosition);
 }
 
 void CelObj::drawUncompNoFlip(Buffer &target, const Common::Rect &targetRect, const Common::Point &scaledPosition) const {
-	debug("drawUncompNoFlip");
-	dummyFill(target, targetRect);
+	render<MAPPER_NoMap, SCALER_NoScale<false, READER_Uncompressed> >(target, targetRect, scaledPosition);
 }
 
 void CelObj::drawUncompHzFlip(Buffer &target, const Common::Rect &targetRect, const Common::Point &scaledPosition) const {
-	debug("drawUncompHzFlip");
-	dummyFill(target, targetRect);
+	render<MAPPER_NoMap, SCALER_NoScale<true, READER_Uncompressed> >(target, targetRect, scaledPosition);
 }
 
 void CelObj::scaleDraw(Buffer &target, const Ratio &scaleX, const Ratio &scaleY, const Common::Rect &targetRect, const Common::Point &scaledPosition) const {
-	debug("scaleDraw");
-	dummyFill(target, targetRect);
+	if (_drawMirrored) {
+		render<MAPPER_NoMap, SCALER_Scale<true, READER_Compressed> >(target, targetRect, scaledPosition, scaleX, scaleY);
+	} else {
+		render<MAPPER_NoMap, SCALER_Scale<false, READER_Compressed> >(target, targetRect, scaledPosition, scaleX, scaleY);
+	}
 }
 
 void CelObj::scaleDrawUncomp(Buffer &target, const Ratio &scaleX, const Ratio &scaleY, const Common::Rect &targetRect, const Common::Point &scaledPosition) const {
-	debug("scaleDrawUncomp");
-	dummyFill(target, targetRect);
+	if (_drawMirrored) {
+		render<MAPPER_NoMap, SCALER_Scale<true, READER_Uncompressed> >(target, targetRect, scaledPosition, scaleX, scaleY);
+	} else {
+		render<MAPPER_NoMap, SCALER_Scale<false, READER_Uncompressed> >(target, targetRect, scaledPosition, scaleX, scaleY);
+	}
 }
 
 void CelObj::drawHzFlipMap(Buffer &target, const Common::Rect &targetRect, const Common::Point &scaledPosition) const {
@@ -670,17 +700,19 @@ void CelObj::drawUncompHzFlipMap(Buffer &target, const Common::Rect &targetRect,
 }
 
 void CelObj::scaleDrawMap(Buffer &target, const Ratio &scaleX, const Ratio &scaleY, const Common::Rect &targetRect, const Common::Point &scaledPosition) const {
-	if (_drawMirrored)
+	if (_drawMirrored) {
 		render<MAPPER_Map, SCALER_Scale<true, READER_Compressed> >(target, targetRect, scaledPosition, scaleX, scaleY);
-	else
+	} else {
 		render<MAPPER_Map, SCALER_Scale<false, READER_Compressed> >(target, targetRect, scaledPosition, scaleX, scaleY);
+	}
 }
 
 void CelObj::scaleDrawUncompMap(Buffer &target, const Ratio &scaleX, const Ratio &scaleY, const Common::Rect &targetRect, const Common::Point &scaledPosition) const {
-	if (_drawMirrored)
+	if (_drawMirrored) {
 		render<MAPPER_Map, SCALER_Scale<true, READER_Uncompressed> >(target, targetRect, scaledPosition, scaleX, scaleY);
-	else
+	} else {
 		render<MAPPER_Map, SCALER_Scale<false, READER_Uncompressed> >(target, targetRect, scaledPosition, scaleX, scaleY);
+	}
 }
 
 void CelObj::drawNoFlipNoMD(Buffer &target, const Common::Rect &targetRect, const Common::Point &scaledPosition) const {
@@ -715,14 +747,16 @@ void CelObj::scaleDrawNoMD(Buffer &target, const Ratio &scaleX, const Ratio &sca
 }
 
 void CelObj::scaleDrawUncompNoMD(Buffer &target, const Ratio &scaleX, const Ratio &scaleY, const Common::Rect &targetRect, const Common::Point &scaledPosition) const {
-	if (_drawMirrored)
+	if (_drawMirrored) {
 		render<MAPPER_NoMD, SCALER_Scale<true, READER_Uncompressed> >(target, targetRect, scaledPosition, scaleX, scaleY);
-	else
+	} else {
 		render<MAPPER_NoMD, SCALER_Scale<false, READER_Uncompressed> >(target, targetRect, scaledPosition, scaleX, scaleY);
+	}
 }
 
 #pragma mark -
 #pragma mark CelObjView
+
 CelObjView::CelObjView(const GuiResourceId viewId, const int16 loopNo, const int16 celNo) {
 	_info.type = kCelTypeView;
 	_info.resourceId = viewId;
@@ -840,8 +874,12 @@ CelObjView::CelObjView(const GuiResourceId viewId, const int16 loopNo, const int
 bool CelObjView::analyzeUncompressedForRemap() const {
 	byte *pixels = getResPointer() + READ_SCI11ENDIAN_UINT32(getResPointer() + _celHeaderOffset + 24);
 	for (int i = 0; i < _width * _height; ++i) {
-		byte pixel = pixels[i];
-		if (pixel >= g_sci->_gfxRemap32->getStartColor() && pixel <= g_sci->_gfxRemap32->getEndColor() && pixel != _transparentColor) {
+		const byte pixel = pixels[i];
+		if (
+			pixel >= g_sci->_gfxRemap32->getStartColor() &&
+			pixel <= g_sci->_gfxRemap32->getEndColor() &&
+			pixel != _transparentColor
+		) {
 			return true;
 		}
 	}
@@ -853,8 +891,12 @@ bool CelObjView::analyzeForRemap() const {
 	for (int y = 0; y < _height; y++) {
 		const byte *curRow = reader.getRow(y);
 		for (int x = 0; x < _width; x++) {
-			byte pixel = curRow[x];
-			if (pixel >= g_sci->_gfxRemap32->getStartColor() && pixel <= g_sci->_gfxRemap32->getEndColor() && pixel != _transparentColor) {
+			const byte pixel = curRow[x];
+			if (
+				pixel >= g_sci->_gfxRemap32->getStartColor() &&
+				pixel <= g_sci->_gfxRemap32->getEndColor() &&
+				pixel != _transparentColor
+			) {
 				return true;
 			}
 		}
@@ -881,6 +923,7 @@ byte *CelObjView::getResPointer() const {
 
 #pragma mark -
 #pragma mark CelObjPic
+
 CelObjPic::CelObjPic(const GuiResourceId picId, const int16 celNo) {
 	_info.type = kCelTypePic;
 	_info.resourceId = picId;
@@ -1002,6 +1045,7 @@ byte *CelObjPic::getResPointer() const {
 
 #pragma mark -
 #pragma mark CelObjMem
+
 CelObjMem::CelObjMem(const reg_t bitmapObject) {
 	_info.type = kCelTypeMem;
 	_info.bitmap = bitmapObject;
@@ -1031,6 +1075,7 @@ byte *CelObjMem::getResPointer() const {
 
 #pragma mark -
 #pragma mark CelObjColor
+
 CelObjColor::CelObjColor(const uint8 color, const int16 width, const int16 height) {
 	_info.type = kCelTypeColor;
 	_info.color = color;