From 5cffa3891f48d64154f00571c8127ef2c601d6d5 Mon Sep 17 00:00:00 2001 From: Colin Snover Date: Thu, 5 Oct 2017 20:53:27 -0500 Subject: 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 --- engines/sci/engine/savegame.cpp | 40 ++++++------ engines/sci/graphics/palette32.cpp | 121 ++++++++++++++++--------------------- engines/sci/graphics/palette32.h | 14 +++-- 3 files changed, 78 insertions(+), 97 deletions(-) (limited to 'engines/sci') 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) { 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 &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 _varyStartPalette; /** * An optional palette used to provide target colors for a palette vary * operation. */ - Palette *_varyTargetPalette; + Common::ScopedPtr _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 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 -- cgit v1.2.3