Simplify BOB interface

Modify links so the offset to the images is represented there. Hence overlay indices from links array can be directly used
Also some refactoring:
- Use ImgDir to avoid bulk arguments or misplaced argument order
- Use named constants
- Translate most comments
- Use better variable and function names

Author: Flamefire

include/libsiedler2/ArchivItem_Bob.h

@@ -21,42 +21,50 @@
 
 #include "Archiv.h"
 #include "ArchivItem.h"
+#include "ImgDir.h"
 #include <cstdint>
 #include <iosfwd>
 #include <map>
 #include <vector>
 
 namespace libsiedler2 {
 class ArchivItem_Palette;
-}
+class ArchivItem_Bitmap_Player;
+} // namespace libsiedler2
 
 namespace libsiedler2 {
-/// Klasse für Bobfiles.
+
 class ArchivItem_Bob : public ArchivItem, public Archiv
 {
 public:
     ArchivItem_Bob();
-
     ~ArchivItem_Bob() override;
     RTTR_CLONEABLE(ArchivItem_Bob)
 
-    /// lädt die Bobdaten aus einer Datei.
+    /// Load BOB data from file
     int load(std::istream& file, const ArchivItem_Palette* palette);
 
-    /// schreibt die Bobdaten in eine Datei.
+    /// Write BOB data to file. TODO: Implement
     static int write(std::ostream& file, const ArchivItem_Palette* palette);
 
-    uint32_t getNumGoodImgs() const { return numGoodImgs; }
-    uint32_t getNumItems() const { return uint32_t(links.size()); }
-    uint16_t getLink(uint32_t idx) const { return links[idx]; };
+    uint32_t getNumOverlayImgs() const { return numOverlayImgs; }
+    uint16_t getNumLinks() const { return static_cast<uint16_t>(links.size()); }
+    uint16_t getOverlayIdx(unsigned idx) const { return links[idx]; };
+    uint16_t getOverlayIdx(unsigned overlayIdx, bool fat, ImgDir direction, unsigned animationstep) const
+    {
+        // Array [overlayIdx][animStep][fat][direction]: [35][8][2][6]
+        return getOverlayIdx(((overlayIdx * 8 + animationstep) * 2 + fat) * 6 + static_cast<unsigned>(direction));
+    }
+    ArchivItem_Bitmap_Player* getBody(bool fat, ImgDir direction, unsigned animationstep);
+    ArchivItem_Bitmap_Player* getOverlay(unsigned overlayIdx, bool fat, ImgDir direction, unsigned animationstep);
 
     /// Write the links in mapping format (TAB separated entries with # comments)
     void writeLinks(std::ostream& file) const;
-    static std::map<unsigned, uint16_t> readLinks(std::istream& file);
+    static std::map<uint16_t, uint16_t> readLinks(std::istream& file);
 
 protected:
-    uint16_t numGoodImgs;        /// Number of pictures for wares
-    std::vector<uint16_t> links; /// "Links" (Zugehörigkeiten der Bilder)
+    uint16_t numOverlayImgs;     /// Number of actual overlay pictures (e.g. carried wares)
+    std::vector<uint16_t> links; /// Array [overlayId][animStep=8][fat=2][direction=6] mapping to an overlay picture
 };
 } // namespace libsiedler2
 

include/libsiedler2/ImgDir.h

@@ -0,0 +1,35 @@
+// Copyright (c) 2005 - 2020 Settlers Freaks (sf-team at siedler25.org)
+//
+// This file is part of Return To The Roots.
+//
+// Return To The Roots is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 2 of the License, or
+// (at your option) any later version.
+//
+// Return To The Roots is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Return To The Roots. If not, see <http://www.gnu.org/licenses/>.
+
+#ifndef libsiedler2_imgdir_h__
+#define libsiedler2_imgdir_h__
+
+namespace libsiedler2 {
+/// Direction that image figures are facing/walking
+enum class ImgDir
+{
+    // Order as in S2 arrays: Right first, then clockwise
+    E,
+    SE,
+    SW,
+    W,
+    NW,
+    NE
+};
+} // namespace libsiedler2
+
+#endif // libsiedler2_imgdir_h__

