From 5b14ff44704488b886b89f482f3ceb5ebc57f869 Mon Sep 17 00:00:00 2001 From: elsid Date: Fri, 7 Apr 2023 20:59:46 +0200 Subject: [PATCH 1/3] Add tests to verify RefId size written to ESM file --- apps/openmw_test_suite/esm3/testesmwriter.cpp | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/apps/openmw_test_suite/esm3/testesmwriter.cpp b/apps/openmw_test_suite/esm3/testesmwriter.cpp index 23922d7461..6f14dbe112 100644 --- a/apps/openmw_test_suite/esm3/testesmwriter.cpp +++ b/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> + { + }; + + // 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> 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)); } } From 794050df63f1c7d0591aa0951f9fefc3cead2fe3 Mon Sep 17 00:00:00 2001 From: elsid Date: Fri, 7 Apr 2023 21:40:44 +0200 Subject: [PATCH 2/3] Fix and add tests for ESM3ExteriorCellRefId serialization and text representation --- apps/openmw_test_suite/esm/testrefid.cpp | 14 ++++++- components/esm/esm3exteriorcellrefid.cpp | 25 ++++++------ components/esm/formidrefid.cpp | 4 +- components/esm/generatedrefid.cpp | 4 +- components/esm/indexrefid.cpp | 8 ++-- components/esm/refid.cpp | 18 +++++--- components/esm/serializerefid.hpp | 52 ++++++++++++++++++++---- 7 files changed, 91 insertions(+), 34 deletions(-) diff --git a/apps/openmw_test_suite/esm/testrefid.cpp b/apps/openmw_test_suite/esm/testrefid.cpp index 6c7afb05cc..19aa0d5d13 100644 --- a/apps/openmw_test_suite/esm/testrefid.cpp +++ b/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::max()), "Index:INGR:0xffffffff" }, + { RefId::esm3ExteriorCell(-13, 42), "Esm3ExteriorCell:-13:42" }, + { RefId::esm3ExteriorCell( + std::numeric_limits::min(), std::numeric_limits::min()), + "Esm3ExteriorCell:-2147483648:-2147483648" }, + { RefId::esm3ExteriorCell( + std::numeric_limits::max(), std::numeric_limits::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::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(text[i]); + EXPECT_EQ(RefId::deserializeText(text), refId); } TYPED_TEST_P(ESMRefIdTypesTest, shouldBeEqualToItself) diff --git a/components/esm/esm3exteriorcellrefid.cpp b/components/esm/esm3exteriorcellrefid.cpp index 72b540cfdb..b29e6fdd0c 100644 --- a/components/esm/esm3exteriorcellrefid.cpp +++ b/components/esm/esm3exteriorcellrefid.cpp @@ -1,6 +1,7 @@ #include "esm3exteriorcellrefid.hpp" #include "serializerefid.hpp" +#include #include #include @@ -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(); } } diff --git a/components/esm/formidrefid.cpp b/components/esm/formidrefid.cpp index f4635f1a87..91790e503a 100644 --- a/components/esm/formidrefid.cpp +++ b/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; } diff --git a/components/esm/generatedrefid.cpp b/components/esm/generatedrefid.cpp index 03d9732107..b1b55f5143 100644 --- a/components/esm/generatedrefid.cpp +++ b/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; } diff --git a/components/esm/indexrefid.cpp b/components/esm/indexrefid.cpp index ee99f10e68..8c1c691281 100644 --- a/components/esm/indexrefid.cpp +++ b/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; } diff --git a/components/esm/refid.cpp b/components/esm/refid.cpp index 880b6da7e2..b24ef82cf6 100644 --- a/components/esm/refid.cpp +++ b/components/esm/refid.cpp @@ -225,23 +225,29 @@ namespace ESM return ESM::RefId(); if (value.starts_with(formIdRefIdPrefix)) - return ESM::RefId::formIdRefId(deserializeIntegral(formIdRefIdPrefix.size(), value)); + return ESM::RefId::formIdRefId(deserializeHexIntegral(formIdRefIdPrefix.size(), value)); if (value.starts_with(generatedRefIdPrefix)) - return ESM::RefId::generated(deserializeIntegral(generatedRefIdPrefix.size(), value)); + return ESM::RefId::generated(deserializeHexIntegral(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(indexRefIdPrefix.size() + sizeof(recordType) + 1, value)); + deserializeHexIntegral(indexRefIdPrefix.size() + sizeof(recordType) + 1, value)); } + if (value.starts_with(esm3ExteriorCellRefIdPrefix)) { - std::int32_t x = deserializeIntegral(esm3ExteriorCellRefIdPrefix.size(), value); - std::int32_t y - = deserializeIntegral(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(esm3ExteriorCellRefIdPrefix.size(), separator, value); + const std::int32_t y = deserializeDecIntegral(separator + 1, value.size(), value); return ESM::ESM3ExteriorCellRefId(x, y); } diff --git a/components/esm/serializerefid.hpp b/components/esm/serializerefid.hpp index d8c5fa982a..bc191b3553 100644 --- a/components/esm/serializerefid.hpp +++ b/components/esm/serializerefid.hpp @@ -3,9 +3,11 @@ #include #include +#include #include #include #include +#include namespace ESM { @@ -15,8 +17,19 @@ namespace ESM constexpr std::string_view esm3ExteriorCellRefIdPrefix = "Esm3ExteriorCell:"; template - std::size_t getIntegralSize(T value) + std::size_t getDecIntegralCapacity(T value) { + if (value == 0) + return 1; + if (value > 0) + return static_cast(std::numeric_limits::digits10); + return static_cast(std::numeric_limits::digits10) + 1; + } + + template + std::size_t getHexIntegralSize(T value) + { + static_assert(!std::is_signed_v); std::size_t result = sizeof(T) * 2; while (true) { @@ -34,30 +47,55 @@ namespace ESM } template - 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 + void serializeHexIntegral(T value, std::size_t shift, std::string& out) + { + static_assert(!std::is_signed_v); 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 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); + serializeRefIdPrefix(getHexIntegralSize(value), prefix, out); + serializeHexIntegral(value, prefix.size(), out); } template - T deserializeIntegral(std::size_t shift, std::string_view value) + 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 + T deserializeHexIntegral(std::size_t shift, std::string_view value) + { + static_assert(!std::is_signed_v); 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; } } From 740f409a095604b6a60023346d4afd11e0f76b06 Mon Sep 17 00:00:00 2001 From: elsid Date: Sat, 8 Apr 2023 01:07:13 +0200 Subject: [PATCH 3/3] Add benchmarks for ESM3ExteriorCellRefId serialization --- apps/benchmarks/esm/benchrefid.cpp | 46 ++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/apps/benchmarks/esm/benchrefid.cpp b/apps/benchmarks/esm/benchrefid.cpp index c2cd41e093..b12f494ab9 100644 --- a/apps/benchmarks/esm/benchrefid.cpp +++ b/apps/benchmarks/esm/benchrefid.cpp @@ -82,6 +82,23 @@ namespace return generateSerializedRefIds(generateGeneratedRefIds(random), serialize); } + template + std::vector generateESM3ExteriorCellRefIds(Random& random) + { + std::vector result; + result.reserve(refIdsCount); + std::uniform_int_distribution distribution(-100, 100); + std::generate_n(std::back_inserter(result), refIdsCount, + [&] { return ESM::ESM3ExteriorCellRefId(distribution(random), distribution(random)); }); + return result; + } + + template + std::vector 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 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 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();