Merge branch 'esm_signed_left' into 'master'

Use signed type for left record and files size in ESM3 reader context

See merge request OpenMW/openmw!2722

apps/openmw_test_suite/CMakeLists.txt

@@ -90,6 +90,7 @@ file(GLOB UNITTEST_SRC_FILES
     fx/technique.cpp
 
     esm3/readerscache.cpp
+    esm3/testsaveload.cpp
 
     nifosg/testnifloader.cpp
 )

apps/openmw_test_suite/esm/variant.cpp

@@ -1,3 +1,4 @@
+#include <components/esm/fourcc.hpp>
 #include <components/esm3/esmreader.hpp>
 #include <components/esm3/esmwriter.hpp>
 #include <components/esm3/variant.hpp>
@@ -12,6 +13,8 @@ namespace
     using namespace testing;
     using namespace ESM;
 
+    constexpr std::uint32_t fakeRecordId = fourCC("FAKE");
+
     Variant makeVariant(VarType type)
     {
         Variant v;
@@ -430,25 +433,28 @@ namespace
         std::ostringstream out;
         ESMWriter writer;
         writer.save(out);
+        writer.startRecord(fakeRecordId);
         variant.write(writer, format);
+        writer.endRecord(fakeRecordId);
         writer.close();
         return out.str();
     }
 
-    Variant read(const Variant::Format format, const std::string& data)
+    void read(const Variant::Format format, const std::string& data, Variant& result)
     {
-        Variant result;
         ESMReader reader;
-        reader.open(std::make_unique<std::istringstream>(data), "");
+        reader.open(std::make_unique<std::istringstream>(data), "stream");
+        ASSERT_TRUE(reader.hasMoreRecs());
+        ASSERT_EQ(reader.getRecName().toInt(), fakeRecordId);
+        reader.getRecHeader();
         result.read(reader, format);
-        return result;
     }
 
-    Variant writeAndRead(const Variant& variant, const Variant::Format format, std::size_t dataSize)
+    void writeAndRead(const Variant& variant, const Variant::Format format, std::size_t dataSize, Variant& result)
     {
         const std::string data = write(variant, format);
         EXPECT_EQ(data.size(), dataSize);
-        return read(format, data);
+        read(format, data, result);
     }
 
     struct ESMVariantToESMTest : TestWithParam<WriteToESMTestCase>
@@ -458,36 +464,27 @@ namespace
     TEST_P(ESMVariantToESMTest, deserialized_is_equal_to_serialized)
     {
         const auto param = GetParam();
-        const auto result = writeAndRead(param.mVariant, param.mFormat, param.mDataSize);
+        ESM::Variant result;
+        writeAndRead(param.mVariant, param.mFormat, param.mDataSize, result);
         ASSERT_EQ(param.mVariant, result);
     }
 
