Merge branch 'ref_id_tests' into 'master'

Add more tests for RefId and fix ESM3ExteriorCellRefId

See merge request OpenMW/openmw!2905

apps/benchmarks/esm/benchrefid.cpp

@@ -82,6 +82,23 @@ namespace
         return generateSerializedRefIds(generateGeneratedRefIds(random), serialize);
     }
 
+    template <class Random>
+    std::vector<ESM::RefId> generateESM3ExteriorCellRefIds(Random& random)
+    {
+        std::vector<ESM::RefId> result;
+        result.reserve(refIdsCount);
+        std::uniform_int_distribution<std::int32_t> distribution(-100, 100);
+        std::generate_n(std::back_inserter(result), refIdsCount,
+            [&] { return ESM::ESM3ExteriorCellRefId(distribution(random), distribution(random)); });
+        return result;
+    }
+
+    template <class Random, class Serialize>
+    std::vector<std::string> generateSerializedESM3ExteriorCellRefIds(Random& random, Serialize&& serialize)
+    {
+        return generateSerializedRefIds(generateESM3ExteriorCellRefIds(random), serialize);
+    }
+
     void serializeRefId(benchmark::State& state)
     {
         std::minstd_rand random;
@@ -189,6 +206,33 @@ namespace
                 i = 0;
         }
     }
+
+    void serializeTextESM3ExteriorCellRefId(benchmark::State& state)
+    {
+        std::minstd_rand random;
+        std::vector<ESM::RefId> refIds = generateESM3ExteriorCellRefIds(random);
+        std::size_t i = 0;
+        for (auto _ : state)
+        {
+            benchmark::DoNotOptimize(refIds[i].serializeText());
+            if (++i >= refIds.size())
+                i = 0;
+        }
+    }
+
+    void deserializeTextESM3ExteriorCellRefId(benchmark::State& state)
+    {
+        std::minstd_rand random;
+        std::vector<std::string> serializedRefIds
+            = generateSerializedESM3ExteriorCellRefIds(random, [](ESM::RefId v) { return v.serializeText(); });
+        std::size_t i = 0;
+        for (auto _ : state)
+        {
+            benchmark::DoNotOptimize(ESM::RefId::deserializeText(serializedRefIds[i]));
+            if (++i >= serializedRefIds.size())
+                i = 0;
+        }
+    }
 }
 
 BENCHMARK(serializeRefId)->RangeMultiplier(4)->Range(8, 64);
@@ -199,5 +243,7 @@ BENCHMARK(serializeTextGeneratedRefId);
 BENCHMARK(deserializeTextGeneratedRefId);
 BENCHMARK(serializeTextIndexRefId);
 BENCHMARK(deserializeTextIndexRefId);
+BENCHMARK(serializeTextESM3ExteriorCellRefId);
+BENCHMARK(deserializeTextESM3ExteriorCellRefId);
 
 BENCHMARK_MAIN();

apps/openmw_test_suite/esm/testrefid.cpp

@@ -230,6 +230,7 @@ namespace ESM
             { RefId::formIdRefId(42), "0x2a" },
             { RefId::generated(42), "0x2a" },
             { RefId::index(REC_ARMO, 42), "ARMO:0x2a" },
+            { RefId::esm3ExteriorCell(-13, 42), "-13:42" },
         };
 
         INSTANTIATE_TEST_SUITE_P(ESMRefIdToString, ESMRefIdToStringTest, ValuesIn(toStringParams));
@@ -262,6 +263,7 @@ namespace ESM
             { RefId::formIdRefId(42), "FormId:0x2a" },
             { RefId::generated(42), "Generated:0x2a" },
             { RefId::index(REC_ARMO, 42), "Index:ARMO:0x2a" },
+            { RefId::esm3ExteriorCell(-13, 42), "Esm3ExteriorCell:-13:42" },
         };
 
         INSTANTIATE_TEST_SUITE_P(ESMRefIdToDebugString, ESMRefIdToDebugStringTest, ValuesIn(toDebugStringParams));
@@ -297,6 +299,13 @@ namespace ESM
             { RefId::index(REC_INGR, 1), "Index:INGR:0x1" },
             { RefId::index(REC_INGR, 0x1f), "Index:INGR:0x1f" },
             { RefId::index(REC_INGR, std::numeric_limits<std::uint32_t>::max()), "Index:INGR:0xffffffff" },
