SCI32: Clean up GfxPalette32

* Replace raw pointers with smart pointers
* Use references instead of const pointers where appropriate
* Tweak initialisation
* Tweak palette copies to the stack

3 files changed, 78 insertions, 97 deletions

diff --git a/engines/sci/engine/savegame.cpp b/engines/sci/engine/savegame.cpp
index b0fa31a1bb..06a26ec58e 100644
--- a/engines/sci/engine/savegame.cpp
+++ b/engines/sci/engine/savegame.cpp
@@ -866,25 +866,25 @@ void GfxPalette::saveLoadWithSerializer(Common::Serializer &s) {
 }
 
 #ifdef ENABLE_SCI32
-static void saveLoadPalette32(Common::Serializer &s, Palette *const palette) {
-	s.syncAsUint32LE(palette->timestamp);
-	for (int i = 0; i < ARRAYSIZE(palette->colors); ++i) {
-		s.syncAsByte(palette->colors[i].used);
-		s.syncAsByte(palette->colors[i].r);
-		s.syncAsByte(palette->colors[i].g);
-		s.syncAsByte(palette->colors[i].b);
+static void saveLoadPalette32(Common::Serializer &s, Palette &palette) {
+	s.syncAsUint32LE(palette.timestamp);
+	for (int i = 0; i < ARRAYSIZE(palette.colors); ++i) {
+		s.syncAsByte(palette.colors[i].used);
+		s.syncAsByte(palette.colors[i].r);
+		s.syncAsByte(palette.colors[i].g);
+		s.syncAsByte(palette.colors[i].b);
 	}
 }
 
-static void saveLoadOptionalPalette32(Common::Serializer &s, Palette **const palette) {
+static void saveLoadOptionalPalette32(Common::Serializer &s, Common::ScopedPtr<Palette> &palette) {
 	bool hasPalette = false;
 	if (s.isSaving()) {
-		hasPalette = (*palette != nullptr);
+		hasPalette = palette;
 	}
 	s.syncAsByte(hasPalette);
 	if (hasPalette) {
 		if (s.isLoading()) {
-			*palette = new Palette;
+			palette.reset(new Palette);
 		}
 		saveLoadPalette32(s, *palette);
 	}
@@ -899,14 +899,11 @@ void GfxPalette32::saveLoadWithSerializer(Common::Serializer &s) {
 		++_version;
 
 		for (int i = 0; i < kNumCyclers; ++i) {
-			delete _cyclers[i];
-			_cyclers[i] = nullptr;
+			_cyclers[i].reset();
 		}
 
-		delete _varyTargetPalette;
-		_varyTargetPalette = nullptr;
-		delete _varyStartPalette;
-		_varyStartPalette = nullptr;
+		_varyTargetPalette.reset();
+		_varyStartPalette.reset();
 	}
 
 	s.syncAsSint16LE(_varyDirection);
@@ -928,14 +925,14 @@ void GfxPalette32::saveLoadWithSerializer(Common::Serializer &s) {
 
 	if (g_sci->_features->hasLatePaletteCode() && s.getVersion() >= 41) {
 		s.syncAsSint16LE(_gammaLevel);
-		saveLoadPalette32(s, &_sourcePalette);
+		saveLoadPalette32(s, _sourcePalette);
 		++_version;
 		_needsUpdate = true;
 		_gammaChanged = true;
 	}
 
-	saveLoadOptionalPalette32(s, &_varyTargetPalette);
-	saveLoadOptionalPalette32(s, &_varyStartPalette);
+	saveLoadOptionalPalette32(s, _varyTargetPalette);
+	saveLoadOptionalPalette32(s, _varyStartPalette);
 
 	// _nextPalette is not saved by SSCI
 
@@ -944,14 +941,15 @@ void GfxPalette32::saveLoadWithSerializer(Common::Serializer &s) {
 
 		bool hasCycler = false;
 		if (s.isSaving()) {
-			cycler = _cyclers[i];
+			cycler = _cyclers[i].get();
 			hasCycler = (cycler != nullptr);
 		}
 		s.syncAsByte(hasCycler);
 
 		if (hasCycler) {
 			if (s.isLoading()) {
-				_cyclers[i] = cycler = new PalCycler;
+				cycler = new PalCycler;
+				_cyclers[i].reset(cycler);
 			}
 
 			s.syncAsByte(cycler->fromColor);
diff --git a/engines/sci/graphics/palette32.cpp b/engines/sci/graphics/palette32.cpp
index d8a2ead66d..7a3b396da8 100644
--- a/engines/sci/graphics/palette32.cpp
+++ b/engines/sci/graphics/palette32.cpp
@@ -39,11 +39,10 @@ namespace Sci {
 
 HunkPalette::HunkPalette(const SciSpan<const byte> &rawPalette) :
 	_version(0),
-	// NOTE: The header size in palettes is garbage. In at least KQ7 2.00b and
-	// Phant1, the 999.pal sets this value to 0. In most other palettes it is
-	// set to 14, but the *actual* size of the header structure used in SSCI is
-	// 13, which is reflected by `kHunkPaletteHeaderSize`.
-	// _headerSize(rawPalette[0]),
+	// The header size in palettes is garbage. In at least KQ7 2.00b and Phant1,
+	// the 999.pal sets this value to 0. In most other palettes it is set to 14,
+	// but the *actual* size of the header structure used in SSCI is 13, which
+	// is reflected by `kHunkPaletteHeaderSize`.
 	_numPalettes(rawPalette.getUint8At(kNumPaletteEntriesOffset)),
 	_data() {
 	assert(_numPalettes == 0 || _numPalettes == 1);
@@ -371,18 +370,17 @@ static const uint8 gammaTables[GfxPalette32::numGammaTables][256] = {
 	_varyLastTick(0),
 	_varyTime(0),
 	_varyDirection(0),
+	_varyPercent(0),
 	_varyTargetPercent(0),
 	_varyNumTimesPaused(0),
 
 	// Palette cycling
-	_cyclers(),
 	_cycleMap(),
 
 	// Gamma correction
 	_gammaLevel(-1),
 	_gammaChanged(false) {
 
-	_varyPercent = _varyTargetPercent;
 	for (int i = 0, len = ARRAYSIZE(_fadeTable); i < len; ++i) {
 		_fadeTable[i] = 100;
 	}
@@ -390,11 +388,6 @@ static const uint8 gammaTables[GfxPalette32::numGammaTables][256] = {
 	loadPalette(999);
 }
 
-GfxPalette32::~GfxPalette32() {
-	varyOff();
-	cycleAllOff();
-}
-
 bool GfxPalette32::loadPalette(const GuiResourceId resourceId) {
 	Resource *palResource = _resMan->findResource(ResourceId(kResourceTypePalette, resourceId), false);
 
@@ -604,16 +597,16 @@ void GfxPalette32::setVary(const Palette &target, const int16 percent, const int
 }
 
 void GfxPalette32::setVaryPercent(const int16 percent, const int32 ticks) {
-	if (_varyTargetPalette != nullptr) {
+	if (_varyTargetPalette) {
 		setVaryTime(percent, ticks);
 	}
 
-	// NOTE: SSCI had two additional parameters for this function to change the
+	// SSCI had two additional parameters for this function to change the
 	// `_varyFromColor`, but they were always hardcoded to be ignored
 }
 
 void GfxPalette32::setVaryTime(const int32 time) {
-	if (_varyTargetPalette != nullptr) {
+	if (_varyTargetPalette) {
 		setVaryTime(_varyTargetPercent, time);
 	}
 }
@@ -645,16 +638,8 @@ void GfxPalette32::varyOff() {
 	_varyFromColor = 0;
 	_varyToColor = 255;
 	_varyDirection = 0;
-
-	if (_varyTargetPalette != nullptr) {
-		delete _varyTargetPalette;
-		_varyTargetPalette = nullptr;
-	}
-
-	if (_varyStartPalette != nullptr) {
-		delete _varyStartPalette;
-		_varyStartPalette = nullptr;
-	}
+	_varyTargetPalette.reset();
+	_varyStartPalette.reset();
 }
 
 void GfxPalette32::varyPause() {
@@ -667,7 +652,7 @@ void GfxPalette32::varyOn() {
 		--_varyNumTimesPaused;
 	}
 
-	if (_varyTargetPalette != nullptr && _varyNumTimesPaused == 0) {
+	if (_varyTargetPalette && _varyNumTimesPaused == 0) {
 		if (_varyPercent != _varyTargetPercent && _varyTime != 0) {
 			_varyDirection = (_varyTargetPercent - _varyPercent > 0) ? 1 : -1;
 		} else {
@@ -677,28 +662,26 @@ void GfxPalette32::varyOn() {
 }
 
 void GfxPalette32::setTarget(const Palette &palette) {
-	delete _varyTargetPalette;
-	_varyTargetPalette = new Palette(palette);
+	_varyTargetPalette.reset(new Palette(palette));
 }
 
 void GfxPalette32::setStart(const Palette &palette) {
-	delete _varyStartPalette;
-	_varyStartPalette = new Palette(palette);
+	_varyStartPalette.reset(new Palette(palette));
 }
 
 void GfxPalette32::mergeStart(const Palette &palette) {
-	if (_varyStartPalette != nullptr) {
+	if (_varyStartPalette) {
 		mergePalette(*_varyStartPalette, palette);
 	} else {
-		_varyStartPalette = new Palette(palette);
+		_varyStartPalette.reset(new Palette(palette));
 	}
 }
 
 void GfxPalette32::mergeTarget(const Palette &palette) {
-	if (_varyTargetPalette != nullptr) {
+	if (_varyTargetPalette) {
 		mergePalette(*_varyTargetPalette, palette);
 	} else {
-		_varyTargetPalette = new Palette(palette);
+		_varyTargetPalette.reset(new Palette(palette));
 	}
 }
 
@@ -714,9 +697,9 @@ void GfxPalette32::applyVary() {
 		_varyPercent += _varyDirection;
 	}
 
-	if (_varyPercent == 0 || _varyTargetPalette == nullptr) {
+	if (_varyPercent == 0 || !_varyTargetPalette) {
 		for (int i = 0; i < ARRAYSIZE(_nextPalette.colors); ++i) {
-			if (_varyStartPalette != nullptr && i >= _varyFromColor && i <= _varyToColor) {
+			if (_varyStartPalette && i >= _varyFromColor && i <= _varyToColor) {
 				_nextPalette.colors[i] = _varyStartPalette->colors[i];
 			} else {
 				_nextPalette.colors[i] = _sourcePalette.colors[i];
@@ -728,7 +711,7 @@ void GfxPalette32::applyVary() {
 				Color targetColor = _varyTargetPalette->colors[i];
 				Color sourceColor;
 
-				if (_varyStartPalette != nullptr) {
+				if (_varyStartPalette) {
 					sourceColor = _varyStartPalette->colors[i];
 				} else {
 					sourceColor = _sourcePalette.colors[i];
@@ -816,27 +799,28 @@ void GfxPalette32::setCycle(const uint8 fromColor, const uint8 toColor, const in
 		clearCycleMap(fromColor, cycler->numColorsToCycle);
 	} else {
 		for (int i = 0; i < kNumCyclers; ++i) {
-			if (_cyclers[i] == nullptr) {
-				_cyclers[i] = cycler = new PalCycler;
+			if (!_cyclers[i]) {
+				cycler = new PalCycler;
+				_cyclers[i].reset(cycler);
 				break;
 			}
 		}
 	}
 
-	// If there are no free cycler slots, SCI engine overrides the first oldest
-	// cycler that it finds, where "oldest" is determined by the difference
-	// between the tick and now
+	// If there are no free cycler slots, SSCI overrides the first oldest cycler
+	// that it finds, where "oldest" is determined by the difference between the
+	// tick and now
 	if (cycler == nullptr) {
 		const uint32 now = g_sci->getTickCount();
 		uint32 minUpdateDelta = 0xFFFFFFFF;
 
 		for (int i = 0; i < kNumCyclers; ++i) {
-			PalCycler *const candidate = _cyclers[i];
+			PalCyclerOwner &candidate = _cyclers[i];
 
 			const uint32 updateDelta = now - candidate->lastUpdateTick;
 			if (updateDelta < minUpdateDelta) {
 				minUpdateDelta = updateDelta;
-				cycler = candidate;
+				cycler = candidate.get();
 			}
 		}
 
@@ -882,19 +866,18 @@ void GfxPalette32::cyclePause(const uint8 fromColor) {
 
 void GfxPalette32::cycleAllOn() {
 	for (int i = 0; i < kNumCyclers; ++i) {
-		PalCycler *const cycler = _cyclers[i];
-		if (cycler != nullptr && cycler->numTimesPaused > 0) {
+		PalCyclerOwner &cycler = _cyclers[i];
+		if (cycler && cycler->numTimesPaused > 0) {
 			--cycler->numTimesPaused;
 		}
 	}
 }
 
 void GfxPalette32::cycleAllPause() {
-	// NOTE: The original engine did not check for null pointers in the
-	// palette cyclers pointer array.
+	// SSCI did not check for null pointers in the palette cyclers pointer array
 	for (int i = 0; i < kNumCyclers; ++i) {
-		PalCycler *const cycler = _cyclers[i];
-		if (cycler != nullptr) {
+		PalCyclerOwner &cycler = _cyclers[i];
+		if (cycler) {
 			// This seems odd, because currentCycle is 0..numColorsPerCycle,
 			// but fromColor is 0..255. When applyAllCycles runs, the values
 			// end up back in range
@@ -905,8 +888,8 @@ void GfxPalette32::cycleAllPause() {
 	applyAllCycles();
 
 	for (int i = 0; i < kNumCyclers; ++i) {
-		PalCycler *const cycler = _cyclers[i];
-		if (cycler != nullptr) {
+		PalCyclerOwner &cycler = _cyclers[i];
+		if (cycler) {
 			++cycler->numTimesPaused;
 		}
 	}
@@ -914,11 +897,10 @@ void GfxPalette32::cycleAllPause() {
 
 void GfxPalette32::cycleOff(const uint8 fromColor) {
 	for (int i = 0; i < kNumCyclers; ++i) {
-		PalCycler *const cycler = _cyclers[i];
-		if (cycler != nullptr && cycler->fromColor == fromColor) {
+		PalCyclerOwner &cycler = _cyclers[i];
+		if (cycler && cycler->fromColor == fromColor) {
 			clearCycleMap(fromColor, cycler->numColorsToCycle);
-			delete cycler;
-			_cyclers[i] = nullptr;
+			_cyclers[i].reset();
 			break;
 		}
 	}
@@ -926,11 +908,10 @@ void GfxPalette32::cycleOff(const uint8 fromColor) {
 
 void GfxPalette32::cycleAllOff() {
 	for (int i = 0; i < kNumCyclers; ++i) {
-		PalCycler *const cycler = _cyclers[i];
-		if (cycler != nullptr) {
+		PalCyclerOwner &cycler = _cyclers[i];
+		if (cycler) {
 			clearCycleMap(cycler->fromColor, cycler->numColorsToCycle);
-			delete cycler;
-			_cyclers[i] = nullptr;
+			_cyclers[i].reset();
 		}
 	}
 }
@@ -969,9 +950,9 @@ void GfxPalette32::setCycleMap(const uint16 fromColor, const uint16 numColorsToS
 
 PalCycler *GfxPalette32::getCycler(const uint16 fromColor) {
 	for (int cyclerIndex = 0; cyclerIndex < kNumCyclers; ++cyclerIndex) {
-		PalCycler *cycler = _cyclers[cyclerIndex];
-		if (cycler != nullptr && cycler->fromColor == fromColor) {
-			return cycler;
+		PalCyclerOwner &cycler = _cyclers[cyclerIndex];
+		if (cycler && cycler->fromColor == fromColor) {
+			return cycler.get();
 		}
 	}
 
@@ -980,12 +961,12 @@ PalCycler *GfxPalette32::getCycler(const uint16 fromColor) {
 
 void GfxPalette32::applyAllCycles() {
 	Color paletteCopy[256];
-	memcpy(paletteCopy, _nextPalette.colors, sizeof(Color) * 256);
+	memcpy(paletteCopy, _nextPalette.colors, sizeof(paletteCopy));
 
 	for (int i = 0; i < kNumCyclers; ++i) {
-		PalCycler *const cycler = _cyclers[i];
-		if (cycler != nullptr) {
-			cycler->currentCycle = (((int) cycler->currentCycle) + 1) % cycler->numColorsToCycle;
+		PalCyclerOwner &cycler = _cyclers[i];
+		if (cycler) {
+			cycler->currentCycle = (cycler->currentCycle + 1) % cycler->numColorsToCycle;
 			for (int j = 0; j < cycler->numColorsToCycle; j++) {
 				_nextPalette.colors[cycler->fromColor + j] = paletteCopy[cycler->fromColor + (cycler->currentCycle + j) % cycler->numColorsToCycle];
 			}
@@ -995,12 +976,12 @@ void GfxPalette32::applyAllCycles() {
 
 void GfxPalette32::applyCycles() {
 	Color paletteCopy[256];
-	memcpy(paletteCopy, _nextPalette.colors, sizeof(Color) * 256);
+	memcpy(paletteCopy, _nextPalette.colors, sizeof(paletteCopy));
 
 	const uint32 now = g_sci->getTickCount();
 	for (int i = 0; i < kNumCyclers; ++i) {
-		PalCycler *const cycler = _cyclers[i];
-		if (cycler == nullptr) {
+		PalCyclerOwner &cycler = _cyclers[i];
+		if (!cycler) {
 			continue;
 		}
 
diff --git a/engines/sci/graphics/palette32.h b/engines/sci/graphics/palette32.h
index b8ba85eb4a..2d00b86501 100644
--- a/engines/sci/graphics/palette32.h
+++ b/engines/sci/graphics/palette32.h
@@ -23,6 +23,8 @@
 #ifndef SCI_GRAPHICS_PALETTE32_H
 #define SCI_GRAPHICS_PALETTE32_H
 
+#include "common/ptr.h"
+
 namespace Sci {
 
 #pragma mark HunkPalette
@@ -233,7 +235,6 @@ struct PalCycler {
 class GfxPalette32 {
 public:
 	GfxPalette32(ResourceManager *resMan);
-	~GfxPalette32();
 
 	void saveLoadWithSerializer(Common::Serializer &s);
 
@@ -440,13 +441,13 @@ private:
 	 * operation. If this palette is not specified, `_sourcePalette` is used
 	 * instead.
 	 */
-	Palette *_varyStartPalette;
+	Common::ScopedPtr<Palette> _varyStartPalette;
 
 	/**
 	 * An optional palette used to provide target colors for a palette vary
 	 * operation.
 	 */
-	Palette *_varyTargetPalette;
+	Common::ScopedPtr<Palette> _varyTargetPalette;
 
 	/**
 	 * The minimum palette index that has been varied from the source palette.
@@ -553,7 +554,8 @@ private:
 		kNumCyclers = 10
 	};
 
-	PalCycler *_cyclers[kNumCyclers];
+	typedef Common::ScopedPtr<PalCycler> PalCyclerOwner;
+	PalCyclerOwner _cyclers[kNumCyclers];
 
 	/**
 	 * Updates the `currentCycle` of the given `cycler` by `speed` entries.
@@ -564,8 +566,8 @@ private:
 	 * The cycle map is used to detect overlapping cyclers, and to avoid
 	 * remapping to palette entries that are being cycled.
 	 *
-	 * According to SCI engine code, when two cyclers overlap, a fatal error has
-	 * occurred and the engine will display an error and then exit.
+	 * According to SSCI, when two cyclers overlap, a fatal error has occurred
+	 * and the engine will display an error and then exit.
 	 *
 	 * The color remapping system avoids attempts to remap to palette entries
 	 * that are cycling because they won't be the expected color once the cycler