Applies review advice

2d coord hash moved to hash.hpp file
format version adds suffix to be more coherent
don't use ESM::RefId::sEmpty
RefId equality with string_view, conversion to refId unecessary
action teleport remove test that mCellId is empty
removes some const references, when copy is enough
invalid refid => empty refid
removes useless change

apps/openmw/mwbase/world.hpp

@@ -512,15 +512,15 @@ namespace MWBase
         virtual bool screenshot360(osg::Image* image) = 0;
 
         /// Find default position inside exterior cell specified by name
-        /// \return invalid RefId if exterior with given name not exists, the cell's RefId otherwise
+        /// \return empty RefId if exterior with given name not exists, the cell's RefId otherwise
         virtual ESM::RefId findExteriorPosition(std::string_view name, ESM::Position& pos) = 0;
 
         /// Find default position inside interior cell specified by name
-        /// \return invalid RefId if interior with given name not exists, the cell's RefId otherwise
+        /// \return empty RefId if interior with given name not exists, the cell's RefId otherwise
         virtual ESM::RefId findInteriorPosition(std::string_view name, ESM::Position& pos) = 0;
 
         /// Find default position inside interior or exterior cell specified by name
-        /// \return invalid RefId if interior with given name not exists, the cell's RefId otherwise
+        /// \return empty RefId if interior with given name not exists, the cell's RefId otherwise
         virtual ESM::RefId findCellPosition(std::string_view cellName, ESM::Position& pos) = 0;
         /// Enables or disables use of teleport spell effects (recall, intervention, etc).
         virtual void enableTeleporting(bool enable) = 0;

apps/openmw/mwmechanics/aifollow.cpp

@@ -174,8 +174,7 @@ namespace MWMechanics
                         return true;
                     }
                 }
