1
0
Fork 0
mirror of https://github.com/OpenMW/openmw.git synced 2025-01-29 22:45:36 +00:00

refactors a case insensitive map (#3184)

This PR aims to spark the retirement of a questionable pattern I have found all over our code base. I will illustrate how this pattern encourages code duplication, lacks type safety, requires documentation and can be prone to bugs.
```
std::map<std::string, Object> mMap; // Stored in all lowercase for a case-insensitive lookup

std::string lowerKey = Misc::StringUtils::lowerCase(key);
mMap.emplace(lowerKey, object);

std::string lowerKey = Misc::StringUtils::lowerCase(key);
mMap.find(lowerKey);

mMap.find(key); // Not found. Oops!
```
An alternative approach produces no such issues.
```
std::unordered_map<std::string, Object, Misc::StringUtils::CiHash, Misc::StringUtils::CiEqual> mMap;

mMap.emplace(key, object);

mMap.find(key);
```
Of course, such an alternative will work for a `map` as well, but an `unordered_map` is generally preferable over a `map` with these changes because we have moved `lowerCase` into the comparison operator. 

In this PR I have refactored `Animation::mNodeMap` accordingly. I have reviewed and adapted all direct and indirect usage of `Animation::mNodeMap` to ensure we do not change behaviour with this PR.
This commit is contained in:
Bo Svensson 2021-10-25 07:18:26 +00:00 committed by GitHub
parent 6fb376f1cf
commit 7f9beac3a7
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 41 additions and 23 deletions

View file

@ -74,7 +74,7 @@ PartHolderPtr ActorAnimation::attachMesh(const std::string& model, const std::st
osg::ref_ptr<osg::Node> instance = mResourceSystem->getSceneManager()->getInstance(model, parent);
const NodeMap& nodeMap = getNodeMap();
NodeMap::const_iterator found = nodeMap.find(Misc::StringUtils::lowerCase(bonename));
NodeMap::const_iterator found = nodeMap.find(bonename);
if (found == nodeMap.end())
return PartHolderPtr();

View file

@ -1510,7 +1510,7 @@ namespace MWRender
parentNode = mInsert;
else
{
NodeMap::const_iterator found = getNodeMap().find(Misc::StringUtils::lowerCase(bonename));
NodeMap::const_iterator found = getNodeMap().find(bonename);
if (found == getNodeMap().end())
throw std::runtime_error("Can't find bone " + bonename);
@ -1619,8 +1619,7 @@ namespace MWRender
const osg::Node* Animation::getNode(const std::string &name) const
{
std::string lowerName = Misc::StringUtils::lowerCase(name);
NodeMap::const_iterator found = getNodeMap().find(lowerName);
NodeMap::const_iterator found = getNodeMap().find(name);
if (found == getNodeMap().end())
return nullptr;
else

View file

@ -7,8 +7,10 @@
#include <components/sceneutil/textkeymap.hpp>
#include <components/sceneutil/util.hpp>
#include <components/sceneutil/nodecallback.hpp>
#include <components/misc/stringops.hpp>
#include <vector>
#include <unordered_map>
namespace ESM
{
@ -157,6 +159,8 @@ public:
virtual bool updateCarriedLeftVisible(const int weaptype) const { return false; };
typedef std::unordered_map<std::string, osg::ref_ptr<osg::MatrixTransform>, Misc::StringUtils::CiHash, Misc::StringUtils::CiEqual> NodeMap;
protected:
class AnimationTime : public SceneUtil::ControllerSource
{
@ -250,8 +254,6 @@ protected:
std::shared_ptr<AnimationTime> mAnimationTimePtr[sNumBlendMasks];
// Stored in all lowercase for a case-insensitive lookup
typedef std::map<std::string, osg::ref_ptr<osg::MatrixTransform> > NodeMap;
mutable NodeMap mNodeMap;
mutable bool mNodeMapCreated;

View file

@ -126,7 +126,7 @@ void CreatureWeaponAnimation::updatePart(PartHolderPtr& scene, int slot)
if (bonename != "Weapon Bone")
{
const NodeMap& nodeMap = getNodeMap();
NodeMap::const_iterator found = nodeMap.find(Misc::StringUtils::lowerCase(bonename));
NodeMap::const_iterator found = nodeMap.find(bonename);
if (found == nodeMap.end())
bonename = "Weapon Bone";
}
@ -161,7 +161,7 @@ void CreatureWeaponAnimation::updatePart(PartHolderPtr& scene, int slot)
osg::ref_ptr<const osg::Node> node = mResourceSystem->getSceneManager()->getTemplate(itemModel);
const NodeMap& nodeMap = getNodeMap();
NodeMap::const_iterator found = nodeMap.find(Misc::StringUtils::lowerCase(bonename));
NodeMap::const_iterator found = nodeMap.find(bonename);
if (found == nodeMap.end())
throw std::runtime_error("Can't find attachment node " + bonename);
osg::ref_ptr<osg::Node> attached = SceneUtil::attach(node, mObjectRoot, bonename, found->second.get(), mResourceSystem->getSceneManager());

View file

@ -713,7 +713,7 @@ PartHolderPtr NpcAnimation::insertBoundedPart(const std::string& model, const st
osg::ref_ptr<const osg::Node> templateNode = mResourceSystem->getSceneManager()->getTemplate(model);
const NodeMap& nodeMap = getNodeMap();
NodeMap::const_iterator found = nodeMap.find(Misc::StringUtils::lowerCase(bonename));
NodeMap::const_iterator found = nodeMap.find(bonename);
if (found == nodeMap.end())
throw std::runtime_error("Can't find attachment node " + bonename);
@ -813,7 +813,7 @@ bool NpcAnimation::addOrReplaceIndividualPart(ESM::PartReferenceType type, int g
if (weaponBonename != bonename)
{
const NodeMap& nodeMap = getNodeMap();
NodeMap::const_iterator found = nodeMap.find(Misc::StringUtils::lowerCase(weaponBonename));
NodeMap::const_iterator found = nodeMap.find(weaponBonename);
if (found != nodeMap.end())
bonename = weaponBonename;
}

View file

@ -172,14 +172,13 @@ void WeaponAnimation::releaseArrow(MWWorld::Ptr actor, float attackStrength)
}
}
void WeaponAnimation::addControllers(const std::map<std::string, osg::ref_ptr<osg::MatrixTransform> >& nodes,
std::vector<std::pair<osg::ref_ptr<osg::Node>, osg::ref_ptr<osg::Callback>>> &map, osg::Node* objectRoot)
void WeaponAnimation::addControllers(const Animation::NodeMap& nodes, std::vector<std::pair<osg::ref_ptr<osg::Node>, osg::ref_ptr<osg::Callback>>> &map, osg::Node* objectRoot)
{
for (int i=0; i<2; ++i)
{
mSpineControllers[i] = nullptr;
std::map<std::string, osg::ref_ptr<osg::MatrixTransform> >::const_iterator found = nodes.find(i == 0 ? "bip01 spine1" : "bip01 spine2");
Animation::NodeMap::const_iterator found = nodes.find(i == 0 ? "bip01 spine1" : "bip01 spine2");
if (found != nodes.end())
{
osg::Node* node = found->second;

View file

@ -42,8 +42,7 @@ namespace MWRender
void releaseArrow(MWWorld::Ptr actor, float attackStrength);
/// Add WeaponAnimation-related controllers to \a nodes and store the added controllers in \a map.
void addControllers(const std::map<std::string, osg::ref_ptr<osg::MatrixTransform> >& nodes,
std::vector<std::pair<osg::ref_ptr<osg::Node>, osg::ref_ptr<osg::Callback>>>& map, osg::Node* objectRoot);
void addControllers(const Animation::NodeMap& nodes, std::vector<std::pair<osg::ref_ptr<osg::Node>, osg::ref_ptr<osg::Callback>>>& map, osg::Node* objectRoot);
void deleteControllers();

View file

@ -6,6 +6,7 @@
#include <algorithm>
#include <string_view>
#include <iterator>
#include <functional>
#include "utf8stream.hpp"
@ -182,6 +183,21 @@ public:
return out;
}
struct CiEqual
{
bool operator()(const std::string& left, const std::string& right) const
{
return ciEqual(left, right);
}
};
struct CiHash
{
std::size_t operator()(std::string str) const
{
lowerCaseInPlace(str);
return std::hash<std::string>{}(str);
}
};
struct CiComp
{
bool operator()(const std::string& left, const std::string& right) const

View file

@ -51,18 +51,17 @@ namespace SceneUtil
void NodeMapVisitor::apply(osg::MatrixTransform& trans)
{
// Take transformation for first found node in file
std::string originalNodeName = Misc::StringUtils::lowerCase(trans.getName());
// Choose first found node in file
if (trans.libraryName() == std::string("osgAnimation"))
{
std::string nodeName = trans.getName();
// Convert underscores to whitespaces as a workaround for Collada (OpenMW's animation system uses whitespace-separated names)
std::replace(originalNodeName.begin(), originalNodeName.end(), '_', ' ');
std::replace(nodeName.begin(), nodeName.end(), '_', ' ');
mMap.emplace(nodeName, &trans);
}
const std::string nodeName = originalNodeName;
mMap.emplace(nodeName, &trans);
else
mMap.emplace(trans.getName(), &trans);
traverse(trans);
}

View file

@ -4,6 +4,10 @@
#include <osg/MatrixTransform>
#include <osg/NodeVisitor>
#include <unordered_map>
#include <components/misc/stringops.hpp>
// Commonly used scene graph visitors
namespace SceneUtil
{
@ -49,7 +53,7 @@ namespace SceneUtil
class NodeMapVisitor : public osg::NodeVisitor
{
public:
typedef std::map<std::string, osg::ref_ptr<osg::MatrixTransform> > NodeMap;
typedef std::unordered_map<std::string, osg::ref_ptr<osg::MatrixTransform>, Misc::StringUtils::CiHash, Misc::StringUtils::CiEqual> NodeMap;
NodeMapVisitor(NodeMap& map)
: osg::NodeVisitor(osg::NodeVisitor::TRAVERSE_ALL_CHILDREN)