+            { RefId::esm3ExteriorCell(-13, 42), "Esm3ExteriorCell:-13:42" },
+            { RefId::esm3ExteriorCell(
+                  std::numeric_limits<std::int32_t>::min(), std::numeric_limits<std::int32_t>::min()),
+                "Esm3ExteriorCell:-2147483648:-2147483648" },
+            { RefId::esm3ExteriorCell(
+                  std::numeric_limits<std::int32_t>::max(), std::numeric_limits<std::int32_t>::max()),
+                "Esm3ExteriorCell:2147483647:2147483647" },
         };
 
         INSTANTIATE_TEST_SUITE_P(ESMRefIdText, ESMRefIdTextTest, ValuesIn(serializedRefIds));
@@ -364,7 +373,10 @@ namespace ESM
         TYPED_TEST_P(ESMRefIdTypesTest, serializeTextThenDeserializeTextShouldProduceSameValue)
         {
             const RefId refId = GenerateRefId<TypeParam>::call();
-            EXPECT_EQ(RefId::deserializeText(refId.serializeText()), refId);
+            const std::string text = refId.serializeText();
+            for (std::size_t i = 0; i < text.size(); ++i)
+                ASSERT_TRUE(std::isprint(text[i])) << "index: " << i << ", int value: " << static_cast<int>(text[i]);
+            EXPECT_EQ(RefId::deserializeText(text), refId);
         }
 
         TYPED_TEST_P(ESMRefIdTypesTest, shouldBeEqualToItself)

apps/openmw_test_suite/esm3/testesmwriter.cpp

@@ -50,5 +50,38 @@ namespace ESM
             writer.save(stream);
             EXPECT_THROW(writer.writeMaybeFixedSizeString(generateRandomString(33), 32), std::runtime_error);
         }
+
+        struct Esm3EsmWriterRefIdSizeTest : TestWithParam<std::pair<RefId, std::size_t>>
+        {
+        };
+
+        // If this test failed probably there is a change in RefId format and CurrentSaveGameFormatVersion should be
+        // incremented, current version should be handled.
+        TEST_P(Esm3EsmWriterRefIdSizeTest, writeHRefIdShouldProduceCertainNubmerOfBytes)
+        {
+            const auto [refId, size] = GetParam();
+
+            std::ostringstream stream;
+
+            {
+                ESMWriter writer;
+                writer.setFormatVersion(CurrentSaveGameFormatVersion);
+                writer.save(stream);
+                writer.writeHRefId(refId);
+            }
+
+            EXPECT_EQ(stream.view().size(), size);
+        }
+
+        const std::vector<std::pair<RefId, std::size_t>> refIdSizes = {
+            { RefId(), 57 },
+            { RefId::stringRefId(std::string(32, 'a')), 89 },
+            { RefId::formIdRefId(0x1f), 61 },
+            { RefId::generated(0x1f), 65 },
+            { RefId::index(REC_INGR, 0x1f), 65 },
+            { RefId::esm3ExteriorCell(-42, 42), 65 },
+        };
+
+        INSTANTIATE_TEST_SUITE_P(RefIds, Esm3EsmWriterRefIdSizeTest, ValuesIn(refIdSizes));
     }
 }

components/esm/esm3exteriorcellrefid.cpp

@@ -1,6 +1,7 @@
 #include "esm3exteriorcellrefid.hpp"
 #include "serializerefid.hpp"
 
+#include <limits>
 #include <ostream>
 #include <sstream>
 
@@ -9,28 +10,28 @@ namespace ESM
     std::string ESM3ExteriorCellRefId::toString() const
     {
         std::string result;
-        std::size_t integralSizeX = getIntegralSize(mX);
-        result.resize(integralSizeX + getIntegralSize(mY) + 3, '\0');
-        serializeIntegral(mX, 0, result);
-        result[integralSizeX] = ':';
-        serializeIntegral(mY, integralSizeX + 1, result);
+        result.resize(getDecIntegralCapacity(mX) + getDecIntegralCapacity(mY) + 3, '\0');
+        const std::size_t endX = serializeDecIntegral(mX, 0, result);
+        result[endX] = ':';
+        const std::size_t endY = serializeDecIntegral(mY, endX + 1, result);
+        result.resize(endY);
         return result;
     }
 
     std::string ESM3ExteriorCellRefId::toDebugString() const
     {
         std::string result;
-        std::size_t integralSizeX = getIntegralSize(mX);
-
-        serializeRefIdPrefix(integralSizeX + getIntegralSize(mY) + 1, esm3ExteriorCellRefIdPrefix, result);
-        serializeIntegral(mX, esm3ExteriorCellRefIdPrefix.size(), result);
-        result[esm3ExteriorCellRefIdPrefix.size() + integralSizeX] = ':';
-        serializeIntegral(mY, esm3ExteriorCellRefIdPrefix.size() + integralSizeX + 1, result);
+        serializeRefIdPrefix(
+            getDecIntegralCapacity(mX) + getDecIntegralCapacity(mY) + 1, esm3ExteriorCellRefIdPrefix, result);
+        const std::size_t endX = serializeDecIntegral(mX, esm3ExteriorCellRefIdPrefix.size(), result);
+        result[endX] = ':';
+        const std::size_t endY = serializeDecIntegral(mY, endX + 1, result);
+        result.resize(endY);
         return result;
     }
 
     std::ostream& operator<<(std::ostream& stream, ESM3ExteriorCellRefId value)
     {
-        return stream << "Vec2i{" << value.mX << "," << value.mY << '}';
+        return stream << value.toDebugString();
     }
 }

