Address review feedback

Author: morganchristiansson

CIO/CFile.cpp

@@ -348,13 +348,13 @@ bool CFile::read_lbm(FILE* fp, const boost::filesystem::path& filepath)
     // skip unknown data (length - 20 x 1 Byte)
     // fseek(fp, length-20, SEEK_CUR);
 
+    const Uint16 tilesetSlot = static_cast<Uint16>(bmpArray - global::bmpArray.data());
+    int chunkIdx = 0;
+
     /* READ SECOND CHUNK "CMAP" */
 
     // chunk-identifier (4 Bytes)
     // search for the "CMAP" and skip other chunk-types
-    const Uint16 tilesetSlot = static_cast<Uint16>(bmpArray - global::bmpArray.data());
-    int chunkIdx = 0;
-
     while(!feof(fp))
     {
         CHECK_READ(libendian::read(chunk_identifier.data(), 4, fp));
@@ -401,24 +401,28 @@ bool CFile::read_lbm(FILE* fp, const boost::filesystem::path& filepath)
             CHECK_READ(libendian::be_read_ui(&chunkLen, fp));
             if(chunkLen >= 8)
             {
-                PaletteAnimation anim;
                 uint16_t padding, rate, flags;
                 CHECK_READ(libendian::be_read_us(&padding, fp));
                 CHECK_READ(libendian::be_read_us(&rate, fp));
                 CHECK_READ(libendian::be_read_us(&flags, fp));
                 uint8_t firstClr, lastClr;
                 CHECK_READ(libendian::read(&firstClr, 1, fp));
                 CHECK_READ(libendian::read(&lastClr, 1, fp));
-                anim.isActive = (flags & 1) != 0;
-                anim.moveUp = (flags & 2) != 0;
-                if(rate)
-                    anim.isActive = anim.moveUp = true;
-                anim.rate = rate;
-                anim.firstClr = firstClr;
-                anim.lastClr = lastClr;
-                anim.currentOffset = 0;
-                anim.lastUpdateTime = SDL_GetTicks();
-                global::paletteAnimations[tilesetSlot][chunkIdx] = anim;
+                // Only register valid animations: at least 2 colors and non-zero rate
+                if(rate > 0 && lastClr > firstClr)
+                {
+                    PaletteAnimation anim;
+                    anim.moveUp = (flags & 2) != 0;
+                    if(rate)
+                        anim.moveUp = true;
+                    anim.rate = rate;
+                    anim.firstClr = firstClr;
+                    anim.lastClr = lastClr;
+                    anim.currentOffset = 0;
+                    anim.lastAppliedOffset = 0;
+                    anim.lastUpdateTime = SDL_GetTicks();
+                    global::paletteAnimations[tilesetSlot][chunkIdx] = anim;
+                }
                 if(chunkLen > 8)
                 {
                     uint32_t remaining = chunkLen - 8;

CSurface.cpp

@@ -22,7 +22,7 @@
 // locking. Was originally called in CGame::Init().
 static bool sgeLockOff = (sge_Lock_OFF(), true);
 
-static Uint16 tilesetIdxForMapType(MapType mapType, bool want32bit);
+static unsigned tilesetIdxForMapType(MapType mapType, bool want32bit);
 
 namespace {
 const TerrainDesc* getTerrainDesc(const bobMAP& map, Uint8 rawTextureId)
@@ -1512,7 +1512,7 @@ float CSurface::absf(float a)
 }
 
 /// Map a MapType to the 8-bit or 32-bit tileset bmpArray slot
-static Uint16 tilesetIdxForMapType(MapType mapType, bool want32bit)
+static unsigned tilesetIdxForMapType(MapType mapType, bool want32bit)
 {
     switch(mapType)
     {
@@ -1538,8 +1538,8 @@ static void rotatePaletteRange(SDL_Palette* pal, uint8_t firstClr, int colorCoun
 
 void CSurface::UpdatePaletteAnimations(MapType mapType)
 {
-    const Uint16 tilesetIdx8 = tilesetIdxForMapType(mapType, false);
-    auto animIt = global::paletteAnimations.find(tilesetIdx8);
+    const auto tilesetIdx8 = tilesetIdxForMapType(mapType, false);
+    auto animIt = global::paletteAnimations.find(static_cast<Uint16>(tilesetIdx8));
     if(animIt == global::paletteAnimations.end() || animIt->second.empty())
         return;
 
@@ -1552,15 +1552,15 @@ void CSurface::UpdatePaletteAnimations(MapType mapType)
       (global::s2 && global::s2->getMapObj()) ? global::s2->getMapObj()->getSurfacePalette() : nullptr;
 
     // 32-bit tileset surface for blitting after palette updates
-    const Uint16 tilesetIdx32 = tilesetIdxForMapType(mapType, true);
-    auto* surf32 = global::bmpArray[tilesetIdx32].surface.get();
+    const auto tilesetIdx32 = tilesetIdxForMapType(mapType, true);
+    auto& surf32 = *global::bmpArray[tilesetIdx32].surface;
 
-    const Uint32 now = SDL_GetTicks();
+    const unsigned now = SDL_GetTicks();
     bool anyUpdate = false;
 
     for(auto& [palAnimIdx, anim] : animMap)
     {
-        if(!anim.isActive || anim.rate == 0)
+        if(anim.rate == 0)
             continue;
 
         const int colorCount = anim.lastClr - anim.firstClr + 1;
@@ -1570,12 +1570,12 @@ void CSurface::UpdatePaletteAnimations(MapType mapType)
         // Time per step: (8192/30) / rate seconds, converted to ms
         const float intervalMs = (8192.0f / 30.0f) * 1000.0f / anim.rate;
 
-        const uint32_t elapsed = now - anim.lastUpdateTime;
-        if(elapsed < static_cast<uint32_t>(intervalMs))
+        const unsigned elapsed = now - anim.lastUpdateTime;
+        if(elapsed < static_cast<unsigned>(intervalMs))
             continue;
 
         int steps = static_cast<int>(elapsed / intervalMs);
-        anim.lastUpdateTime += static_cast<uint32_t>(steps * intervalMs);
+        anim.lastUpdateTime += static_cast<unsigned>(steps * intervalMs);
 
         int newOffset = anim.moveUp ? (anim.currentOffset + steps) % colorCount :
                                       (anim.currentOffset - steps + colorCount * steps) % colorCount;
@@ -1595,16 +1595,13 @@ void CSurface::UpdatePaletteAnimations(MapType mapType)
         return;
 
     // Blit 8-bit tileset to 32-bit so the rotated palette takes effect in 32bpp mode
-    if(surf32 && surf8)
-    {
-        if(SDL_MUSTLOCK(surf32))
-            SDL_LockSurface(surf32);
-        if(SDL_MUSTLOCK(surf8))
-            SDL_LockSurface(surf8);
-        SDL_BlitSurface(surf8, nullptr, surf32, nullptr);
-        if(SDL_MUSTLOCK(surf8))
-            SDL_UnlockSurface(surf8);
-        if(SDL_MUSTLOCK(surf32))
-            SDL_UnlockSurface(surf32);
-    }
+    if(SDL_MUSTLOCK(&surf32))
+        SDL_LockSurface(&surf32);
+    if(SDL_MUSTLOCK(surf8))
+        SDL_LockSurface(surf8);
+    SDL_BlitSurface(surf8, nullptr, &surf32, nullptr);
+    if(SDL_MUSTLOCK(surf8))
+        SDL_UnlockSurface(surf8);
+    if(SDL_MUSTLOCK(&surf32))
+        SDL_UnlockSurface(&surf32);
 }

globals.cpp

@@ -12,7 +12,7 @@ std::vector<bobBMP> global::bmpArray(MAXBOBBMP);
 std::vector<bobSHADOW> global::shadowArray(MAXBOBSHADOW);
 // array for all palettes
 std::vector<bobPAL> global::palArray(MAXBOBPAL);
-// Palette animation entries keyed by the bmpArray slot of the 8-bit tileset surface.
+// Palette animations keyed by (bmpArray slot of 8-bit tileset) -> (CRNG chunk index).
 std::map<Uint16, std::map<int, PaletteAnimation>> global::paletteAnimations;
 // the game object
 CGame* global::s2;

globals.h

@@ -24,8 +24,7 @@ extern std::vector<bobBMP> bmpArray;
 extern std::vector<bobSHADOW> shadowArray;
 // array for all palettes
 extern std::vector<bobPAL> palArray;
-// Palette animation entries keyed by the bmpArray slot of the 8-bit tileset surface.
-// Inner map keyed by the CRNG chunk's index in the tileset LBM file.
+// Palette animations keyed by (bmpArray slot of 8-bit tileset) -> (CRNG chunk index).
 extern std::map<Uint16, std::map<int, PaletteAnimation>> paletteAnimations;
 // the game object
 extern CGame* s2;

include/defines.h

@@ -94,14 +94,13 @@ struct bobPAL
 // Palette animation parsed from a CRNG chunk in an LBM file
 struct PaletteAnimation
 {
-    bool isActive = false;
-    bool moveUp = true;
-    uint16_t rate = 16384;
-    uint8_t firstClr = 0;
-    uint8_t lastClr = 0;
-    int currentOffset = 0;
-    int lastAppliedOffset = 0;
-    uint32_t lastUpdateTime = 0;
+    bool moveUp;
+    uint16_t rate;
+    uint8_t firstClr;
+    uint8_t lastClr;
+    int currentOffset;
+    int lastAppliedOffset;
+    unsigned lastUpdateTime;
 };
 
 // Structure for Bobtype 7 (Shadow-Bitmaps)