Merge branch 'test_ptr' into 'master'

Add tests for MWWorld::Ptr

See merge request OpenMW/openmw!4344

apps/openmw/mwrender/characterpreview.cpp

@@ -533,7 +533,7 @@ namespace MWRender
         : CharacterPreview(
             parent, resourceSystem, MWMechanics::getPlayer(), 512, 512, osg::Vec3f(0, 125, 8), osg::Vec3f(0, 0, 8))
         , mBase(*mCharacter.get<ESM::NPC>()->mBase)
-        , mRef(&mBase)
+        , mRef(ESM::makeBlankCellRef(), &mBase)
         , mPitchRadians(osg::DegreesToRadians(6.f))
     {
         mCharacter = MWWorld::Ptr(&mRef, nullptr);

apps/openmw/mwworld/cellstore.cpp

@@ -317,9 +317,9 @@ namespace
         }
 
         // new reference
-        MWWorld::LiveCellRef<T> ref(record);
+        MWWorld::LiveCellRef<T> ref(ESM::makeBlankCellRef(), record);
         ref.load(state);
-        collection.mList.push_back(ref);
+        collection.mList.push_back(std::move(ref));
 
         MWWorld::LiveCellRefBase* base = &collection.mList.back();
         MWBase::Environment::get().getWorldModel()->registerPtr(MWWorld::Ptr(base, cellstore));
@@ -426,9 +426,9 @@ namespace MWWorld
                 liveCellRef.mData.setDeletedByContentFile(true);
 
             if (iter != mList.end())
-                *iter = liveCellRef;
+                *iter = std::move(liveCellRef);
             else
-                mList.push_back(liveCellRef);
+                mList.push_back(std::move(liveCellRef));
         }
         else
         {
@@ -455,7 +455,7 @@ namespace MWWorld
         LiveCellRef<X> liveCellRef(ref, ptr);
         if (!isEnabled(ref, esmStore))
             liveCellRef.mData.disable();
-        list.push_back(liveCellRef);
+        list.push_back(std::move(liveCellRef));
     }
 
     template <typename X>

apps/openmw/mwworld/containerstore.cpp

