Add unit tests for saving and loading ESM3 Land

apps/esmtool/record.cpp

@@ -866,7 +866,9 @@ namespace EsmTool
 
         if (const ESM::Land::LandData* data = mData.getLandData(mData.mDataTypes))
         {
-            std::cout << "  Height Offset: " << data->mHeightOffset << std::endl;
+            std::cout << "  MinHeight: " << data->mMinHeight << std::endl;
+            std::cout << "  MaxHeight: " << data->mMaxHeight << std::endl;
+            std::cout << "  DataLoaded: " << data->mDataLoaded << std::endl;
         }
         mData.unloadData();
         std::cout << "  Deleted: " << mIsDeleted << std::endl;

apps/openmw_test_suite/esm3/testsaveload.cpp

@@ -7,6 +7,7 @@
 #include <components/esm3/loadcont.hpp>
 #include <components/esm3/loaddial.hpp>
 #include <components/esm3/loadinfo.hpp>
+#include <components/esm3/loadland.hpp>
 #include <components/esm3/loadregn.hpp>
 #include <components/esm3/loadscpt.hpp>
 #include <components/esm3/loadweap.hpp>
@@ -20,8 +21,8 @@
 #include <array>
 #include <limits>
 #include <memory>
+#include <numeric>
 #include <random>
-#include <type_traits>
 
 namespace ESM
 {
@@ -172,6 +173,17 @@ namespace ESM
             reader.getComposite(record);
         }
 
+        void load(ESMReader& reader, Land& record)
+        {
+            bool deleted = false;
+            record.load(reader, deleted);
+            if (deleted)
+                return;
+            record.mLandData = std::make_unique<LandRecordData>();
+            reader.restoreContext(record.mContext);
+            loadLandRecordData(record.mDataTypes, reader, *record.mLandData);
+        }
+
         template <typename T>
         void saveAndLoadRecord(const T& record, FormatVersion formatVersion, T& result)
         {
@@ -681,6 +693,43 @@ namespace ESM
             }
         }
 
+        TEST_P(Esm3SaveLoadRecordTest, landShouldNotChange)
+        {
+            LandRecordData data;
+            std::iota(data.mHeights.begin(), data.mHeights.end(), 1);
+            std::for_each(data.mHeights.begin(), data.mHeights.end(), [](float& v) { v *= Land::sHeightScale; });
+            data.mMinHeight = *std::min_element(data.mHeights.begin(), data.mHeights.end());
+            data.mMaxHeight = *std::max_element(data.mHeights.begin(), data.mHeights.end());
+            std::iota(data.mNormals.begin(), data.mNormals.end(), 2);
+            std::iota(data.mTextures.begin(), data.mTextures.end(), 3);
+            std::iota(data.mColours.begin(), data.mColours.end(), 4);
+            data.mDataLoaded = Land::DATA_VNML | Land::DATA_VHGT | Land::DATA_VCLR | Land::DATA_VTEX;
+
+            Land record;
+            record.mFlags = 42;
+            record.mX = 2;
+            record.mY = 3;
+            record.mDataTypes = Land::DATA_VNML | Land::DATA_VHGT | Land::DATA_WNAM | Land::DATA_VCLR | Land::DATA_VTEX;
+            generateWnam(data.mHeights, record.mWnam);
+            record.mLandData = std::make_unique<LandRecordData>(data);
+
+            Land result;
+            saveAndLoadRecord(record, GetParam(), result);
+
+            EXPECT_EQ(result.mFlags, record.mFlags);
+            EXPECT_EQ(result.mX, record.mX);
+            EXPECT_EQ(result.mY, record.mY);
+            EXPECT_EQ(result.mDataTypes, record.mDataTypes);
+            EXPECT_EQ(result.mWnam, record.mWnam);
+            EXPECT_EQ(result.mLandData->mHeights, record.mLandData->mHeights);
+            EXPECT_EQ(result.mLandData->mMinHeight, record.mLandData->mMinHeight);
+            EXPECT_EQ(result.mLandData->mMaxHeight, record.mLandData->mMaxHeight);
+            EXPECT_EQ(result.mLandData->mNormals, record.mLandData->mNormals);
+            EXPECT_EQ(result.mLandData->mTextures, record.mLandData->mTextures);
+            EXPECT_EQ(result.mLandData->mColours, record.mLandData->mColours);
+            EXPECT_EQ(result.mLandData->mDataLoaded, record.mLandData->mDataLoaded);
+        }
+
         INSTANTIATE_TEST_SUITE_P(FormatVersions, Esm3SaveLoadRecordTest, ValuesIn(getFormats()));
     }
 }

components/esm3/landrecorddata.hpp

@@ -20,8 +20,6 @@ namespace ESM
         // total number of textures per land
         static constexpr unsigned sLandNumTextures = sLandTextureSize * sLandTextureSize;
 