components/esm/formidrefid.cpp

@@ -9,8 +9,8 @@ namespace ESM
     std::string FormIdRefId::toString() const
     {
         std::string result;
-        result.resize(getIntegralSize(mValue) + 2, '\0');
-        serializeIntegral(mValue, 0, result);
+        result.resize(getHexIntegralSize(mValue) + 2, '\0');
+        serializeHexIntegral(mValue, 0, result);
         return result;
     }
 

components/esm/generatedrefid.cpp

@@ -9,8 +9,8 @@ namespace ESM
     std::string GeneratedRefId::toString() const
     {
         std::string result;
-        result.resize(getIntegralSize(mValue) + 2, '\0');
-        serializeIntegral(mValue, 0, result);
+        result.resize(getHexIntegralSize(mValue) + 2, '\0');
+        serializeHexIntegral(mValue, 0, result);
         return result;
     }
 

components/esm/indexrefid.cpp

@@ -9,20 +9,20 @@ namespace ESM
     std::string IndexRefId::toString() const
     {
         std::string result;
-        result.resize(sizeof(mRecordType) + getIntegralSize(mValue) + 3, '\0');
+        result.resize(sizeof(mRecordType) + getHexIntegralSize(mValue) + 3, '\0');
         std::memcpy(result.data(), &mRecordType, sizeof(mRecordType));
         result[sizeof(mRecordType)] = ':';
-        serializeIntegral(mValue, sizeof(mRecordType) + 1, result);
+        serializeHexIntegral(mValue, sizeof(mRecordType) + 1, result);
         return result;
     }
 
     std::string IndexRefId::toDebugString() const
     {
         std::string result;
-        serializeRefIdPrefix(sizeof(mRecordType) + getIntegralSize(mValue) + 1, indexRefIdPrefix, result);
+        serializeRefIdPrefix(sizeof(mRecordType) + getHexIntegralSize(mValue) + 1, indexRefIdPrefix, result);
         std::memcpy(result.data() + indexRefIdPrefix.size(), &mRecordType, sizeof(mRecordType));
         result[indexRefIdPrefix.size() + sizeof(mRecordType)] = ':';
-        serializeIntegral(mValue, indexRefIdPrefix.size() + sizeof(mRecordType) + 1, result);
+        serializeHexIntegral(mValue, indexRefIdPrefix.size() + sizeof(mRecordType) + 1, result);
         return result;
     }
 

components/esm/refid.cpp

@@ -225,23 +225,29 @@ namespace ESM
             return ESM::RefId();
 
         if (value.starts_with(formIdRefIdPrefix))
-            return ESM::RefId::formIdRefId(deserializeIntegral<ESM4::FormId>(formIdRefIdPrefix.size(), value));
+            return ESM::RefId::formIdRefId(deserializeHexIntegral<ESM4::FormId>(formIdRefIdPrefix.size(), value));
 
         if (value.starts_with(generatedRefIdPrefix))
-            return ESM::RefId::generated(deserializeIntegral<std::uint64_t>(generatedRefIdPrefix.size(), value));
+            return ESM::RefId::generated(deserializeHexIntegral<std::uint64_t>(generatedRefIdPrefix.size(), value));
 
         if (value.starts_with(indexRefIdPrefix))
         {
             ESM::RecNameInts recordType{};
             std::memcpy(&recordType, value.data() + indexRefIdPrefix.size(), sizeof(recordType));
             return ESM::RefId::index(recordType,
-                deserializeIntegral<std::uint32_t>(indexRefIdPrefix.size() + sizeof(recordType) + 1, value));
+                deserializeHexIntegral<std::uint32_t>(indexRefIdPrefix.size() + sizeof(recordType) + 1, value));
         }