src/ArchivItem_Bob.cpp

@@ -26,6 +26,18 @@
 #include <boost/range/adaptor/indexed.hpp>
 #include <iostream>
 
+namespace {
+constexpr uint16_t COLOR_BLOCK_HEADER = 0x01F5;
+constexpr uint16_t IMAGE_DATA_HEADER = 0x01F4;
+/// 8 Animation Steps, 6 directions, 2 types (Fat, Non-Fat) --> Array [fat][direction][animStep]
+constexpr unsigned NUM_BODY_IMAGES = 2 * 6 * 8;
+/// Same for links but Array [animStep][fat][direction]
+constexpr unsigned NUM_LINKS_PER_OVERLAY = 8 * 2 * 6;
+constexpr uint16_t SPRITE_WIDTH = 32;
+/// Draw offset in X
+constexpr uint16_t X_OFFSET = 16;
+} // namespace
+
 namespace libsiedler2 {
 /// Read a block of colors used later
 static int readColorBlock(libendian::EndianIStreamAdapter<false, std::istream&>& fs, std::vector<uint8_t>& pixels)
@@ -34,7 +46,7 @@ static int readColorBlock(libendian::EndianIStreamAdapter<false, std::istream&>&
     if(!(fs >> id >> size))
         return ErrorCode::UNEXPECTED_EOF;
 
-    if(id != 0x01F5)
+    if(id != COLOR_BLOCK_HEADER)
         return ErrorCode::WRONG_FORMAT;
 
     pixels.resize(size);
@@ -50,7 +62,7 @@ static int readImageData(libendian::EndianIStreamAdapter<false, std::istream&>&
     if(!(fs >> id >> height))
         return ErrorCode::UNEXPECTED_EOF;
 
-    if(id != 0x01F4)
+    if(id != IMAGE_DATA_HEADER)
         return ErrorCode::WRONG_FORMAT;
 
     starts.resize(height);
@@ -59,12 +71,7 @@ static int readImageData(libendian::EndianIStreamAdapter<false, std::istream&>&
     return ErrorCode::NONE;
 }
 
-/** @class ArchivItem_Bob
- *
- *  Klasse für Bobfiles.
- */
-
-ArchivItem_Bob::ArchivItem_Bob() : ArchivItem(BobType::Bob), numGoodImgs(0) {}
+ArchivItem_Bob::ArchivItem_Bob() : ArchivItem(BobType::Bob), numOverlayImgs(0) {}
 
 ArchivItem_Bob::~ArchivItem_Bob() = default;
 
@@ -90,9 +97,10 @@ int ArchivItem_Bob::load(std::istream& file, const ArchivItem_Palette* palette)
     if(int ec = readColorBlock(fs, raw_base))
         return ec;
 
-    // Einzelner Bilder auslesen ( untere Körper ): 8 Animation Steps, 6 directions, 2 types (Fat, Non-Fat) = 96
-    alloc(96);
-    for(uint32_t i = 0; i < 96; ++i)
+    // Read body images (to get full figure draw this then draw the item image (below) over it)
+    // They form an array [fat][direction][animStep]
+    alloc(NUM_BODY_IMAGES);
+    for(uint32_t i = 0; i < NUM_BODY_IMAGES; ++i)
     {
         std::vector<uint16_t> starts;
         uint8_t ny;
@@ -101,17 +109,17 @@ int ArchivItem_Bob::load(std::istream& file, const ArchivItem_Palette* palette)
 
         auto image = getAllocator().create<ArchivItem_Bitmap_Player>(BobType::BitmapPlayer);
         assert(image);
-        image->setNx(16); //-V522
+        image->setNx(X_OFFSET);
         image->setNy(ny);
 
-        int ec = image->load(32, raw_base, starts, true, palette);
+        int ec = image->load(SPRITE_WIDTH, raw_base, starts, true, palette);
         if(ec)
             return ec;
 
         set(i, std::move(image));
     }
 
-    // erstmal die 6 Farbblöcke fr die 6 Richtungen
+    // Color blocks for each direction
     std::array<std::vector<uint8_t>, 6> raw;
 
     for(auto& i : raw)
@@ -120,55 +128,58 @@ int ArchivItem_Bob::load(std::istream& file, const ArchivItem_Palette* palette)
             return ec;
     }
 
-    // Anzahl Warenbilder
-    fs >> numGoodImgs;
+    // Number of overlay images (e.g. goods of carriers)
+    fs >> numOverlayImgs;
 
-    alloc_inc(numGoodImgs);
+    alloc_inc(numOverlayImgs);
 
-    std::vector<std::vector<uint16_t>> starts(numGoodImgs);
-    std::vector<uint8_t> ny(numGoodImgs);
+    std::vector<std::vector<uint16_t>> starts(numOverlayImgs);
+    std::vector<uint8_t> ny(numOverlayImgs);
 
-    for(uint16_t i = 0; i < numGoodImgs; ++i)
+    for(uint16_t i = 0; i < numOverlayImgs; ++i)
     {
         if(int ec = readImageData(fs, starts[i], ny[i]))
             return ec;
     }
 
     // Number of complete pictures.
-    // Links form an array: [ware][animStep][fat][direction]: [][8][2][6]
-    // the item at position 96 + link[ware][animStep][fat][direction] shall be combined
+    // Links form an array: [overlay][animStep][fat][direction]: [][8][2][6]
+    // the item at position NUM_BODY_IMAGES + link[overlay][animStep][fat][direction] shall be combined
     // with the appropriate body
-    uint16_t item_count;
-    if(!(fs >> item_count))
+    uint16_t numLinks;
+    if(!(fs >> numLinks))
         return ErrorCode::UNEXPECTED_EOF;
 
-    links.resize(item_count);
-    std::vector<bool> loaded(numGoodImgs, false);
+    links.resize(numLinks);
+    std::vector<bool> loaded(numOverlayImgs, false);
 
-    for(uint32_t i = 0; i < item_count; ++i)
+    for(uint32_t i = 0; i < numLinks; ++i)
     {
         uint16_t unknown;
         if(!(fs >> links[i] >> unknown))
             return ErrorCode::UNEXPECTED_EOF;
 
-        if(links[i] >= numGoodImgs)
+        if(links[i] >= numOverlayImgs)
             return ErrorCode::WRONG_FORMAT;
 
         if(loaded[links[i]])
             continue;
 
         auto image = getAllocator().create<ArchivItem_Bitmap_Player>(BobType::BitmapPlayer);
         assert(image);
-        image->setNx(16); //-V522
+        image->setNx(X_OFFSET);
         image->setNy(ny[links[i]]);
 
-        int ec = image->load(32, raw[i % 6], starts[links[i]], true, palette);
+        int ec = image->load(SPRITE_WIDTH, raw[i % 6], starts[links[i]], true, palette);
         if(ec)
             return ec;
 
-        set(96 + links[i], std::move(image));
+        set(NUM_BODY_IMAGES + links[i], std::move(image));
         loaded[links[i]] = true;
     }
+    // Adjust links so they point to actual indices in the archive as we moved them by NUM_BODY_IMAGES
+    for(auto& link : links)
+        link += NUM_BODY_IMAGES;
 
     return (!fs) ? ErrorCode::UNEXPECTED_EOF : ErrorCode::NONE;
 }
@@ -186,21 +197,37 @@ int ArchivItem_Bob::write(std::ostream&, const ArchivItem_Palette*)
     return ErrorCode::UNSUPPORTED_FORMAT;
 }
 