-        // Initial reference height for the first vertex, only needed for filling mHeights
-        float mHeightOffset = 0;
         // Height in world space for each vertex
         std::array<float, sLandNumVerts> mHeights;
         float mMinHeight = 0;

components/esm3/loadland.cpp

@@ -40,15 +40,15 @@ namespace ESM
 
         // Loads data and marks it as loaded. Return true if data is actually loaded from reader, false otherwise
         // including the case when data is already loaded.
-        bool condLoad(ESMReader& reader, int flags, int& targetFlags, int dataFlag, auto& in)
+        bool condLoad(ESMReader& reader, int dataTypes, int& targetDataTypes, int dataFlag, auto& in)
         {
-            if ((targetFlags & dataFlag) == 0 && (flags & dataFlag) != 0)
+            if ((targetDataTypes & dataFlag) == 0 && (dataTypes & dataFlag) != 0)
             {
                 if constexpr (std::is_same_v<std::remove_cvref_t<decltype(in)>, VHGT>)
                     reader.getSubComposite(in);
                 else
                     reader.getHT(in);
-                targetFlags |= dataFlag;
+                targetDataTypes |= dataFlag;
                 return true;
             }
             reader.skipHSub();
@@ -150,13 +150,13 @@ namespace ESM
             if (mDataTypes & Land::DATA_VHGT)
             {
                 VHGT offsets;
-                offsets.mHeightOffset = mLandData->mHeights[0] / HEIGHT_SCALE;
+                offsets.mHeightOffset = mLandData->mHeights[0] / sHeightScale;
 
                 float prevY = mLandData->mHeights[0];
                 size_t number = 0; // avoid multiplication
                 for (unsigned i = 0; i < LandRecordData::sLandSize; ++i)
                 {
-                    float diff = (mLandData->mHeights[number] - prevY) / HEIGHT_SCALE;
+                    float diff = (mLandData->mHeights[number] - prevY) / sHeightScale;
                     offsets.mHeightData[number]
                         = diff >= 0 ? static_cast<std::int8_t>(diff + 0.5) : static_cast<std::int8_t>(diff - 0.5);
 
@@ -165,7 +165,7 @@ namespace ESM
 
                     for (unsigned j = 1; j < LandRecordData::sLandSize; ++j)
                     {
-                        diff = (mLandData->mHeights[number] - prevX) / HEIGHT_SCALE;
+                        diff = (mLandData->mHeights[number] - prevX) / sHeightScale;
                         offsets.mHeightData[number]
                             = diff >= 0 ? static_cast<std::int8_t>(diff + 0.5) : static_cast<std::int8_t>(diff - 0.5);
 
@@ -178,23 +178,8 @@ namespace ESM
             if (mDataTypes & Land::DATA_WNAM)
             {
                 // Generate WNAM record
-                std::int8_t wnam[LAND_GLOBAL_MAP_LOD_SIZE];
+                std::array<std::int8_t, sGlobalMapLodSize> wnam;
-                constexpr float max = std::numeric_limits<std::int8_t>::max();
+                generateWnam(mLandData->mHeights, wnam);
-                constexpr float min = std::numeric_limits<std::int8_t>::min();
-                constexpr float vertMult
-                    = static_cast<float>(LandRecordData::sLandSize - 1) / LAND_GLOBAL_MAP_LOD_SIZE_SQRT;
-                for (unsigned row = 0; row < LAND_GLOBAL_MAP_LOD_SIZE_SQRT; ++row)
-                {
-                    for (unsigned col = 0; col < LAND_GLOBAL_MAP_LOD_SIZE_SQRT; ++col)
-                    {
-                        float height
-                            = mLandData->mHeights[static_cast<size_t>(row * vertMult) * LandRecordData::sLandSize
-                                + static_cast<size_t>(col * vertMult)];
-                        height /= height > 0 ? 128.f : 16.f;
-                        height = std::clamp(height, min, max);
-                        wnam[row * LAND_GLOBAL_MAP_LOD_SIZE_SQRT + col] = static_cast<std::int8_t>(height);
-                    }
-                }
                 esm.writeHNT("WNAM", wnam);
             }
             if (mDataTypes & Land::DATA_VCLR)
@@ -219,7 +204,6 @@ namespace ESM
         if (mLandData == nullptr)
             mLandData = std::make_unique<LandData>();
 
-        mLandData->mHeightOffset = 0;
         mLandData->mHeights.fill(0);
         mLandData->mMinHeight = 0;
         mLandData->mMaxHeight = 0;
@@ -239,20 +223,20 @@ namespace ESM
         mContext.filename.clear();
     }
 
-    void Land::loadData(int flags) const
+    void Land::loadData(int dataTypes) const
     {
         if (mLandData == nullptr)
             mLandData = std::make_unique<LandData>();
 
-        loadData(flags, *mLandData);
+        loadData(dataTypes, *mLandData);
     }
 
-    void Land::loadData(int flags, LandData& data) const
+    void Land::loadData(int dataTypes, LandData& data) const
     {
         // Try to load only available data
-        flags = flags & mDataTypes;
+        dataTypes = dataTypes & mDataTypes;
         // Return if all required data is loaded
-        if ((data.mDataLoaded & flags) == flags)
+        if ((data.mDataLoaded & dataTypes) == dataTypes)
         {
             return;
         }
@@ -269,57 +253,7 @@ namespace ESM
         ESMReader reader;
         reader.restoreContext(mContext);
 
-        if (reader.isNextSub("VNML"))
+        loadLandRecordData(dataTypes, reader, data);
-        {
-            condLoad(reader, flags, data.mDataLoaded, DATA_VNML, data.mNormals);
-        }
-
-        if (reader.isNextSub("VHGT"))
-        {
-            VHGT vhgt;
-            if (condLoad(reader, flags, data.mDataLoaded, DATA_VHGT, vhgt))
-            {
-                data.mMinHeight = std::numeric_limits<float>::max();
-                data.mMaxHeight = -std::numeric_limits<float>::max();
-                float rowOffset = vhgt.mHeightOffset;
-                for (unsigned y = 0; y < LandRecordData::sLandSize; y++)
-                {
-                    rowOffset += vhgt.mHeightData[y * LandRecordData::sLandSize];
-
-                    data.mHeights[y * LandRecordData::sLandSize] = rowOffset * HEIGHT_SCALE;
-                    if (rowOffset * HEIGHT_SCALE > data.mMaxHeight)
-                        data.mMaxHeight = rowOffset * HEIGHT_SCALE;
-                    if (rowOffset * HEIGHT_SCALE < data.mMinHeight)
-                        data.mMinHeight = rowOffset * HEIGHT_SCALE;
-
-                    float colOffset = rowOffset;
-                    for (unsigned x = 1; x < LandRecordData::sLandSize; x++)
-                    {
-                        colOffset += vhgt.mHeightData[y * LandRecordData::sLandSize + x];
-                        data.mHeights[x + y * LandRecordData::sLandSize] = colOffset * HEIGHT_SCALE;
-
-                        if (colOffset * HEIGHT_SCALE > data.mMaxHeight)
-                            data.mMaxHeight = colOffset * HEIGHT_SCALE;
-                        if (colOffset * HEIGHT_SCALE < data.mMinHeight)
-                            data.mMinHeight = colOffset * HEIGHT_SCALE;
-                    }
-                }
-            }
-        }
-
-        if (reader.isNextSub("WNAM"))
-            reader.skipHSub();
-
-        if (reader.isNextSub("VCLR"))
-            condLoad(reader, flags, data.mDataLoaded, DATA_VCLR, data.mColours);
-        if (reader.isNextSub("VTEX"))
-        {
-            uint16_t vtex[LandRecordData::sLandNumTextures];
-            if (condLoad(reader, flags, data.mDataLoaded, DATA_VTEX, vtex))
-            {
-                transposeTextureData(vtex, data.mTextures.data());
-            }
-        }
     }
 
     void Land::unloadData()
@@ -367,4 +301,75 @@ namespace ESM
         mDataTypes |= flags;
         mLandData->mDataLoaded |= flags;
     }
+
+    void loadLandRecordData(int dataTypes, ESMReader& reader, LandRecordData& data)
+    {
+        if (reader.isNextSub("VNML"))
+            condLoad(reader, dataTypes, data.mDataLoaded, Land::DATA_VNML, data.mNormals);
+
+        if (reader.isNextSub("VHGT"))
+        {
+            VHGT vhgt;
+            if (condLoad(reader, dataTypes, data.mDataLoaded, Land::DATA_VHGT, vhgt))
+            {
+                data.mMinHeight = std::numeric_limits<float>::max();
+                data.mMaxHeight = -std::numeric_limits<float>::max();
+                float rowOffset = vhgt.mHeightOffset;
+                for (unsigned y = 0; y < LandRecordData::sLandSize; y++)
+                {
+                    rowOffset += vhgt.mHeightData[y * LandRecordData::sLandSize];
+
+                    data.mHeights[y * LandRecordData::sLandSize] = rowOffset * Land::sHeightScale;
+                    if (rowOffset * Land::sHeightScale > data.mMaxHeight)
+                        data.mMaxHeight = rowOffset * Land::sHeightScale;
+                    if (rowOffset * Land::sHeightScale < data.mMinHeight)
+                        data.mMinHeight = rowOffset * Land::sHeightScale;
+
+                    float colOffset = rowOffset;
+                    for (unsigned x = 1; x < LandRecordData::sLandSize; x++)
+                    {
+                        colOffset += vhgt.mHeightData[y * LandRecordData::sLandSize + x];
+                        data.mHeights[x + y * LandRecordData::sLandSize] = colOffset * Land::sHeightScale;
+
+                        if (colOffset * Land::sHeightScale > data.mMaxHeight)
+                            data.mMaxHeight = colOffset * Land::sHeightScale;
+                        if (colOffset * Land::sHeightScale < data.mMinHeight)
+                            data.mMinHeight = colOffset * Land::sHeightScale;
+                    }
+                }
+            }
+        }
+
+        if (reader.isNextSub("WNAM"))
+            reader.skipHSub();
+
+        if (reader.isNextSub("VCLR"))
+            condLoad(reader, dataTypes, data.mDataLoaded, Land::DATA_VCLR, data.mColours);
+
+        if (reader.isNextSub("VTEX"))
+        {
+            std::uint16_t vtex[LandRecordData::sLandNumTextures];
+            if (condLoad(reader, dataTypes, data.mDataLoaded, Land::DATA_VTEX, vtex))
+                transposeTextureData(vtex, data.mTextures.data());
+        }
+    }
+
+    void generateWnam(const std::array<float, LandRecordData::sLandNumVerts>& heights,
+        std::array<std::int8_t, Land::sGlobalMapLodSize>& wnam)
+    {
+        constexpr float max = std::numeric_limits<std::int8_t>::max();
+        constexpr float min = std::numeric_limits<std::int8_t>::min();
+        constexpr float vertMult = static_cast<float>(LandRecordData::sLandSize - 1) / Land::sGlobalMapLodSizeSqrt;
+        for (std::size_t row = 0; row < Land::sGlobalMapLodSizeSqrt; ++row)
+        {
+            for (std::size_t col = 0; col < Land::sGlobalMapLodSizeSqrt; ++col)
+            {
+                float height = heights[static_cast<std::size_t>(row * vertMult) * LandRecordData::sLandSize
+                    + static_cast<std::size_t>(col * vertMult)];
+                height /= height > 0 ? 128.f : 16.f;
+                height = std::clamp(height, min, max);
+                wnam[row * Land::sGlobalMapLodSizeSqrt + col] = static_cast<std::int8_t>(height);
+            }
+        }
+    }
 }

components/esm3/loadland.hpp

@@ -78,7 +78,7 @@ namespace ESM
         // total number of vertices
         static constexpr int LAND_NUM_VERTS = LandRecordData::sLandNumVerts;
 
-        static constexpr int HEIGHT_SCALE = 8;
+        static constexpr int sHeightScale = 8;
 
         // number of textures per side of land
         static constexpr int LAND_TEXTURE_SIZE = LandRecordData::sLandTextureSize;
@@ -86,23 +86,25 @@ namespace ESM
         // total number of textures per land
         static constexpr int LAND_NUM_TEXTURES = LandRecordData::sLandNumTextures;
 
-        static constexpr unsigned LAND_GLOBAL_MAP_LOD_SIZE = 81;
+        static constexpr std::size_t sGlobalMapLodSizeSqrt = 9;
 
-        static constexpr unsigned LAND_GLOBAL_MAP_LOD_SIZE_SQRT = 9;
+        static constexpr std::size_t sGlobalMapLodSize = sGlobalMapLodSizeSqrt * sGlobalMapLodSizeSqrt;
 
         using LandData = ESM::LandRecordData;
 
         // low-LOD heightmap (used for rendering the global map)
-        std::array<std::int8_t, LAND_GLOBAL_MAP_LOD_SIZE> mWnam;
+        std::array<std::int8_t, sGlobalMapLodSize> mWnam;
+
+        mutable std::unique_ptr<LandData> mLandData;
 
         void load(ESMReader& esm, bool& isDeleted);
         void save(ESMWriter& esm, bool isDeleted = false) const;
 
         void blank();
 
-        void loadData(int flags) const;
+        void loadData(int dataTypes) const;
 
-        void loadData(int flags, LandData& data) const;
+        void loadData(int dataTypes, LandData& data) const;
 
         /**
          * Frees memory allocated for mLandData
@@ -127,10 +129,12 @@ namespace ESM
         ///
         /// \note Added data fields will be uninitialised
         void add(int flags);
-
-    private:
-        mutable std::unique_ptr<LandData> mLandData;
     };
 
+    void loadLandRecordData(int dataTypes, ESMReader& reader, LandRecordData& data);
+
+    void generateWnam(const std::array<float, LandRecordData::sLandNumVerts>& heights,
+        std::array<std::int8_t, Land::sGlobalMapLodSize>& wnam);
 }
+
 #endif