@@ -101,9 +101,9 @@ MWWorld::ContainerStoreIterator MWWorld::ContainerStore::getState(
     if (!record)
         return ContainerStoreIterator(this);
 
-    LiveCellRef<T> ref(record);
+    LiveCellRef<T> ref(ESM::makeBlankCellRef(), record);
     ref.load(state);
-    collection.mList.push_back(ref);
+    collection.mList.push_back(std::move(ref));
     auto it = ContainerStoreIterator(this, --collection.mList.end());
     MWBase::Environment::get().getWorldModel()->registerPtr(*it);
 

apps/openmw/mwworld/livecellref.cpp

@@ -15,104 +15,122 @@
 #include "ptr.hpp"
 #include "worldmodel.hpp"
 
-MWWorld::LiveCellRefBase::LiveCellRefBase(unsigned int type, const ESM::CellRef& cref)
+namespace MWWorld
-    : mClass(&Class::get(type))
-    , mRef(cref)
-    , mData(cref)
 {
-}
+    LiveCellRefBase::LiveCellRefBase(unsigned int type, const ESM::CellRef& cref)
+        : mClass(&Class::get(type))
+        , mRef(cref)
+        , mData(cref)
+    {
+    }
 
-MWWorld::LiveCellRefBase::LiveCellRefBase(unsigned int type, const ESM4::Reference& cref)
+    LiveCellRefBase::LiveCellRefBase(unsigned int type, const ESM4::Reference& cref)
-    : mClass(&Class::get(type))
+        : mClass(&Class::get(type))
-    , mRef(cref)
+        , mRef(cref)
-    , mData(cref)
+        , mData(cref)
-{
+    {
-}
+    }
 
-MWWorld::LiveCellRefBase::LiveCellRefBase(unsigned int type, const ESM4::ActorCharacter& cref)
+    LiveCellRefBase::LiveCellRefBase(unsigned int type, const ESM4::ActorCharacter& cref)
-    : mClass(&Class::get(type))
+        : mClass(&Class::get(type))
-    , mRef(cref)
+        , mRef(cref)
-    , mData(cref)
+        , mData(cref)
-{
+    {
-}
+    }
 
-MWWorld::LiveCellRefBase::~LiveCellRefBase()
+    LiveCellRefBase::LiveCellRefBase(LiveCellRefBase&& other) noexcept
-{
+        : mClass(other.mClass)
-    MWBase::Environment::get().getWorldModel()->deregisterLiveCellRef(*this);
+        , mRef(std::move(other.mRef))
-}
+        , mData(std::move(other.mData))
+        , mWorldModel(std::exchange(other.mWorldModel, nullptr))
+    {
+    }
 
-void MWWorld::LiveCellRefBase::loadImp(const ESM::ObjectState& state)
+    LiveCellRefBase::~LiveCellRefBase()
-{
+    {
-    mRef = MWWorld::CellRef(state.mRef);
+        if (mWorldModel != nullptr)
-    mData = RefData(state, mData.isDeletedByContentFile());
+            mWorldModel->deregisterLiveCellRef(*this);
+    }
 
-    Ptr ptr(this);
+    LiveCellRefBase& LiveCellRefBase::operator=(LiveCellRefBase&& other) noexcept
+    {
+        mClass = other.mClass;
+        mRef = std::move(other.mRef);
+        mData = std::move(other.mData);
+        mWorldModel = std::exchange(other.mWorldModel, nullptr);
+        return *this;
+    }
 
-    if (state.mHasLocals)
+    void LiveCellRefBase::loadImp(const ESM::ObjectState& state)
     {
-        const ESM::RefId& scriptId = mClass->getScript(ptr);
+        mRef = CellRef(state.mRef);
-        // Make sure we still have a script. It could have been coming from a content file that is no longer active.
+        mData = RefData(state, mData.isDeletedByContentFile());
-        if (!scriptId.empty())
+
+        Ptr ptr(this);
+
+        if (state.mHasLocals)
         {
-            if (const ESM::Script* script
+            const ESM::RefId& scriptId = mClass->getScript(ptr);
-                = MWBase::Environment::get().getESMStore()->get<ESM::Script>().search(scriptId))
+            // Make sure we still have a script. It could have been coming from a content file that is no longer active.
+            if (!scriptId.empty())
             {
-                try
+                if (const ESM::Script* script
-                {
+                    = MWBase::Environment::get().getESMStore()->get<ESM::Script>().search(scriptId))
-                    mData.setLocals(*script);
-                    mData.getLocals().read(state.mLocals, scriptId);
-                }
-                catch (const std::exception& exception)
                 {
-                    Log(Debug::Error) << "Error: failed to load state for local script " << scriptId
+                    try
-                                      << " because an exception has been thrown: " << exception.what();
+                    {
+                        mData.setLocals(*script);
+                        mData.getLocals().read(state.mLocals, scriptId);
+                    }
+                    catch (const std::exception& exception)
+                    {
+                        Log(Debug::Error) << "Error: failed to load state for local script " << scriptId
+                                          << " because an exception has been thrown: " << exception.what();
+                    }
                 }
             }
         }
-    }
 
-    mClass->readAdditionalState(ptr, state);
+        mClass->readAdditionalState(ptr, state);
 
-    if (!mRef.getSoul().empty()
+        if (!mRef.getSoul().empty()
-        && !MWBase::Environment::get().getESMStore()->get<ESM::Creature>().search(mRef.getSoul()))
+            && !MWBase::Environment::get().getESMStore()->get<ESM::Creature>().search(mRef.getSoul()))
-    {
+        {
-        Log(Debug::Warning) << "Soul '" << mRef.getSoul() << "' not found, removing the soul from soul gem";
+            Log(Debug::Warning) << "Soul '" << mRef.getSoul() << "' not found, removing the soul from soul gem";
-        mRef.setSoul(ESM::RefId());
+            mRef.setSoul(ESM::RefId());
-    }
+        }
 
-    MWBase::Environment::get().getLuaManager()->loadLocalScripts(ptr, state.mLuaScripts);
+        MWBase::Environment::get().getLuaManager()->loadLocalScripts(ptr, state.mLuaScripts);
-}
+    }
 