-    INSTANTIATE_TEST_SUITE_P(VariantAndData, ESMVariantToESMTest,
+    const std::array deserializedParams = {
-        Values(WriteToESMTestCase{ Variant(), Variant::Format_Gmst, 324 },
+        WriteToESMTestCase{ Variant(), Variant::Format_Gmst, 340 },
-            WriteToESMTestCase{ Variant(int{ 42 }), Variant::Format_Global, 345 },
+        WriteToESMTestCase{ Variant(int{ 42 }), Variant::Format_Global, 361 },
-            WriteToESMTestCase{ Variant(float{ 2.7f }), Variant::Format_Global, 345 },
+        WriteToESMTestCase{ Variant(float{ 2.7f }), Variant::Format_Global, 361 },
-            WriteToESMTestCase{ Variant(float{ 2.7f }), Variant::Format_Info, 336 },
+        WriteToESMTestCase{ Variant(float{ 2.7f }), Variant::Format_Info, 352 },
-            WriteToESMTestCase{ Variant(float{ 2.7f }), Variant::Format_Local, 336 },
+        WriteToESMTestCase{ Variant(float{ 2.7f }), Variant::Format_Local, 352 },
-            WriteToESMTestCase{ makeVariant(VT_Short, 42), Variant::Format_Global, 345 },
+        WriteToESMTestCase{ makeVariant(VT_Short, 42), Variant::Format_Global, 361 },
-            WriteToESMTestCase{ makeVariant(VT_Short, 42), Variant::Format_Local, 334 },
+        WriteToESMTestCase{ makeVariant(VT_Short, 42), Variant::Format_Local, 350 },
-            WriteToESMTestCase{ makeVariant(VT_Int, 42), Variant::Format_Info, 336 },
+        WriteToESMTestCase{ makeVariant(VT_Int, 42), Variant::Format_Info, 352 },
-            WriteToESMTestCase{ makeVariant(VT_Int, 42), Variant::Format_Local, 336 }));
+        WriteToESMTestCase{ makeVariant(VT_Int, 42), Variant::Format_Local, 352 },
-
+        WriteToESMTestCase{ Variant(float{ 2.7f }), Variant::Format_Gmst, 352 },
-    struct ESMVariantToESMNoneTest : TestWithParam<WriteToESMTestCase>
+        WriteToESMTestCase{ Variant(std::string("foo")), Variant::Format_Gmst, 351 },
-    {
+        WriteToESMTestCase{ makeVariant(VT_Int, 42), Variant::Format_Gmst, 352 },
     };
 
-    TEST_P(ESMVariantToESMNoneTest, deserialized_is_none)
+    INSTANTIATE_TEST_SUITE_P(VariantAndData, ESMVariantToESMTest, ValuesIn(deserializedParams));
-    {
-        const auto param = GetParam();
-        const auto result = writeAndRead(param.mVariant, param.mFormat, param.mDataSize);
-        ASSERT_EQ(Variant(), result);
-    }
-
-    INSTANTIATE_TEST_SUITE_P(VariantAndData, ESMVariantToESMNoneTest,
-        Values(WriteToESMTestCase{ Variant(float{ 2.7f }), Variant::Format_Gmst, 336 },
-            WriteToESMTestCase{ Variant(std::string("foo")), Variant::Format_Gmst, 335 },
-            WriteToESMTestCase{ makeVariant(VT_Int, 42), Variant::Format_Gmst, 336 }));
 
     struct ESMVariantWriteToESMFailTest : TestWithParam<WriteToESMTestCase>
     {

apps/openmw_test_suite/esm3/testsaveload.cpp

@@ -0,0 +1,133 @@
+#include <components/esm/fourcc.hpp>
+#include <components/esm3/esmreader.hpp>
+#include <components/esm3/esmwriter.hpp>
+#include <components/esm3/player.hpp>
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+#include <array>
+#include <memory>
+#include <random>
+
+namespace ESM
+{
+    namespace
+    {
+        using namespace ::testing;
+
+        constexpr std::array formats = {
+            CurrentSaveGameFormatVersion,
+        };
+
+        constexpr std::uint32_t fakeRecordId = fourCC("FAKE");
+
+        template <class T>
+        void save(const T& record, ESMWriter& writer)
+        {
+            record.save(writer);
+        }
+
+        void save(const CellRef& record, ESMWriter& writer)
+        {
+            record.save(writer, true);
+        }
+
+        template <typename T>
+        std::unique_ptr<std::istream> makeEsmStream(const T& record, FormatVersion formatVersion)
+        {
+            ESMWriter writer;
+            auto stream = std::make_unique<std::stringstream>();
+            writer.setFormatVersion(formatVersion);
+            writer.save(*stream);
+            writer.startRecord(fakeRecordId);
+            save(record, writer);
+            writer.endRecord(fakeRecordId);
+            return stream;
+        }
+
+        template <class T>
+        void load(ESMReader& reader, T& record)
+        {
+            record.load(reader);
+        }
+
+        void load(ESMReader& reader, CellRef& record)
+        {
+            bool deleted = false;
+            record.load(reader, deleted, true);
+        }
+
+        template <typename T>
+        void saveAndLoadRecord(const T& record, FormatVersion formatVersion, T& result)
+        {
+            ESMReader reader;
+            reader.open(makeEsmStream(record, formatVersion), "stream");
+            ASSERT_TRUE(reader.hasMoreRecs());
+            ASSERT_EQ(reader.getRecName().toInt(), fakeRecordId);
+            reader.getRecHeader();
+            load(reader, result);
+        }
+
+        struct Esm3SaveLoadRecordTest : public TestWithParam<FormatVersion>
+        {
+            std::minstd_rand mRandom;
+            std::uniform_int_distribution<short> mRefIdDistribution{ 'a', 'z' };
+
+            RefId generateRandomRefId(std::size_t size = 33)
+            {
+                std::string value;
+                while (value.size() < size)
+                    value.push_back(static_cast<char>(mRefIdDistribution(mRandom)));
+                return RefId::stringRefId(value);
+            }
+        };
+
+        TEST_P(Esm3SaveLoadRecordTest, playerShouldNotChange)
+        {
+            std::minstd_rand random;
+            Player record{};
+            record.mObject.blank();
+            record.mBirthsign = generateRandomRefId();
+            record.mObject.mRef.mRefID = generateRandomRefId();
+            std::generate_n(std::inserter(record.mPreviousItems, record.mPreviousItems.end()), 2,
+                [&] { return std::make_pair(generateRandomRefId(), generateRandomRefId()); });
+            Player result;
+            saveAndLoadRecord(record, GetParam(), result);
+            EXPECT_EQ(record.mBirthsign, result.mBirthsign);
+            EXPECT_EQ(record.mPreviousItems, result.mPreviousItems);
+        }
+
+        TEST_P(Esm3SaveLoadRecordTest, cellRefShouldNotChange)
+        {
+            CellRef record;
+            record.blank();
+            record.mRefID = generateRandomRefId();
+            record.mOwner = generateRandomRefId();
+            record.mSoul = generateRandomRefId();
+            record.mFaction = generateRandomRefId();
+            record.mKey = generateRandomRefId();
+            CellRef result;
+            saveAndLoadRecord(record, GetParam(), result);
+            EXPECT_EQ(record.mRefID, result.mRefID);
+            EXPECT_EQ(record.mOwner, result.mOwner);
+            EXPECT_EQ(record.mSoul, result.mSoul);
+            EXPECT_EQ(record.mFaction, result.mFaction);
+            EXPECT_EQ(record.mKey, result.mKey);
+        }
+
+        TEST_P(Esm3SaveLoadRecordTest, creatureStatsShouldNotChange)
+        {
+            CreatureStats record;
+            record.blank();
+            record.mLastHitAttemptObject = generateRandomRefId();
+            record.mLastHitObject = generateRandomRefId();
+            CreatureStats result;
+            saveAndLoadRecord(record, GetParam(), result);
+            EXPECT_EQ(record.mLastHitAttemptObject, result.mLastHitAttemptObject);
+            EXPECT_EQ(record.mLastHitObject, result.mLastHitObject);
+        }
+
+        INSTANTIATE_TEST_SUITE_P(FormatVersions, Esm3SaveLoadRecordTest, ValuesIn(formats));
+    }
+}

components/esm/esmcommon.hpp

@@ -188,8 +188,9 @@ namespace ESM
     struct ESM_Context
     {
         std::filesystem::path filename;
-        uint32_t leftRec, leftSub;
+        std::streamsize leftRec;
-        size_t leftFile;
+        std::uint32_t leftSub;
+        std::streamsize leftFile;
         NAME recName, subName;
         // When working with multiple esX files, we will generate lists of all files that
         //  actually contribute to a specific cell. Therefore, we need to store the index

components/esm3/cellref.cpp

@@ -169,7 +169,7 @@ namespace ESM
         else
         {
             if (isSet() && !hasContentFile())
-                Log(Debug::Error) << "Generated RefNum can not be saved in 32bit format";
+                throw std::runtime_error("Generated RefNum can not be saved in 32bit format");
             int refNum = (mIndex & 0xffffff) | ((hasContentFile() ? mContentFile : 0xff) << 24);
             esm.writeHNT(tag, refNum, 4);
         }

components/esm3/esmreader.cpp

@@ -297,16 +297,18 @@ namespace ESM
 
     void ESMReader::getSubHeader()
     {
-        if (mCtx.leftRec < sizeof(mCtx.leftSub))
+        if (mCtx.leftRec < static_cast<std::streamsize>(sizeof(mCtx.leftSub)))
-            fail("End of record while reading sub-record header");
+            fail("End of record while reading sub-record header: " + std::to_string(mCtx.leftRec) + " < "
+                + std::to_string(sizeof(mCtx.leftSub)));
 
         // Get subrecord size
-        getT(mCtx.leftSub);
+        getUint(mCtx.leftSub);
         mCtx.leftRec -= sizeof(mCtx.leftSub);
 
         // Adjust number of record bytes left
         if (mCtx.leftRec < mCtx.leftSub)
-            fail("Record size is larger than rest of file");
+            fail("Record size is larger than rest of file: " + std::to_string(mCtx.leftRec) + " < "
+                + std::to_string(mCtx.leftSub));
         mCtx.leftRec -= mCtx.leftSub;
     }
 
@@ -335,12 +337,14 @@ namespace ESM
     void ESMReader::getRecHeader(uint32_t& flags)
     {
         // General error checking
-        if (mCtx.leftFile < 3 * sizeof(uint32_t))
+        if (mCtx.leftFile < static_cast<std::streamsize>(3 * sizeof(uint32_t)))
             fail("End of file while reading record header");
         if (mCtx.leftRec)
             fail("Previous record contains unread bytes");
 
-        getUint(mCtx.leftRec);
+        std::uint32_t leftRec = 0;
+        getUint(leftRec);
+        mCtx.leftRec = static_cast<std::streamsize>(leftRec);
         getUint(flags); // This header entry is always zero
         getUint(flags);
         mCtx.leftFile -= 3 * sizeof(uint32_t);

components/esm3/esmreader.hpp

@@ -297,7 +297,7 @@ namespace ESM
     private:
         [[noreturn]] void reportSubSizeMismatch(size_t want, size_t got)
         {
-            fail("record size mismatch, requested " + std::to_string(want) + ", got" + std::to_string(got));
+            fail("record size mismatch, requested " + std::to_string(want) + ", got " + std::to_string(got));
         }
 
         void clearCtx();