+ArchivItem_Bitmap_Player* ArchivItem_Bob::getBody(bool fat, ImgDir direction, unsigned animationstep)
+{
+    // Array: [fat][direction][animStep]: [2][6][8]
+    const unsigned bodyIdx = (fat * 6 + static_cast<unsigned>(direction)) * 8 + animationstep;
+    return dynamic_cast<ArchivItem_Bitmap_Player*>(get(bodyIdx));
+}
+
+ArchivItem_Bitmap_Player* ArchivItem_Bob::getOverlay(unsigned overlayIdx, bool fat, ImgDir direction, unsigned animationstep)
+{
+    return dynamic_cast<ArchivItem_Bitmap_Player*>(get(getOverlayIdx(overlayIdx, fat, direction, animationstep)));
+}
+
 void ArchivItem_Bob::writeLinks(std::ostream& file) const
 {
     // links[][8][2][6]
     for(const auto it : links | boost::adaptors::indexed())
     {
-        if(it.index() % (8 * 2 * 6) == 0)
-            file << "# Job ID " << it.index() / (8 * 2 * 6) << "\n";
+        if(it.index() % NUM_LINKS_PER_OVERLAY == 0)
+            file << "# Job ID " << it.index() / NUM_LINKS_PER_OVERLAY << "\n";
         file << s25util::toStringClassic(it.index()) << "\t" << s25util::toStringClassic(it.value()) << "\n";
     }
 }
 