-void MWWorld::LiveCellRefBase::saveImp(ESM::ObjectState& state) const
+    void LiveCellRefBase::saveImp(ESM::ObjectState& state) const
-{
+    {
-    mRef.writeState(state);
+        mRef.writeState(state);
 
-    ConstPtr ptr(this);
+        ConstPtr ptr(this);
 
-    mData.write(state, mClass->getScript(ptr));
+        mData.write(state, mClass->getScript(ptr));
-    MWBase::Environment::get().getLuaManager()->saveLocalScripts(
+        MWBase::Environment::get().getLuaManager()->saveLocalScripts(
-        Ptr(const_cast<LiveCellRefBase*>(this)), state.mLuaScripts);
+            Ptr(const_cast<LiveCellRefBase*>(this)), state.mLuaScripts);
 
-    mClass->writeAdditionalState(ptr, state);
+        mClass->writeAdditionalState(ptr, state);
-}
+    }
 
-bool MWWorld::LiveCellRefBase::checkStateImp(const ESM::ObjectState& state)
+    bool LiveCellRefBase::checkStateImp(const ESM::ObjectState& state)
-{
+    {
-    return true;
+        return true;
-}
+    }
 
-unsigned int MWWorld::LiveCellRefBase::getType() const
+    unsigned int LiveCellRefBase::getType() const
-{
+    {
-    return mClass->getType();
+        return mClass->getType();
-}
+    }
 
-bool MWWorld::LiveCellRefBase::isDeleted() const
+    bool LiveCellRefBase::isDeleted() const
-{
+    {
-    return mData.isDeletedByContentFile() || mRef.getCount(false) == 0;
+        return mData.isDeletedByContentFile() || mRef.getCount(false) == 0;
-}
+    }
 