+
         if (value.starts_with(esm3ExteriorCellRefIdPrefix))
         {
-            std::int32_t x = deserializeIntegral<std::int32_t>(esm3ExteriorCellRefIdPrefix.size(), value);
-            std::int32_t y
-                = deserializeIntegral<std::int32_t>(esm3ExteriorCellRefIdPrefix.size() + getIntegralSize(x) + 1, value);
+            if (value.size() < esm3ExteriorCellRefIdPrefix.size() + 3)
+                throw std::runtime_error("Invalid ESM3ExteriorCellRefId format: not enough size");
+            const std::size_t separator = value.find(':', esm3ExteriorCellRefIdPrefix.size() + 1);
+            if (separator == std::string_view::npos)
+                throw std::runtime_error("Invalid ESM3ExteriorCellRefId format: coordinates separator is not found");
+            const std::int32_t x
+                = deserializeDecIntegral<std::int32_t>(esm3ExteriorCellRefIdPrefix.size(), separator, value);
+            const std::int32_t y = deserializeDecIntegral<std::int32_t>(separator + 1, value.size(), value);
             return ESM::ESM3ExteriorCellRefId(x, y);
         }
 

components/esm/serializerefid.hpp

@@ -3,9 +3,11 @@
 
 #include <charconv>
 #include <cstring>
+#include <limits>
 #include <string>
 #include <string_view>
 #include <system_error>
+#include <type_traits>
 
 namespace ESM
 {
@@ -15,8 +17,19 @@ namespace ESM
     constexpr std::string_view esm3ExteriorCellRefIdPrefix = "Esm3ExteriorCell:";
 
     template <class T>
-    std::size_t getIntegralSize(T value)
+    std::size_t getDecIntegralCapacity(T value)
     {
+        if (value == 0)
+            return 1;
+        if (value > 0)
+            return static_cast<std::size_t>(std::numeric_limits<T>::digits10);
+        return static_cast<std::size_t>(std::numeric_limits<T>::digits10) + 1;
+    }
+
+    template <class T>
+    std::size_t getHexIntegralSize(T value)
+    {
+        static_assert(!std::is_signed_v<T>);
         std::size_t result = sizeof(T) * 2;
         while (true)
         {
@@ -34,30 +47,55 @@ namespace ESM
     }
 
     template <class T>
-    void serializeIntegral(T value, std::size_t shift, std::string& out)
+    std::size_t serializeDecIntegral(T value, std::size_t shift, std::string& out)
+    {
+        const auto r = std::to_chars(out.data() + shift, out.data() + out.size(), value, 10);
+        if (r.ec != std::errc())
+            throw std::system_error(std::make_error_code(r.ec), "Failed to serialize ESM::RefId dec integral value");
+        return r.ptr - out.data();
+    }
+
+    template <class T>
+    void serializeHexIntegral(T value, std::size_t shift, std::string& out)
     {
+        static_assert(!std::is_signed_v<T>);
         out[shift] = '0';
         out[shift + 1] = 'x';
         const auto r = std::to_chars(out.data() + shift + 2, out.data() + out.size(), value, 16);
         if (r.ec != std::errc())
-            throw std::system_error(std::make_error_code(r.ec), "Failed to serialize ESM::RefId integral value");
+            throw std::system_error(std::make_error_code(r.ec), "Failed to serialize ESM::RefId hex integral value");
     }
 
     template <class T>
     void serializeRefIdValue(T value, std::string_view prefix, std::string& out)
     {
-        serializeRefIdPrefix(getIntegralSize(value), prefix, out);
-        serializeIntegral(value, prefix.size(), out);
+        static_assert(!std::is_signed_v<T>);
+        serializeRefIdPrefix(getHexIntegralSize(value), prefix, out);
+        serializeHexIntegral(value, prefix.size(), out);
+    }
+
+    template <class T>
+    T deserializeDecIntegral(std::size_t shift, std::size_t end, std::string_view value)
+    {
+        T result{};
+        const auto r = std::from_chars(value.data() + shift, value.data() + end, result, 10);
+        if (r.ec != std::errc())
+            throw std::system_error(std::make_error_code(r.ec),
+                "Failed to deserialize ESM::RefId dec integral value: \""
+                    + std::string(value.data() + shift, value.data() + end) + '"');
+        return result;
     }
 
     template <class T>
-    T deserializeIntegral(std::size_t shift, std::string_view value)
+    T deserializeHexIntegral(std::size_t shift, std::string_view value)
     {
+        static_assert(!std::is_signed_v<T>);
         T result{};
         const auto r = std::from_chars(value.data() + shift + 2, value.data() + value.size(), result, 16);
         if (r.ec != std::errc())
             throw std::system_error(std::make_error_code(r.ec),
-                "Failed to deserialize ESM::RefId integral value: \"" + std::string(value) + '"');
+                "Failed to deserialize ESM::RefId hex integral value: \""
+                    + std::string(value.data() + shift + 2, value.data() + value.size()) + '"');
         return result;
     }
 }