diff --git a/apps/opencs/view/render/actor.cpp b/apps/opencs/view/render/actor.cpp index 271ca2365a..94b82c96cd 100644 --- a/apps/opencs/view/render/actor.cpp +++ b/apps/opencs/view/render/actor.cpp @@ -111,7 +111,7 @@ namespace CSVRender if (!mesh.empty() && node != mNodeMap.end()) { auto instance = sceneMgr->getInstance(mesh); - SceneUtil::attach(instance, mSkeleton, boneName, node->second); + SceneUtil::attach(instance, mSkeleton, boneName, node->second, sceneMgr); } } diff --git a/apps/openmw/mwrender/animation.cpp b/apps/openmw/mwrender/animation.cpp index 10624c8bca..ca76856d48 100644 --- a/apps/openmw/mwrender/animation.cpp +++ b/apps/openmw/mwrender/animation.cpp @@ -1319,10 +1319,10 @@ namespace MWRender cache.insert(std::make_pair(model, created)); - return sceneMgr->createInstance(created); + return sceneMgr->getInstance(created); } else - return sceneMgr->createInstance(found->second); + return sceneMgr->getInstance(found->second); } else { diff --git a/apps/openmw/mwrender/creatureanimation.cpp b/apps/openmw/mwrender/creatureanimation.cpp index 502340d4b9..b64b205961 100644 --- a/apps/openmw/mwrender/creatureanimation.cpp +++ b/apps/openmw/mwrender/creatureanimation.cpp @@ -164,7 +164,7 @@ void CreatureWeaponAnimation::updatePart(PartHolderPtr& scene, int slot) NodeMap::const_iterator found = nodeMap.find(Misc::StringUtils::lowerCase(bonename)); if (found == nodeMap.end()) throw std::runtime_error("Can't find attachment node " + bonename); - osg::ref_ptr attached = SceneUtil::attach(node, mObjectRoot, bonename, found->second.get()); + osg::ref_ptr attached = SceneUtil::attach(node, mObjectRoot, bonename, found->second.get(), mResourceSystem->getSceneManager()); scene.reset(new PartHolder(attached)); diff --git a/apps/openmw/mwrender/npcanimation.cpp b/apps/openmw/mwrender/npcanimation.cpp index 0e100326dd..42215c00bc 100644 --- a/apps/openmw/mwrender/npcanimation.cpp +++ b/apps/openmw/mwrender/npcanimation.cpp @@ -717,7 +717,7 @@ PartHolderPtr NpcAnimation::insertBoundedPart(const std::string& model, const st if (found == nodeMap.end()) throw std::runtime_error("Can't find attachment node " + bonename); - osg::ref_ptr attached = SceneUtil::attach(templateNode, mObjectRoot, bonefilter, found->second); + osg::ref_ptr attached = SceneUtil::attach(templateNode, mObjectRoot, bonefilter, found->second, mResourceSystem->getSceneManager()); if (enchantedGlow) mGlowUpdater = SceneUtil::addEnchantedGlow(attached, mResourceSystem, *glowColor); diff --git a/apps/openmw/mwrender/objectpaging.cpp b/apps/openmw/mwrender/objectpaging.cpp index 5c22c3fc8d..947f23f781 100644 --- a/apps/openmw/mwrender/objectpaging.cpp +++ b/apps/openmw/mwrender/objectpaging.cpp @@ -617,6 +617,10 @@ namespace MWRender pat->setAttitude(nodeAttitude); } + // DO NOT COPY AND PASTE THIS CODE. Cloning osg::Geometry without also cloning its contained Arrays is generally unsafe. + // In this specific case the operation is safe under the following two assumptions: + // - When Arrays are removed or replaced in the cloned geometry, the original Arrays in their place must outlive the cloned geometry regardless. (ensured by TemplateMultiRef) + // - Arrays that we add or replace in the cloned geometry must be explicitely forbidden from reusing BufferObjects of the original geometry. (ensured by needvbo() in optimizer.cpp) copyop.setCopyFlags(merge ? osg::CopyOp::DEEP_COPY_NODES|osg::CopyOp::DEEP_COPY_DRAWABLES : osg::CopyOp::DEEP_COPY_NODES); copyop.mOptimizeBillboards = (size > 1/4.f); copyop.mNodePath.push_back(trans); @@ -645,7 +649,8 @@ namespace MWRender } if (numinstances > 0) { - // add a ref to the original template, to hint to the cache that it's still being used and should be kept in cache + // add a ref to the original template to help verify the safety of shallow cloning operations + // in addition, we hint to the cache that it's still being used and should be kept in cache templateRefs->addRef(cnode); if (pair.second.mNeedCompile) diff --git a/components/resource/scenemanager.cpp b/components/resource/scenemanager.cpp index c5ef957c3e..4184a77c54 100644 --- a/components/resource/scenemanager.cpp +++ b/components/resource/scenemanager.cpp @@ -693,19 +693,33 @@ namespace Resource } } - osg::ref_ptr SceneManager::createInstance(const std::string& name) + osg::ref_ptr SceneManager::getInstance(const std::string& name) { osg::ref_ptr scene = getTemplate(name); - return createInstance(scene); + return getInstance(scene); } - osg::ref_ptr SceneManager::createInstance(const osg::Node *base) + osg::ref_ptr SceneManager::cloneNode(const osg::Node* base) { - osg::ref_ptr cloned = static_cast(base->clone(SceneUtil::CopyOp())); - - // add a ref to the original template, to hint to the cache that it's still being used and should be kept in cache + SceneUtil::CopyOp copyop; + if (const osg::Drawable* drawable = base->asDrawable()) + { + if (drawable->asGeometry()) + { + Log(Debug::Warning) << "SceneManager::cloneNode: attempting to clone osg::Geometry. For safety reasons this will be expensive. Consider avoiding this call."; + copyop.setCopyFlags(copyop.getCopyFlags()|osg::CopyOp::DEEP_COPY_ARRAYS|osg::CopyOp::DEEP_COPY_PRIMITIVES); + } + } + osg::ref_ptr cloned = static_cast(base->clone(copyop)); + // add a ref to the original template to help verify the safety of shallow cloning operations + // in addition, if this node is managed by a cache, we hint to the cache that it's still being used and should be kept in cache cloned->getOrCreateUserDataContainer()->addUserObject(new TemplateRef(base)); + return cloned; + } + osg::ref_ptr SceneManager::getInstance(const osg::Node *base) + { + osg::ref_ptr cloned = cloneNode(base); // we can skip any scene graphs without update callbacks since we know that particle emitters will have an update callback set if (cloned->getNumChildrenRequiringUpdateTraversal() > 0) { @@ -716,11 +730,6 @@ namespace Resource return cloned; } - osg::ref_ptr SceneManager::getInstance(const std::string &name) - { - return createInstance(name); - } - osg::ref_ptr SceneManager::getInstance(const std::string &name, osg::Group* parentNode) { osg::ref_ptr cloned = getInstance(name); diff --git a/components/resource/scenemanager.hpp b/components/resource/scenemanager.hpp index b5d3e453a0..85e012071d 100644 --- a/components/resource/scenemanager.hpp +++ b/components/resource/scenemanager.hpp @@ -132,16 +132,22 @@ namespace Resource /// @note Thread safe. osg::ref_ptr getTemplate(const std::string& name, bool compile=true); - osg::ref_ptr createInstance(const std::string& name); + /// Clone osg::Node safely. + /// @note Thread safe. + static osg::ref_ptr cloneNode(const osg::Node* base); - osg::ref_ptr createInstance(const osg::Node* base); void shareState(osg::ref_ptr node); - /// Get an instance of the given scene template + + /// Clone osg::Node and adjust it according to SceneManager's settings. + /// @note Thread safe. + osg::ref_ptr getInstance(const osg::Node* base); + + /// Instance the given scene template. /// @see getTemplate /// @note Thread safe. osg::ref_ptr getInstance(const std::string& name); - /// Get an instance of the given scene template and immediately attach it to a parent node + /// Instance the given scene template and immediately attach it to a parent node /// @see getTemplate /// @note Not thread safe, unless parentNode is not part of the main scene graph yet. osg::ref_ptr getInstance(const std::string& name, osg::Group* parentNode); diff --git a/components/sceneutil/attach.cpp b/components/sceneutil/attach.cpp index 6690148c74..9dabb282b0 100644 --- a/components/sceneutil/attach.cpp +++ b/components/sceneutil/attach.cpp @@ -13,9 +13,9 @@ #include #include +#include #include "visitor.hpp" -#include "clone.hpp" namespace SceneUtil { @@ -49,10 +49,10 @@ namespace SceneUtil if (!filterMatches(drawable.getName())) return; - osg::Node* node = &drawable; + const osg::Node* node = &drawable; for (auto it = getNodePath().rbegin()+1; it != getNodePath().rend(); ++it) { - osg::Node* parent = *it; + const osg::Node* parent = *it; if (!filterMatches(parent->getName())) break; node = parent; @@ -60,11 +60,11 @@ namespace SceneUtil mToCopy.emplace(node); } - void doCopy() + void doCopy(Resource::SceneManager* sceneManager) { - for (const osg::ref_ptr& node : mToCopy) + for (const osg::ref_ptr& node : mToCopy) { - mParent->addChild(static_cast(node->clone(SceneUtil::CopyOp()))); + mParent->addChild(sceneManager->getInstance(node)); } mToCopy.clear(); } @@ -78,7 +78,7 @@ namespace SceneUtil || (lowerName.size() >= mFilter2.size() && lowerName.compare(0, mFilter2.size(), mFilter2) == 0); } - using NodeSet = std::set>; + using NodeSet = std::set>; NodeSet mToCopy; osg::ref_ptr mParent; @@ -100,7 +100,7 @@ namespace SceneUtil } } - osg::ref_ptr attach(osg::ref_ptr toAttach, osg::Node *master, const std::string &filter, osg::Group* attachNode) + osg::ref_ptr attach(osg::ref_ptr toAttach, osg::Node *master, const std::string &filter, osg::Group* attachNode, Resource::SceneManager* sceneManager) { if (dynamic_cast(toAttach.get())) { @@ -108,7 +108,9 @@ namespace SceneUtil CopyRigVisitor copyVisitor(handle, filter); const_cast(toAttach.get())->accept(copyVisitor); - copyVisitor.doCopy(); + copyVisitor.doCopy(sceneManager); + // add a ref to the original template to hint to the cache that it is still being used and should be kept in cache. + handle->getOrCreateUserDataContainer()->addUserObject(new Resource::TemplateRef(toAttach)); if (handle->getNumChildren() == 1) { @@ -127,7 +129,7 @@ namespace SceneUtil } else { - osg::ref_ptr clonedToAttach = static_cast(toAttach->clone(SceneUtil::CopyOp())); + osg::ref_ptr clonedToAttach = sceneManager->getInstance(toAttach); FindByNameVisitor findBoneOffset("BoneOffset"); clonedToAttach->accept(findBoneOffset); diff --git a/components/sceneutil/attach.hpp b/components/sceneutil/attach.hpp index 806fc53488..f5fc693724 100644 --- a/components/sceneutil/attach.hpp +++ b/components/sceneutil/attach.hpp @@ -10,6 +10,10 @@ namespace osg class Node; class Group; } +namespace Resource +{ + class SceneManager; +} namespace SceneUtil { @@ -19,7 +23,7 @@ namespace SceneUtil /// Otherwise, just attach all of the toAttach scenegraph to the attachment node on the master scenegraph, with no filtering. /// @note The master scene graph is expected to include a skeleton. /// @return A newly created node that is directly attached to the master scene graph - osg::ref_ptr attach(osg::ref_ptr toAttach, osg::Node* master, const std::string& filter, osg::Group* attachNode); + osg::ref_ptr attach(osg::ref_ptr toAttach, osg::Node* master, const std::string& filter, osg::Group* attachNode, Resource::SceneManager *sceneManager); } diff --git a/components/sceneutil/clone.hpp b/components/sceneutil/clone.hpp index 1cf00c9e58..35240cbfba 100644 --- a/components/sceneutil/clone.hpp +++ b/components/sceneutil/clone.hpp @@ -18,6 +18,7 @@ namespace SceneUtil /// @par Defines the cloning behaviour we need: /// * Assigns updated ParticleSystem pointers on cloned emitters and programs. /// * Deep copies RigGeometry and MorphGeometry so they can animate without affecting clones. + /// @warning Avoid using this class directly. The safety of cloning operations depends on the copy flags and the objects involved. Consider using SceneManager::cloneNode for additional safety. /// @warning Do not use an object of this class for more than one copy operation. class CopyOp : public osg::CopyOp { diff --git a/components/sceneutil/morphgeometry.cpp b/components/sceneutil/morphgeometry.cpp index 78be559989..59adbffffe 100644 --- a/components/sceneutil/morphgeometry.cpp +++ b/components/sceneutil/morphgeometry.cpp @@ -1,6 +1,7 @@ #include "morphgeometry.hpp" #include +#include #include @@ -27,11 +28,19 @@ MorphGeometry::MorphGeometry(const MorphGeometry ©, const osg::CopyOp ©o void MorphGeometry::setSourceGeometry(osg::ref_ptr sourceGeom) { + for (unsigned int i=0; i<2; ++i) + mGeometry[i] = nullptr; + mSourceGeometry = sourceGeom; for (unsigned int i=0; i<2; ++i) { + // DO NOT COPY AND PASTE THIS CODE. Cloning osg::Geometry without also cloning its contained Arrays is generally unsafe. + // In this specific case the operation is safe under the following two assumptions: + // - When Arrays are removed or replaced in the cloned geometry, the original Arrays in their place must outlive the cloned geometry regardless. (ensured by TemplateRef) + // - Arrays that we add or replace in the cloned geometry must be explicitely forbidden from reusing BufferObjects of the original geometry. (ensured by vbo below) mGeometry[i] = new osg::Geometry(*mSourceGeometry, osg::CopyOp::SHALLOW_COPY); + mGeometry[i]->getOrCreateUserDataContainer()->addUserObject(new Resource::TemplateRef(mSourceGeometry)); const osg::Geometry& from = *mSourceGeometry; osg::Geometry& to = *mGeometry[i]; diff --git a/components/sceneutil/riggeometry.cpp b/components/sceneutil/riggeometry.cpp index ec00efa535..84b31f4afc 100644 --- a/components/sceneutil/riggeometry.cpp +++ b/components/sceneutil/riggeometry.cpp @@ -3,6 +3,7 @@ #include #include +#include #include #include "skeleton.hpp" @@ -60,12 +61,22 @@ RigGeometry::RigGeometry(const RigGeometry ©, const osg::CopyOp ©op) void RigGeometry::setSourceGeometry(osg::ref_ptr sourceGeometry) { + for (unsigned int i=0; i<2; ++i) + mGeometry[i] = nullptr; + mSourceGeometry = sourceGeometry; for (unsigned int i=0; i<2; ++i) { const osg::Geometry& from = *sourceGeometry; + + // DO NOT COPY AND PASTE THIS CODE. Cloning osg::Geometry without also cloning its contained Arrays is generally unsafe. + // In this specific case the operation is safe under the following two assumptions: + // - When Arrays are removed or replaced in the cloned geometry, the original Arrays in their place must outlive the cloned geometry regardless. (ensured by mSourceGeometry) + // - Arrays that we add or replace in the cloned geometry must be explicitely forbidden from reusing BufferObjects of the original geometry. (ensured by vbo below) mGeometry[i] = new osg::Geometry(from, osg::CopyOp::SHALLOW_COPY); + mGeometry[i]->getOrCreateUserDataContainer()->addUserObject(new Resource::TemplateRef(mSourceGeometry)); + osg::Geometry& to = *mGeometry[i]; to.setSupportsDisplayList(false); to.setUseVertexBufferObjects(true); diff --git a/components/sceneutil/riggeometry.hpp b/components/sceneutil/riggeometry.hpp index e01583399e..25ae5a3243 100644 --- a/components/sceneutil/riggeometry.hpp +++ b/components/sceneutil/riggeometry.hpp @@ -9,6 +9,13 @@ namespace SceneUtil class Skeleton; class Bone; + // TODO: This class has a lot of issues. + // - We require too many workarounds to ensure safety. + // - mSourceGeometry should be const, but can not be const because of a use case in shadervisitor.cpp. + // - We create useless mGeometry clones in template RigGeometries. + // - We do not support compileGLObjects. + // - We duplicate some code in MorphGeometry. + /// @brief Mesh skinning implementation. /// @note A RigGeometry may be attached directly to a Skeleton, or somewhere below a Skeleton. /// Note though that the RigGeometry ignores any transforms below the Skeleton, so the attachment point is not that important.