-namespace MWWorld
-{
     std::string makeDynamicCastErrorMessage(const LiveCellRefBase* value, std::string_view recordType)
     {
         std::stringstream message;

apps/openmw/mwworld/livecellref.hpp

@@ -17,6 +17,7 @@ namespace MWWorld
     class Ptr;
     class ESMStore;
     class Class;
+    class WorldModel;
 
     template <typename X>
     struct LiveCellRef;
@@ -29,17 +30,28 @@ namespace MWWorld
         /** Information about this instance, such as 3D location and rotation
          * and individual type-dependent data.
          */
-        MWWorld::CellRef mRef;
+        CellRef mRef;
 
         /** runtime-data */
         RefData mData;
 
-        LiveCellRefBase(unsigned int type, const ESM::CellRef& cref = ESM::CellRef());
+        WorldModel* mWorldModel = nullptr;
+
+        LiveCellRefBase(unsigned int type, const ESM::CellRef& cref);
         LiveCellRefBase(unsigned int type, const ESM4::Reference& cref);
         LiveCellRefBase(unsigned int type, const ESM4::ActorCharacter& cref);
+
+        LiveCellRefBase(const LiveCellRefBase& other) = default;
+
+        LiveCellRefBase(LiveCellRefBase&& other) noexcept;
+
         /* Need this for the class to be recognized as polymorphic */
         virtual ~LiveCellRefBase();
 
+        LiveCellRefBase& operator=(const LiveCellRefBase& other) = default;
+
+        LiveCellRefBase& operator=(LiveCellRefBase&& other) noexcept;
+
         virtual void load(const ESM::ObjectState& state) = 0;
         ///< Load state into a LiveCellRef, that has already been initialised with base and class.
         ///
@@ -132,12 +144,6 @@ namespace MWWorld
         {
         }
 
-        LiveCellRef(const X* b = nullptr)
-            : LiveCellRefBase(X::sRecordId)
-            , mBase(b)
-        {
-        }
-
         // The object that this instance is based on.
         const X* mBase;
 

apps/openmw/mwworld/player.cpp

@@ -36,8 +36,20 @@
 
 namespace MWWorld
 {
+    namespace
+    {
+        ESM::CellRef makePlayerCellRef()
+        {
+            ESM::CellRef result;
+            result.blank();
+            result.mRefID = ESM::RefId::stringRefId("Player");
+            return result;
+        }
+    }
+
     Player::Player(const ESM::NPC* player)
-        : mCellStore(nullptr)
+        : mPlayer(makePlayerCellRef(), player)
+        , mCellStore(nullptr)
         , mLastKnownExteriorPosition(0, 0, 0)
         , mMarkedPosition(ESM::Position())
         , mMarkedCell(nullptr)
@@ -46,11 +58,6 @@ namespace MWWorld
         , mPaidCrimeId(-1)
         , mJumping(false)
     {
-        ESM::CellRef cellRef;
-        cellRef.blank();
-        cellRef.mRefID = ESM::RefId::stringRefId("Player");
-        mPlayer = LiveCellRef<ESM::NPC>(cellRef, player);
-
         ESM::Position playerPos = mPlayer.mData.getPosition();
         playerPos.pos[0] = playerPos.pos[1] = playerPos.pos[2] = 0;
         mPlayer.mData.setPosition(playerPos);

apps/openmw/mwworld/worldmodel.cpp

@@ -3,6 +3,7 @@
 #include <algorithm>
 #include <cassert>
 #include <optional>
+#include <stdexcept>
 
 #include <components/debug/debuglog.hpp>
 #include <components/esm/defs.hpp>
@@ -339,6 +340,20 @@ namespace MWWorld
             throw std::runtime_error(std::string("Can't find cell with name ") + std::string(name));
         return *result;
     }
+
+    void WorldModel::registerPtr(const Ptr& ptr)
+    {
+        if (ptr.mRef == nullptr)
+            throw std::logic_error("Ptr with nullptr mRef is not allowed to be registered");
+        mPtrRegistry.insert(ptr);
+        ptr.mRef->mWorldModel = this;
+    }
+
+    void WorldModel::deregisterLiveCellRef(LiveCellRefBase& ref) noexcept
+    {
+        mPtrRegistry.remove(ref);
+        ref.mWorldModel = nullptr;
+    }
 }
 
 MWWorld::Ptr MWWorld::WorldModel::getPtrByRefId(const ESM::RefId& name)

apps/openmw/mwworld/worldmodel.hpp

@@ -77,9 +77,9 @@ namespace MWWorld
 
         std::size_t getPtrRegistryRevision() const { return mPtrRegistry.getRevision(); }
 
-        void registerPtr(const Ptr& ptr) { mPtrRegistry.insert(ptr); }
+        void registerPtr(const Ptr& ptr);
 
-        void deregisterLiveCellRef(const LiveCellRefBase& ref) noexcept { mPtrRegistry.remove(ref); }
+        void deregisterLiveCellRef(LiveCellRefBase& ref) noexcept;
 
         void assignSaveFileRefNum(ESM::CellRef& ref) { mPtrRegistry.assign(ref); }
 

apps/openmw_tests/CMakeLists.txt

@@ -9,6 +9,7 @@ file(GLOB UNITTEST_SRC_FILES
     mwworld/test_store.cpp
     mwworld/testduration.cpp
     mwworld/testtimestamp.cpp
+    mwworld/testptr.cpp
 
     mwdialogue/test_keywordsearch.cpp
 

apps/openmw_tests/main.cpp

@@ -1,11 +1,28 @@
 #include <components/debug/debugging.hpp>
+#include <components/misc/strings/conversion.hpp>
+#include <components/settings/parser.hpp>
+#include <components/settings/values.hpp>
 
 #include <gtest/gtest.h>
 
+#include <filesystem>
+
 int main(int argc, char* argv[])
 {
     Log::sMinDebugLevel = Debug::getDebugLevel();
 
+    const std::filesystem::path settingsDefaultPath = std::filesystem::path{ OPENMW_PROJECT_SOURCE_DIR } / "files"
+        / Misc::StringUtils::stringToU8String("settings-default.cfg");
+
+    Settings::SettingsFileParser parser;
+    parser.loadSettingsFile(settingsDefaultPath, Settings::Manager::mDefaultSettings);
+
+    Settings::StaticValues::initDefaults();
+
+    Settings::Manager::mUserSettings = Settings::Manager::mDefaultSettings;
+
+    Settings::StaticValues::init();
+
     testing::InitGoogleTest(&argc, argv);
     return RUN_ALL_TESTS();
 }

apps/openmw_tests/mwworld/testptr.cpp

@@ -0,0 +1,82 @@
+#include "apps/openmw/mwclass/npc.hpp"
+#include "apps/openmw/mwworld/esmstore.hpp"
+#include "apps/openmw/mwworld/livecellref.hpp"
+#include "apps/openmw/mwworld/ptr.hpp"
+#include "apps/openmw/mwworld/worldmodel.hpp"
+
+#include <components/esm3/loadnpc.hpp>
+#include <components/esm3/readerscache.hpp>
+
+#include <gtest/gtest.h>
+
+namespace MWWorld
+{
+    namespace
+    {
+        using namespace testing;
+
+        TEST(MWWorldPtrTest, toStringShouldReturnHumanReadableTextRepresentationOfPtrWithNullRef)
+        {
+            Ptr ptr;
+            EXPECT_EQ(ptr.toString(), "null object");
+        }
+
+        TEST(MWWorldPtrTest, toStringShouldReturnHumanReadableTextRepresentationOfPtrWithDeletedRef)
+        {
+            MWClass::Npc::registerSelf();
+            ESM::NPC npc;
+            npc.blank();
+            npc.mId = ESM::RefId::stringRefId("Player");
+            ESMStore store;
+            store.insert(npc);
+            ESM::CellRef cellRef;
+            cellRef.blank();
+            cellRef.mRefID = npc.mId;
+            cellRef.mRefNum = ESM::RefNum{ .mIndex = 0x2a, .mContentFile = 0xd };
+            LiveCellRef<ESM::NPC> liveCellRef(cellRef, &npc);
+            liveCellRef.mData.setDeletedByContentFile(true);
+            Ptr ptr(&liveCellRef);
+            EXPECT_EQ(ptr.toString(), "deleted object0xd00002a (NPC, \"player\")");
+        }
+
+        TEST(MWWorldPtrTest, toStringShouldReturnHumanReadableTextRepresentationOfPtr)
+        {
+            MWClass::Npc::registerSelf();
+            ESM::NPC npc;
+            npc.blank();
+            npc.mId = ESM::RefId::stringRefId("Player");
+            ESMStore store;
+            store.insert(npc);
+            ESM::CellRef cellRef;
+            cellRef.blank();
+            cellRef.mRefID = npc.mId;
+            cellRef.mRefNum = ESM::RefNum{ .mIndex = 0x2a, .mContentFile = 0xd };
+            LiveCellRef<ESM::NPC> liveCellRef(cellRef, &npc);
+            Ptr ptr(&liveCellRef);
+            EXPECT_EQ(ptr.toString(), "object0xd00002a (NPC, \"player\")");
+        }
+
+        TEST(MWWorldPtrTest, underlyingLiveCellRefShouldBeDeregisteredOnDestruction)
+        {
+            MWClass::Npc::registerSelf();
+            ESM::NPC npc;
+            npc.blank();
+            npc.mId = ESM::RefId::stringRefId("Player");
+            ESMStore store;
+            store.insert(npc);
+            ESM::ReadersCache readersCache;
+            WorldModel worldModel(store, readersCache);
+            ESM::CellRef cellRef;
+            cellRef.blank();
+            cellRef.mRefID = npc.mId;
+            cellRef.mRefNum = ESM::FormId{ .mIndex = 0x2a, .mContentFile = 0xd };
+            {
+                LiveCellRef<ESM::NPC> liveCellRef(cellRef, &npc);
+                Ptr ptr(&liveCellRef);
+                worldModel.registerPtr(ptr);
+                ASSERT_EQ(worldModel.getPtr(cellRef.mRefNum), ptr);
+            }
+            EXPECT_EQ(worldModel.getPtr(cellRef.mRefNum), Ptr());
+        }
+    }
+}

components/esm3/cellref.cpp

@@ -287,4 +287,10 @@ namespace ESM
         loadDataImpl<false>(esm, isDeleted, cellRef);
     }
 
+    CellRef makeBlankCellRef()
+    {
+        CellRef result;
+        result.blank();
+        return result;
+    }
 }

components/esm3/cellref.hpp

@@ -105,6 +105,8 @@ namespace ESM
     };
 
     void skipLoadCellRef(ESMReader& esm, bool wideRefNum = false);
+
+    CellRef makeBlankCellRef();
 }
 
 #endif