-std::map<unsigned, uint16_t> ArchivItem_Bob::readLinks(std::istream& file)
+std::map<uint16_t, uint16_t> ArchivItem_Bob::readLinks(std::istream& file)
 {
-    std::map<unsigned, uint16_t> result;
-    loadMapping(file, [&result](unsigned idx, const std::string& value) { result[idx] = s25util::fromStringClassic<uint16_t>(value); });
+    std::map<uint16_t, uint16_t> result;
+    loadMapping(file, [&result](unsigned idx, const std::string& value) {
+        if(idx > std::numeric_limits<uint16_t>::max())
+            throw std::range_error("Index " + std::to_string(idx) + " is to large");
+        result[idx] = s25util::fromStringClassic<uint16_t>(value);
+    });
     return result;
 }
 

tests/testBob.cpp

@@ -34,31 +34,43 @@ BOOST_AUTO_TEST_CASE(LoadBobFile)
         return;
     const std::string inPath = libsiedler2::test::s2Path + "/DATA/BOBS/CARRIER.BOB";
     libsiedler2::Archiv archiv;
-    BOOST_REQUIRE_EQUAL(libsiedler2::Load(inPath, archiv, palette), 0);
-    BOOST_REQUIRE_EQUAL(archiv.size(), 1u);
+    BOOST_TEST_REQUIRE(libsiedler2::Load(inPath, archiv, palette) == 0);
+    BOOST_TEST_REQUIRE(archiv.size() == 1u);
     BOOST_TEST_REQUIRE(dynamic_cast<const libsiedler2::ArchivItem_Bob*>(archiv[0]));
-    const libsiedler2::ArchivItem_Bob& bob = *static_cast<const libsiedler2::ArchivItem_Bob*>(archiv[0]);
-    BOOST_REQUIRE_EQUAL(bob.getNumGoodImgs(), 602u);
-    BOOST_REQUIRE_EQUAL(bob.getNumItems(), 3264u);
-    BOOST_REQUIRE_EQUAL(bob.size(), 698u);
+    libsiedler2::ArchivItem_Bob& bob = *static_cast<libsiedler2::ArchivItem_Bob*>(archiv[0]);
+    BOOST_TEST(bob.getNumOverlayImgs() == 602u);
+    BOOST_TEST(bob.getNumLinks() == 3264u);
+    BOOST_TEST(bob.size() == 698u);
     for(unsigned i = 0; i < bob.size(); i++)
     {
         const auto* bmp = dynamic_cast<const libsiedler2::ArchivItem_Bitmap_Player*>(bob[i]);
-        BOOST_REQUIRE(bmp);
-        BOOST_REQUIRE_EQUAL(bmp->getNx(), 16);
-        BOOST_REQUIRE_EQUAL(bmp->getWidth(), 32u);
-        BOOST_REQUIRE_GT(bmp->getHeight(), 0u);
+        BOOST_TEST_REQUIRE(bmp);
+        BOOST_TEST(bmp->getNx() == 16);
+        BOOST_TEST(bmp->getWidth() == 32u);
+        BOOST_TEST(bmp->getHeight() > 0u);
     }
     // Some fixed tests
     const auto* bmp = dynamic_cast<const libsiedler2::ArchivItem_Bitmap_Player*>(bob[0]);