-                else if (Misc::StringUtils::ciEqual(mCellId,
-                             actor.getCell()->getCell()->getWorldSpace().toString())) // Cell to travel to
+                else if (mCellId == actor.getCell()->getCell()->getWorldSpace()) // Cell to travel to
                 {
                     mRemainingDuration = mDuration;
                     return true;

apps/openmw/mwmechanics/aipackage.cpp

@@ -330,7 +330,7 @@ void MWMechanics::AiPackage::openDoors(const MWWorld::Ptr& actor)
 
 const MWMechanics::PathgridGraph& MWMechanics::AiPackage::getPathGridGraph(const MWWorld::CellStore* cell)
 {
-    const ESM::RefId& id = cell->getCell()->getId();
+    const ESM::RefId id = cell->getCell()->getId();
     // static cache is OK for now, pathgrids can never change during runtime
     typedef std::map<ESM::RefId, std::unique_ptr<MWMechanics::PathgridGraph>> CacheMap;
     static CacheMap cache;

apps/openmw/mwstate/statemanagerimp.cpp

@@ -554,7 +554,7 @@ void MWState::StateManager::loadGame(const Character* character, const std::file
 
         if (ptr.isInCell())
         {
-            const ESM::RefId& cellId = ptr.getCell()->getCell()->getId();
+            const ESM::RefId cellId = ptr.getCell()->getCell()->getId();
 
             // Use detectWorldSpaceChange=false, otherwise some of the data we just loaded would be cleared again
             MWBase::Environment::get().getWorld()->changeToCell(cellId, ptr.getRefData().getPosition(), false, false);

apps/openmw/mwworld/actionteleport.cpp

@@ -54,8 +54,7 @@ namespace MWWorld
         if (actor == world->getPlayerPtr())
         {
             world->getPlayer().setTeleported(true);
-            if (!mCellId.empty())
-                world->changeToCell(mCellId, mPosition, true);
+            world->changeToCell(mCellId, mPosition, true);
             teleported = world->getPlayerPtr();
         }
         else

apps/openmw/mwworld/scene.cpp

@@ -752,7 +752,7 @@ namespace MWWorld
             {
                 assert(!(*iter)->getCell()->isExterior());
 
-                if (it->mName == (*iter)->getCell()->getWorldSpace().toString())
+                if (it->mName == (*iter)->getCell()->getNameId())
                 {
                     unloadCell(*iter, navigatorUpdateGuard.get());
                     break;

apps/openmw/mwworld/worldimp.cpp

@@ -2801,7 +2801,7 @@ namespace MWWorld
                 // and use its destination to position inside cell.
                 for (const MWWorld::LiveCellRef<ESM::Door>& destDoor : source->getReadOnlyDoors().mList)
                 {
-                    if (ESM::RefId::stringRefId(name) == destDoor.mRef.getDestCell())
+                    if (name == destDoor.mRef.getDestCell())
                     {
                         /// \note Using _any_ door pointed to the interior,
                         /// not the one pointed to current door.
@@ -2869,7 +2869,7 @@ namespace MWWorld
         {
             ext = mWorldModel.getCell(nameId)->getCell();
             if (!ext->isExterior())
-                return ESM::RefId::sEmpty;
+                return ESM::RefId();
         }
         catch (std::exception&)
         {

apps/openmw_test_suite/mwworld/test_store.cpp

@@ -285,7 +285,7 @@ namespace
             ESM::MaxOldSkillsAndAttributesFormatVersion,
             ESM::MaxOldCreatureStatsFormatVersion,
             ESM::MaxStringRefIdFormatVersion,
-            ESM::MaxUseEsmCellId,
+            ESM::MaxUseEsmCellIdFormatVersion,
         });
         for (ESM::FormatVersion v = result.back() + 1; v <= ESM::CurrentSaveGameFormatVersion; ++v)
             result.push_back(v);

components/CMakeLists.txt

@@ -392,7 +392,7 @@ if (USE_QT)
         )
 
     add_component_qt_dir (misc
-        helpviewer utf8qtextstream
+        helpviewer utf8qtextstream hash
         )
 
     add_component_qt_dir (files

components/esm/esm3exteriorcellrefid.hpp

@@ -6,6 +6,8 @@
 
 #include <utility>
 
+#include <components/misc/hash.hpp>
+
 namespace ESM
 {
     class ESM3ExteriorCellRefId
@@ -47,7 +49,7 @@ namespace std
     {
         std::size_t operator()(ESM::ESM3ExteriorCellRefId value) const noexcept
         {
-            return (53 + std::hash<int32_t>{}(value.mX)) * 53 + std::hash<int32_t>{}(value.mY);
+            return Misc::hash2dCoord(value.mX, value.mY);
         }
     };
 }

components/esm3/esmreader.cpp

@@ -90,7 +90,7 @@ namespace ESM
 
     ESM::RefId ESMReader::getCellId()
     {
-        if (mHeader.mFormatVersion <= ESM::MaxUseEsmCellId)
+        if (mHeader.mFormatVersion <= ESM::MaxUseEsmCellIdFormatVersion)
         {
             ESM::CellId cellId;
             cellId.load(*this);

components/esm3/esmwriter.cpp

@@ -246,7 +246,7 @@ namespace ESM
 
     void ESMWriter::writeCellId(const ESM::RefId& cellId)
     {
-        if (mHeader.mFormatVersion <= ESM::MaxUseEsmCellId)
+        if (mHeader.mFormatVersion <= ESM::MaxUseEsmCellIdFormatVersion)
         {
             ESM::CellId generatedCellid = ESM::CellId::extractFromRefId(cellId);
             generatedCellid.save(*this);

components/esm3/formatversion.hpp

@@ -23,7 +23,7 @@ namespace ESM
     inline constexpr FormatVersion MaxStringRefIdFormatVersion = 23;
     inline constexpr FormatVersion MaxSavedGameCellNameAsRefIdFormatVersion = 24;
     inline constexpr FormatVersion MaxNameIsRefIdOnlyFormatVersion = 25;
-    inline constexpr FormatVersion MaxUseEsmCellId = 26;
+    inline constexpr FormatVersion MaxUseEsmCellIdFormatVersion = 26;
     inline constexpr FormatVersion CurrentSaveGameFormatVersion = 27;
 }
 

components/misc/hash.hpp

@@ -15,6 +15,13 @@ namespace Misc
         std::hash<T> hasher;
         seed ^= static_cast<Seed>(hasher(v) + 0x9e3779b9 + (seed << 6) + (seed >> 2));
     }
+
+    // Comes from https://stackoverflow.com/questions/2634690/good-hash-function-for-a-2d-index
+    // Effective Java (2nd edition) is cited as the source
+    inline std::size_t hash2dCoord(int32_t x, int32_t y)
+    {
+        return (53 + std::hash<int32_t>{}(x)) * 53 + std::hash<int32_t>{}(y);
+    }
 }
 
 #endif