From cfab425fb2ac973f4a96386f1589b93fc20b93cb Mon Sep 17 00:00:00 2001 From: elsid Date: Mon, 29 May 2023 19:27:06 +0200 Subject: [PATCH] Replace operator== for CellStore with pointer quality Equality operator is confusing and redundant in this case. It should not be possible to have 2 CellStores for the same cell. There is no copy constructor defined so it's not possible to get a copy. It's possible to independently create second store when another one already exist but it would mean a bug. Explicitly delete CellStore copy and move constructors and assignment operators to enforce this. --- apps/openmw/mwworld/cellstore.cpp | 6 ------ apps/openmw/mwworld/cellstore.hpp | 12 ++++++++--- apps/openmw/mwworld/scene.cpp | 13 ++---------- apps/openmw/mwworld/scene.hpp | 2 +- apps/openmw/mwworld/worldmodel.cpp | 33 +++++++++++++++++++++--------- 5 files changed, 35 insertions(+), 31 deletions(-) diff --git a/apps/openmw/mwworld/cellstore.cpp b/apps/openmw/mwworld/cellstore.cpp index 1cbf862190..fef4ecb9c8 100644 --- a/apps/openmw/mwworld/cellstore.cpp +++ b/apps/openmw/mwworld/cellstore.cpp @@ -549,7 +549,6 @@ namespace MWWorld } CellStore::~CellStore() = default; - CellStore::CellStore(CellStore&&) = default; const MWWorld::Cell* CellStore::getCell() const { @@ -1071,11 +1070,6 @@ namespace MWWorld requestMergedRefsUpdate(); } - bool CellStore::operator==(const CellStore& right) const - { - return right.mCellVariant.getId() == mCellVariant.getId(); - } - void CellStore::setFog(std::unique_ptr&& fog) { mFogState = std::move(fog); diff --git a/apps/openmw/mwworld/cellstore.hpp b/apps/openmw/mwworld/cellstore.hpp index f0690e13a1..033b92196c 100644 --- a/apps/openmw/mwworld/cellstore.hpp +++ b/apps/openmw/mwworld/cellstore.hpp @@ -134,7 +134,15 @@ namespace MWWorld /// @param readerList The readers to use for loading of the cell on-demand. CellStore(MWWorld::Cell cell, const MWWorld::ESMStore& store, ESM::ReadersCache& readers); - CellStore(CellStore&&); + + CellStore(const CellStore&) = delete; + + CellStore(CellStore&&) = delete; + + CellStore& operator=(const CellStore&) = delete; + + CellStore& operator=(CellStore&&) = delete; + ~CellStore(); const MWWorld::Cell* getCell() const; @@ -324,8 +332,6 @@ namespace MWWorld Ptr getPtr(ESM::RefId id); - bool operator==(const CellStore& right) const; - private: friend struct CellStoreImp; diff --git a/apps/openmw/mwworld/scene.cpp b/apps/openmw/mwworld/scene.cpp index f67b570b02..6925933876 100644 --- a/apps/openmw/mwworld/scene.cpp +++ b/apps/openmw/mwworld/scene.cpp @@ -877,7 +877,7 @@ namespace MWWorld loadingListener->setLabel("#{OMWEngine:LoadingInterior}"); Loading::ScopedLoad load(loadingListener); - if (mCurrentCell != nullptr && *mCurrentCell == cell) + if (mCurrentCell == &cell) { mWorld.moveObject(mWorld.getPlayerPtr(), position.asVec3()); mWorld.rotateObject(mWorld.getPlayerPtr(), position.asRotationVec3()); @@ -1015,16 +1015,7 @@ namespace MWWorld bool Scene::isCellActive(const CellStore& cell) { - CellStoreCollection::iterator active = mActiveCells.begin(); - while (active != mActiveCells.end()) - { - if (**active == cell) - { - return true; - } - ++active; - } - return false; + return mActiveCells.contains(&cell); } Ptr Scene::searchPtrViaActorId(int actorId) diff --git a/apps/openmw/mwworld/scene.hpp b/apps/openmw/mwworld/scene.hpp index c775c3ee35..e6b6e942fb 100644 --- a/apps/openmw/mwworld/scene.hpp +++ b/apps/openmw/mwworld/scene.hpp @@ -74,7 +74,7 @@ namespace MWWorld class Scene { public: - using CellStoreCollection = std::set; + using CellStoreCollection = std::set>; private: struct ChangeCellGridRequest diff --git a/apps/openmw/mwworld/worldmodel.cpp b/apps/openmw/mwworld/worldmodel.cpp index 4f430658c2..149b113b00 100644 --- a/apps/openmw/mwworld/worldmodel.cpp +++ b/apps/openmw/mwworld/worldmodel.cpp @@ -17,6 +17,22 @@ #include "cellstore.hpp" #include "esmstore.hpp" +namespace MWWorld +{ + namespace + { + template + CellStore& emplaceCellStore(ESM::RefId id, const T& cell, ESMStore& store, ESM::ReadersCache& readers, + std::unordered_map& cells) + { + return cells + .emplace(std::piecewise_construct, std::forward_as_tuple(id), + std::forward_as_tuple(Cell(cell), store, readers)) + .first->second; + } + } +} + MWWorld::CellStore& MWWorld::WorldModel::getOrInsertCellStore(const ESM::Cell& cell) { const auto it = mCells.find(cell.mId); @@ -27,7 +43,7 @@ MWWorld::CellStore& MWWorld::WorldModel::getOrInsertCellStore(const ESM::Cell& c MWWorld::CellStore& MWWorld::WorldModel::insertCellStore(const ESM::Cell& cell) { - CellStore& cellStore = mCells.emplace(cell.mId, CellStore(Cell(cell), mStore, mReaders)).first->second; + CellStore& cellStore = emplaceCellStore(cell.mId, cell, mStore, mReaders, mCells); if (cell.mData.mFlags & ESM::Cell::Interior) mInteriors.emplace(cell.mName, &cellStore); else @@ -112,8 +128,7 @@ MWWorld::CellStore& MWWorld::WorldModel::getExterior(ESM::ExteriorCellLocation c cell = mStore.insert(record); } - CellStore* cellStore - = &mCells.emplace(cell->mId, CellStore(MWWorld::Cell(*cell), mStore, mReaders)).first->second; + CellStore* cellStore = &emplaceCellStore(cell->mId, *cell, mStore, mReaders, mCells); result = mExteriors.emplace(cellIndex, cellStore).first; } else @@ -132,8 +147,7 @@ MWWorld::CellStore& MWWorld::WorldModel::getExterior(ESM::ExteriorCellLocation c record.mCellFlags = 0; cell = mStore.insert(record); } - CellStore* cellStore - = &mCells.emplace(cell->mId, CellStore(MWWorld::Cell(*cell), mStore, mReaders)).first->second; + CellStore* cellStore = &emplaceCellStore(cell->mId, *cell, mStore, mReaders, mCells); result = mExteriors.emplace(cellIndex, cellStore).first; } } @@ -152,10 +166,9 @@ MWWorld::CellStore* MWWorld::WorldModel::getInteriorOrNull(std::string_view name { CellStore* newCellStore = nullptr; if (const ESM::Cell* cell = mStore.get().search(name)) - newCellStore = &mCells.emplace(cell->mId, CellStore(MWWorld::Cell(*cell), mStore, mReaders)).first->second; + newCellStore = &emplaceCellStore(cell->mId, *cell, mStore, mReaders, mCells); else if (const ESM4::Cell* cell4 = mStore.get().searchCellName(name)) - newCellStore - = &mCells.emplace(cell4->mId, CellStore(MWWorld::Cell(*cell4), mStore, mReaders)).first->second; + newCellStore = &emplaceCellStore(cell4->mId, *cell4, mStore, mReaders, mCells); if (!newCellStore) return nullptr; // Cell not found result = mInteriors.emplace(name, newCellStore).first; @@ -189,11 +202,11 @@ MWWorld::CellStore& MWWorld::WorldModel::getCell(const ESM::RefId& id, bool forc if (!cell4) { const ESM::Cell* cell = mStore.get().find(id); - newCellStore = &mCells.emplace(cell->mId, CellStore(MWWorld::Cell(*cell), mStore, mReaders)).first->second; + newCellStore = &emplaceCellStore(cell->mId, *cell, mStore, mReaders, mCells); } else { - newCellStore = &mCells.emplace(cell4->mId, CellStore(MWWorld::Cell(*cell4), mStore, mReaders)).first->second; + newCellStore = &emplaceCellStore(cell4->mId, *cell4, mStore, mReaders, mCells); } if (newCellStore->getCell()->isExterior()) {