From 1bfcfaff341f65666fb9d9f774f9baeae1e425ce Mon Sep 17 00:00:00 2001 From: elsid Date: Fri, 12 Jan 2024 03:35:57 +0100 Subject: [PATCH 1/3] Use proper naming for member variable --- components/esm3/aisequence.cpp | 2 +- components/esm3/aisequence.hpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/components/esm3/aisequence.cpp b/components/esm3/aisequence.cpp index 99c85db1bb..d5b15893bf 100644 --- a/components/esm3/aisequence.cpp +++ b/components/esm3/aisequence.cpp @@ -14,7 +14,7 @@ namespace ESM void AiWander::load(ESMReader& esm) { esm.getHNT("DATA", mData.mDistance, mData.mDuration, mData.mTimeOfDay, mData.mIdle, mData.mShouldRepeat); - esm.getHNT("STAR", mDurationData.mRemainingDuration, mDurationData.unused); // was mStartTime + esm.getHNT("STAR", mDurationData.mRemainingDuration, mDurationData.mUnused); // was mStartTime mStoredInitialActorPosition = esm.getHNOT("POS_", mInitialActorPosition.mValues); } diff --git a/components/esm3/aisequence.hpp b/components/esm3/aisequence.hpp index 107fdf3bdb..099e5560e1 100644 --- a/components/esm3/aisequence.hpp +++ b/components/esm3/aisequence.hpp @@ -48,7 +48,7 @@ namespace ESM struct AiWanderDuration { float mRemainingDuration; - int32_t unused; + int32_t mUnused; }; struct AiTravelData { From 3592dc4c8835d49a076007296714019a9ec83230 Mon Sep 17 00:00:00 2001 From: elsid Date: Fri, 12 Jan 2024 03:01:49 +0100 Subject: [PATCH 2/3] Add tests for saving and loading AiSequence::AiWander --- apps/openmw_test_suite/esm3/testsaveload.cpp | 82 ++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/apps/openmw_test_suite/esm3/testsaveload.cpp b/apps/openmw_test_suite/esm3/testsaveload.cpp index ff68d0d4f1..501a0b47c0 100644 --- a/apps/openmw_test_suite/esm3/testsaveload.cpp +++ b/apps/openmw_test_suite/esm3/testsaveload.cpp @@ -1,4 +1,5 @@ #include +#include #include #include #include @@ -410,6 +411,87 @@ namespace ESM EXPECT_EQ(result.mStringId, record.mStringId); } + TEST_P(Esm3SaveLoadRecordTest, aiSequenceAiWanderShouldNotChange) + { + AiSequence::AiWander record; + record.mData.mDistance = 1; + record.mData.mDuration = 2; + record.mData.mTimeOfDay = 3; + constexpr std::uint8_t idle[8] = { 4, 5, 6, 7, 8, 9, 10, 11 }; + static_assert(std::size(idle) == std::size(record.mData.mIdle)); + std::copy(std::begin(idle), std::end(idle), record.mData.mIdle); + record.mData.mShouldRepeat = 12; + record.mDurationData.mRemainingDuration = 13; + record.mDurationData.mUnused = 14; + record.mStoredInitialActorPosition = true; + constexpr float initialActorPosition[3] = { 15, 16, 17 }; + static_assert(std::size(initialActorPosition) == std::size(record.mInitialActorPosition.mValues)); + std::copy( + std::begin(initialActorPosition), std::end(initialActorPosition), record.mInitialActorPosition.mValues); + + AiSequence::AiWander result; + saveAndLoadRecord(record, GetParam(), result); + + EXPECT_EQ(result.mData.mDistance, record.mData.mDistance); + EXPECT_EQ(result.mData.mDuration, record.mData.mDuration); + EXPECT_EQ(result.mData.mTimeOfDay, record.mData.mTimeOfDay); + EXPECT_THAT(result.mData.mIdle, ElementsAreArray(record.mData.mIdle)); + EXPECT_EQ(result.mData.mShouldRepeat, record.mData.mShouldRepeat); + EXPECT_EQ(result.mDurationData.mRemainingDuration, record.mDurationData.mRemainingDuration); + EXPECT_EQ(result.mDurationData.mUnused, record.mDurationData.mUnused); + EXPECT_EQ(result.mStoredInitialActorPosition, record.mStoredInitialActorPosition); + EXPECT_THAT(result.mInitialActorPosition.mValues, ElementsAreArray(record.mInitialActorPosition.mValues)); + } + + TEST_P(Esm3SaveLoadRecordTest, aiSequenceAiTravelShouldNotChange) + { + AiSequence::AiTravel record; + record.mData.mX = 1; + record.mData.mY = 2; + record.mData.mZ = 3; + record.mHidden = true; + record.mRepeat = true; + + AiSequence::AiTravel result; + saveAndLoadRecord(record, GetParam(), result); + + EXPECT_EQ(result.mData.mX, record.mData.mX); + EXPECT_EQ(result.mData.mY, record.mData.mY); + EXPECT_EQ(result.mData.mZ, record.mData.mZ); + EXPECT_EQ(result.mHidden, record.mHidden); + EXPECT_EQ(result.mRepeat, record.mRepeat); + } + + TEST_P(Esm3SaveLoadRecordTest, aiSequenceAiEscortShouldNotChange) + { + AiSequence::AiEscort record; + record.mData.mX = 1; + record.mData.mY = 2; + record.mData.mZ = 3; + record.mData.mDuration = 4; + record.mTargetActorId = 5; + record.mTargetId = generateRandomRefId(32); + record.mCellId = generateRandomString(257); + record.mRemainingDuration = 6; + record.mRepeat = true; + + AiSequence::AiEscort result; + saveAndLoadRecord(record, GetParam(), result); + + EXPECT_EQ(result.mData.mX, record.mData.mX); + EXPECT_EQ(result.mData.mY, record.mData.mY); + EXPECT_EQ(result.mData.mZ, record.mData.mZ); + if (GetParam() <= MaxOldAiPackageFormatVersion) + EXPECT_EQ(result.mData.mDuration, record.mRemainingDuration); + else + EXPECT_EQ(result.mData.mDuration, record.mData.mDuration); + EXPECT_EQ(result.mTargetActorId, record.mTargetActorId); + EXPECT_EQ(result.mTargetId, record.mTargetId); + EXPECT_EQ(result.mCellId, record.mCellId); + EXPECT_EQ(result.mRemainingDuration, record.mRemainingDuration); + EXPECT_EQ(result.mRepeat, record.mRepeat); + } + INSTANTIATE_TEST_SUITE_P(FormatVersions, Esm3SaveLoadRecordTest, ValuesIn(getFormats())); } } From 645175089016127a252e2172ef5855d0dda499d5 Mon Sep 17 00:00:00 2001 From: elsid Date: Fri, 12 Jan 2024 02:15:54 +0100 Subject: [PATCH 3/3] Write AiSequence and Script data field by field via decompose function Use the same function to load and save to have single place with field order definition. Use concepts for overload over different types. --- apps/openmw_test_suite/esm3/testsaveload.cpp | 2 - components/esm/decompose.hpp | 10 ++++ components/esm3/aisequence.cpp | 50 +++++++++++++++----- components/esm3/aisequence.hpp | 13 +++-- components/esm3/esmreader.hpp | 12 +++++ components/esm3/esmwriter.hpp | 17 ++++++- components/esm3/loadscpt.cpp | 15 +++--- components/misc/concepts.hpp | 13 +++++ 8 files changed, 104 insertions(+), 28 deletions(-) create mode 100644 components/esm/decompose.hpp create mode 100644 components/misc/concepts.hpp diff --git a/apps/openmw_test_suite/esm3/testsaveload.cpp b/apps/openmw_test_suite/esm3/testsaveload.cpp index 501a0b47c0..f8ef23e887 100644 --- a/apps/openmw_test_suite/esm3/testsaveload.cpp +++ b/apps/openmw_test_suite/esm3/testsaveload.cpp @@ -422,7 +422,6 @@ namespace ESM std::copy(std::begin(idle), std::end(idle), record.mData.mIdle); record.mData.mShouldRepeat = 12; record.mDurationData.mRemainingDuration = 13; - record.mDurationData.mUnused = 14; record.mStoredInitialActorPosition = true; constexpr float initialActorPosition[3] = { 15, 16, 17 }; static_assert(std::size(initialActorPosition) == std::size(record.mInitialActorPosition.mValues)); @@ -438,7 +437,6 @@ namespace ESM EXPECT_THAT(result.mData.mIdle, ElementsAreArray(record.mData.mIdle)); EXPECT_EQ(result.mData.mShouldRepeat, record.mData.mShouldRepeat); EXPECT_EQ(result.mDurationData.mRemainingDuration, record.mDurationData.mRemainingDuration); - EXPECT_EQ(result.mDurationData.mUnused, record.mDurationData.mUnused); EXPECT_EQ(result.mStoredInitialActorPosition, record.mStoredInitialActorPosition); EXPECT_THAT(result.mInitialActorPosition.mValues, ElementsAreArray(record.mInitialActorPosition.mValues)); } diff --git a/components/esm/decompose.hpp b/components/esm/decompose.hpp new file mode 100644 index 0000000000..eb6f5070d4 --- /dev/null +++ b/components/esm/decompose.hpp @@ -0,0 +1,10 @@ +#ifndef OPENMW_COMPONENTS_ESM_DECOMPOSE_H +#define OPENMW_COMPONENTS_ESM_DECOMPOSE_H + +namespace ESM +{ + template + void decompose(T&& value, const auto& apply) = delete; +} + +#endif diff --git a/components/esm3/aisequence.cpp b/components/esm3/aisequence.cpp index d5b15893bf..c316c2db86 100644 --- a/components/esm3/aisequence.cpp +++ b/components/esm3/aisequence.cpp @@ -3,32 +3,58 @@ #include "esmreader.hpp" #include "esmwriter.hpp" +#include + #include #include namespace ESM { + template T> + void decompose(T&& v, const auto& f) + { + f(v.mDistance, v.mDuration, v.mTimeOfDay, v.mIdle, v.mShouldRepeat); + } + + template T> + void decompose(T&& v, const auto& f) + { + std::uint32_t unused = 0; + f(v.mRemainingDuration, unused); + } + + template T> + void decompose(T&& v, const auto& f) + { + f(v.mX, v.mY, v.mZ); + } + + template T> + void decompose(T&& v, const auto& f) + { + f(v.mX, v.mY, v.mZ, v.mDuration); + } + namespace AiSequence { - void AiWander::load(ESMReader& esm) { - esm.getHNT("DATA", mData.mDistance, mData.mDuration, mData.mTimeOfDay, mData.mIdle, mData.mShouldRepeat); - esm.getHNT("STAR", mDurationData.mRemainingDuration, mDurationData.mUnused); // was mStartTime + esm.getNamedComposite("DATA", mData); + esm.getNamedComposite("STAR", mDurationData); // was mStartTime mStoredInitialActorPosition = esm.getHNOT("POS_", mInitialActorPosition.mValues); } void AiWander::save(ESMWriter& esm) const { - esm.writeHNT("DATA", mData); - esm.writeHNT("STAR", mDurationData); + esm.writeNamedComposite("DATA", mData); + esm.writeNamedComposite("STAR", mDurationData); // was mStartTime if (mStoredInitialActorPosition) - esm.writeHNT("POS_", mInitialActorPosition); + esm.writeHNT("POS_", mInitialActorPosition.mValues); } void AiTravel::load(ESMReader& esm) { - esm.getHNT("DATA", mData.mX, mData.mY, mData.mZ); + esm.getNamedComposite("DATA", mData); esm.getHNT(mHidden, "HIDD"); mRepeat = false; esm.getHNOT(mRepeat, "REPT"); @@ -36,7 +62,7 @@ namespace ESM void AiTravel::save(ESMWriter& esm) const { - esm.writeHNT("DATA", mData); + esm.writeNamedComposite("DATA", mData); esm.writeHNT("HIDD", mHidden); if (mRepeat) esm.writeHNT("REPT", mRepeat); @@ -44,7 +70,7 @@ namespace ESM void AiEscort::load(ESMReader& esm) { - esm.getHNT("DATA", mData.mX, mData.mY, mData.mZ, mData.mDuration); + esm.getNamedComposite("DATA", mData); mTargetId = esm.getHNRefId("TARG"); mTargetActorId = -1; esm.getHNOT(mTargetActorId, "TAID"); @@ -64,7 +90,7 @@ namespace ESM void AiEscort::save(ESMWriter& esm) const { - esm.writeHNT("DATA", mData); + esm.writeNamedComposite("DATA", mData); esm.writeHNRefId("TARG", mTargetId); esm.writeHNT("TAID", mTargetActorId); esm.writeHNT("DURA", mRemainingDuration); @@ -76,7 +102,7 @@ namespace ESM void AiFollow::load(ESMReader& esm) { - esm.getHNT("DATA", mData.mX, mData.mY, mData.mZ, mData.mDuration); + esm.getNamedComposite("DATA", mData); mTargetId = esm.getHNRefId("TARG"); mTargetActorId = -1; esm.getHNOT(mTargetActorId, "TAID"); @@ -101,7 +127,7 @@ namespace ESM void AiFollow::save(ESMWriter& esm) const { - esm.writeHNT("DATA", mData); + esm.writeNamedComposite("DATA", mData); esm.writeHNRefId("TARG", mTargetId); esm.writeHNT("TAID", mTargetActorId); esm.writeHNT("DURA", mRemainingDuration); diff --git a/components/esm3/aisequence.hpp b/components/esm3/aisequence.hpp index 099e5560e1..d6d6259f6b 100644 --- a/components/esm3/aisequence.hpp +++ b/components/esm3/aisequence.hpp @@ -36,32 +36,31 @@ namespace ESM virtual ~AiPackage() {} }; -#pragma pack(push, 1) struct AiWanderData { int16_t mDistance; int16_t mDuration; - unsigned char mTimeOfDay; - unsigned char mIdle[8]; - unsigned char mShouldRepeat; + std::uint8_t mTimeOfDay; + std::uint8_t mIdle[8]; + std::uint8_t mShouldRepeat; }; + struct AiWanderDuration { float mRemainingDuration; - int32_t mUnused; }; + struct AiTravelData { float mX, mY, mZ; }; + struct AiEscortData { float mX, mY, mZ; int16_t mDuration; }; -#pragma pack(pop) - struct AiWander : AiPackage { AiWanderData mData; diff --git a/components/esm3/esmreader.hpp b/components/esm3/esmreader.hpp index 461f154001..276adf749c 100644 --- a/components/esm3/esmreader.hpp +++ b/components/esm3/esmreader.hpp @@ -12,8 +12,10 @@ #include +#include "components/esm/decompose.hpp" #include "components/esm/esmcommon.hpp" #include "components/esm/refid.hpp" + #include "loadtes3.hpp" namespace ESM @@ -177,6 +179,16 @@ namespace ESM (getT(args), ...); } + void getNamedComposite(NAME name, auto& value) + { + decompose(value, [&](auto&... args) { getHNT(name, args...); }); + } + + void getComposite(auto& value) + { + decompose(value, [&](auto&... args) { (getT(args), ...); }); + } + template >> void skipHT() { diff --git a/components/esm3/esmwriter.hpp b/components/esm3/esmwriter.hpp index 5086005b1f..101246fe43 100644 --- a/components/esm3/esmwriter.hpp +++ b/components/esm3/esmwriter.hpp @@ -5,6 +5,7 @@ #include #include +#include "components/esm/decompose.hpp" #include "components/esm/esmcommon.hpp" #include "components/esm/refid.hpp" @@ -121,6 +122,20 @@ namespace ESM endRecord(name); } + void writeNamedComposite(NAME name, const auto& value) + { + decompose(value, [&](const auto&... args) { + startSubRecord(name); + (writeT(args), ...); + endRecord(name); + }); + } + + void writeComposite(const auto& value) + { + decompose(value, [&](const auto&... args) { (writeT(args), ...); }); + } + // Prevent using writeHNT with strings. This already happened by accident and results in // state being discarded without any error on writing or reading it. :( // writeHNString and friends must be used instead. @@ -132,7 +147,7 @@ namespace ESM void writeHNT(NAME name, const T (&data)[size], int) = delete; template - void writeHNT(NAME name, const T& data, int size) + void writeHNT(NAME name, const T& data, std::size_t size) { startSubRecord(name); writeT(data, size); diff --git a/components/esm3/loadscpt.cpp b/components/esm3/loadscpt.cpp index c1dc759cdc..f79f4989ef 100644 --- a/components/esm3/loadscpt.cpp +++ b/components/esm3/loadscpt.cpp @@ -4,12 +4,19 @@ #include #include +#include #include "esmreader.hpp" #include "esmwriter.hpp" namespace ESM { + template T> + void decompose(T&& v, const auto& f) + { + f(v.mNumShorts, v.mNumLongs, v.mNumFloats, v.mScriptDataSize, v.mStringTableSize); + } + void Script::loadSCVR(ESMReader& esm) { uint32_t s = mData.mStringTableSize; @@ -99,11 +106,7 @@ namespace ESM { esm.getSubHeader(); mId = esm.getMaybeFixedRefIdSize(32); - esm.getT(mData.mNumShorts); - esm.getT(mData.mNumLongs); - esm.getT(mData.mNumFloats); - esm.getT(mData.mScriptDataSize); - esm.getT(mData.mStringTableSize); + esm.getComposite(mData); hasHeader = true; break; @@ -157,7 +160,7 @@ namespace ESM esm.startSubRecord("SCHD"); esm.writeMaybeFixedSizeRefId(mId, 32); - esm.writeT(mData, 20); + esm.writeComposite(mData); esm.endRecord("SCHD"); if (isDeleted) diff --git a/components/misc/concepts.hpp b/components/misc/concepts.hpp new file mode 100644 index 0000000000..d8573e94ab --- /dev/null +++ b/components/misc/concepts.hpp @@ -0,0 +1,13 @@ +#ifndef OPENMW_COMPONENTS_MISC_CONCEPTS_H +#define OPENMW_COMPONENTS_MISC_CONCEPTS_H + +#include +#include + +namespace Misc +{ + template + concept SameAsWithoutCvref = std::same_as, std::remove_cvref_t>; +} + +#endif