-    BOOST_REQUIRE_EQUAL(bmp->getNy(), 12);
-    BOOST_REQUIRE_EQUAL(bmp->getHeight(), 13u);
+    BOOST_TEST(bmp->getNy() == 12);
+    BOOST_TEST(bmp->getHeight() == 13u);
     bmp = dynamic_cast<const libsiedler2::ArchivItem_Bitmap_Player*>(bob[1]);
-    BOOST_REQUIRE_EQUAL(bmp->getNy(), 13);
-    BOOST_REQUIRE_EQUAL(bmp->getHeight(), 14u);
+    BOOST_TEST(bmp->getNy() == 13);
+    BOOST_TEST(bmp->getHeight() == 14u);
     bmp = dynamic_cast<const libsiedler2::ArchivItem_Bitmap_Player*>(bob[697]);
-    BOOST_REQUIRE_EQUAL(bmp->getNy(), 18);
-    BOOST_REQUIRE_EQUAL(bmp->getHeight(), 12u);
+    BOOST_TEST(bmp->getNy() == 18);
+    BOOST_TEST(bmp->getHeight() == 12u);
+
+    // Bodies are an array: array [fat][direction][animStep]: [2][6][8]
+    using libsiedler2::ImgDir;
+    BOOST_TEST(bob.getBody(true, ImgDir::W, 5));
+    BOOST_TEST(bob.getBody(true, ImgDir::W, 5) == bob[1 * 48 + 3 * 8 + 5]);
+    BOOST_TEST(bob.getBody(false, ImgDir::NW, 3));
+    BOOST_TEST(bob.getBody(false, ImgDir::NW, 3) == bob[0 * 48 + 4 * 8 + 3]);
+    // Overlay links are an array: [overlay][animStep][fat][direction]: [][8][2][6]
+    BOOST_TEST(bob.getOverlay(21, true, ImgDir::W, 5));
+    BOOST_TEST(bob.getOverlay(21, true, ImgDir::W, 5) == bob[bob.getOverlayIdx(21 * 96 + 5 * 12 + 3 + 1 * 6)]);
+    BOOST_TEST(bob.getOverlay(13, false, ImgDir::NW, 7));
+    BOOST_TEST(bob.getOverlay(13, false, ImgDir::NW, 7) == bob[bob.getOverlayIdx(13 * 96 + 7 * 12 + 4 + 0 * 6)]);
 }
 
 BOOST_AUTO_TEST_CASE(WriteReadLinks)
@@ -74,11 +86,11 @@ BOOST_AUTO_TEST_CASE(WriteReadLinks)
     bob->writeLinks(s);
     s.seekg(0);
     const auto links = libsiedler2::ArchivItem_Bob::readLinks(s);
-    for(unsigned i = 0; i < bob->getNumItems(); i++)
+    for(unsigned i = 0; i < bob->getNumLinks(); i++)
     {
         const auto it = links.find(i);
         BOOST_TEST_REQUIRE((it != links.end()));
-        BOOST_TEST_REQUIRE(it->second == bob->getLink(i));
+        BOOST_TEST_REQUIRE(it->second == bob->getOverlayIdx(i));
     }
 }