Move double buffering implementation inside RigGeometry

The double buffering is an implementation detail so it should be handled as such, rather than mandating the scene graph to be structured in a certain way.

Override accept(NodeVisitor&) instead of using callbacks.

components/nifosg/nifloader.cpp

@@ -1236,7 +1236,6 @@ namespace NifOsg
 
                 SceneUtil::RigGeometry::BoneInfluence influence;
                 const std::vector<Nif::NiSkinData::VertWeight> &weights = data->bones[i].weights;
-                //influence.mWeights.reserve(weights.size());
                 for(size_t j = 0;j < weights.size();j++)
                 {
                     std::pair<unsigned short, float> indexWeight = std::make_pair(weights[j].vertex, weights[j].weight);
@@ -1249,17 +1248,7 @@ namespace NifOsg
             }
             rig->setInfluenceMap(map);
 
-            // Add a copy, we will alternate between the two copies every other frame using the FrameSwitch
+            parentNode->addChild(rig);
-            // This is so we can set the DataVariance as STATIC, giving a huge performance boost
-            rig->setDataVariance(osg::Object::STATIC);
-
-            osg::ref_ptr<FrameSwitch> frameswitch = new FrameSwitch;
-
-            SceneUtil::RigGeometry* rig2 = osg::clone(rig.get(), osg::CopyOp::DEEP_COPY_NODES|osg::CopyOp::DEEP_COPY_DRAWABLES);
-            frameswitch->addChild(rig);
-            frameswitch->addChild(rig2);
-
-            parentNode->addChild(frameswitch);
         }
 
         osg::BlendFunc::BlendFuncMode getBlendMode(int mode)

components/sceneutil/attach.cpp

@@ -43,13 +43,13 @@ namespace SceneUtil
             traverse(node);
         }
 
-        virtual void apply(osg::Geometry& geom)
+        virtual void apply(osg::Drawable& drawable)
         {
-            std::string lowerName = Misc::StringUtils::lowerCase(geom.getName());
+            std::string lowerName = Misc::StringUtils::lowerCase(drawable.getName());
             if ((lowerName.size() >= mFilter.size() && lowerName.compare(0, mFilter.size(), mFilter) == 0)
                     || (lowerName.size() >= mFilter2.size() && lowerName.compare(0, mFilter2.size(), mFilter2) == 0))
             {
-                osg::Node* node = &geom;
+                osg::Node* node = &drawable;
                 while (node && node->getNumParents() && !node->getStateSet())
                     node = node->getParent(0);
                 if (node)

components/sceneutil/riggeometry.cpp

@@ -10,73 +10,17 @@
 namespace SceneUtil
 {
 
-class UpdateRigBounds : public osg::Drawable::UpdateCallback
-{
-public:
-    UpdateRigBounds()
-    {
-    }
-
-    UpdateRigBounds(const UpdateRigBounds& copy, const osg::CopyOp& copyop)
-        : osg::Drawable::UpdateCallback(copy, copyop)
-    {
-    }
-
-    META_Object(SceneUtil, UpdateRigBounds)
-
-    void update(osg::NodeVisitor* nv, osg::Drawable* drw)
-    {
-        RigGeometry* rig = static_cast<RigGeometry*>(drw);
-
-        rig->updateBounds(nv);
-    }
-};
-
-// TODO: make threadsafe for multiple cull threads
-class UpdateRigGeometry : public osg::Drawable::CullCallback
-{
-public:
-    UpdateRigGeometry()
-    {
-    }
-
-    UpdateRigGeometry(const UpdateRigGeometry& copy, const osg::CopyOp& copyop)
-        : osg::Drawable::CullCallback(copy, copyop)
-    {
-    }
-
-    META_Object(SceneUtil, UpdateRigGeometry)
-
-    virtual bool cull(osg::NodeVisitor* nv, osg::Drawable* drw, osg::State*) const
-    {
-        RigGeometry* geom = static_cast<RigGeometry*>(drw);
-        geom->update(nv);
-        return false;
-    }
-};
-
-// We can't compute the bounds without a NodeVisitor, since we need the current geomToSkelMatrix.
-// So we return nothing. Bounds are updated every frame in the UpdateCallback.
-class DummyComputeBoundCallback : public osg::Drawable::ComputeBoundingBoxCallback
-{
-public:
-    virtual osg::BoundingBox computeBound(const osg::Drawable&) const  { return osg::BoundingBox(); }
-};
-
 RigGeometry::RigGeometry()
     : mSkeleton(NULL)
     , mLastFrameNumber(0)
     , mBoundsFirstFrame(true)
 {
-    setCullCallback(new UpdateRigGeometry);
+    setUpdateCallback(new osg::Callback); // dummy to make sure getNumChildrenRequiringUpdateTraversal() is correct
-    setUpdateCallback(new UpdateRigBounds);
+                                          // update done in accept(NodeVisitor&)
-    setSupportsDisplayList(false);
-    setUseVertexBufferObjects(true);
-    setComputeBoundingBoxCallback(new DummyComputeBoundCallback);
 }
 
 RigGeometry::RigGeometry(const RigGeometry &copy, const osg::CopyOp &copyop)
-    : osg::Geometry(copy, copyop)
+    : Drawable(copy, copyop)
     , mSkeleton(NULL)
     , mInfluenceMap(copy.mInfluenceMap)
     , mLastFrameNumber(0)
@@ -89,24 +33,14 @@ void RigGeometry::setSourceGeometry(osg::ref_ptr<osg::Geometry> sourceGeometry)
 {
     mSourceGeometry = sourceGeometry;
 
+    for (unsigned int i=0; i<2; ++i)
+    {
         osg::Geometry& from = *sourceGeometry;
-
+        mGeometry[i] = new osg::Geometry(from, osg::CopyOp::SHALLOW_COPY);
-    if (from.getStateSet())
+        osg::Geometry& to = *mGeometry[i];
-        setStateSet(from.getStateSet());
+        to.setSupportsDisplayList(false);
-
+        to.setUseVertexBufferObjects(true);
-    // shallow copy primitive sets & vertex attributes that we will not modify
+        to.setCullingActive(false); // make sure to disable culling since that's handled by this class
-    setPrimitiveSetList(from.getPrimitiveSetList());
-    setColorArray(from.getColorArray());
-    setSecondaryColorArray(from.getSecondaryColorArray());
-    setFogCoordArray(from.getFogCoordArray());
-
-    // need to copy over texcoord list manually due to a missing null pointer check in setTexCoordArrayList(), this has been fixed in OSG 3.5
-    osg::Geometry::ArrayList& texCoordList = from.getTexCoordArrayList();
-    for (unsigned int i=0; i<texCoordList.size(); ++i)
-        if (texCoordList[i])
-            setTexCoordArray(i, texCoordList[i], osg::Array::BIND_PER_VERTEX);
-
-    setVertexAttribArrayList(from.getVertexAttribArrayList());
 
         // vertices and normals are modified every frame, so we need to deep copy them.
         // assign a dedicated VBO to make sure that modifications don't interfere with source geometry's VBO.
@@ -117,7 +51,7 @@ void RigGeometry::setSourceGeometry(osg::ref_ptr<osg::Geometry> sourceGeometry)
         if (vertexArray)
         {
             vertexArray->setVertexBufferObject(vbo);
-        setVertexArray(vertexArray);
+            to.setVertexArray(vertexArray);
         }
 
         if (osg::Array* normals = from.getNormalArray())
@@ -126,20 +60,20 @@ void RigGeometry::setSourceGeometry(osg::ref_ptr<osg::Geometry> sourceGeometry)
             if (normalArray)
             {
                 normalArray->setVertexBufferObject(vbo);
-            setNormalArray(normalArray, osg::Array::BIND_PER_VERTEX);
+                to.setNormalArray(normalArray, osg::Array::BIND_PER_VERTEX);
             }
         }
 
-
         if (osg::Vec4Array* tangents = dynamic_cast<osg::Vec4Array*>(from.getTexCoordArray(7)))
         {
             mSourceTangents = tangents;
             osg::ref_ptr<osg::Array> tangentArray = osg::clone(tangents, osg::CopyOp::DEEP_COPY_ALL);
             tangentArray->setVertexBufferObject(vbo);
-        setTexCoordArray(7, tangentArray, osg::Array::BIND_PER_VERTEX);
+            to.setTexCoordArray(7, tangentArray, osg::Array::BIND_PER_VERTEX);
         }
         else
             mSourceTangents = NULL;
+    }
 }
 
 osg::ref_ptr<osg::Geometry> RigGeometry::getSourceGeometry()
@@ -228,7 +162,7 @@ void accumulateMatrix(const osg::Matrixf& invBindMatrix, const osg::Matrixf& mat
     ptrresult[14] += ptr[14] * weight;
 }
 
-void RigGeometry::update(osg::NodeVisitor* nv)
+void RigGeometry::cull(osg::NodeVisitor* nv)
 {
     if (!mSkeleton)
     {
@@ -238,23 +172,24 @@ void RigGeometry::update(osg::NodeVisitor* nv)
             return;
     }
 
-    if (!mSkeleton->getActive() && mLastFrameNumber != 0)
+    if ((!mSkeleton->getActive() && mLastFrameNumber != 0) || mLastFrameNumber == nv->getTraversalNumber())
-        return;
+    {
-
+        nv->apply(*getGeometry(mLastFrameNumber));
-    if (mLastFrameNumber == nv->getTraversalNumber())
         return;
+    }
     mLastFrameNumber = nv->getTraversalNumber();
+    osg::Geometry& geom = *getGeometry(mLastFrameNumber);
 
     mSkeleton->updateBoneMatrices(nv->getTraversalNumber());
 
     // skinning
-    osg::Vec3Array* positionSrc = static_cast<osg::Vec3Array*>(mSourceGeometry->getVertexArray());
+    const osg::Vec3Array* positionSrc = static_cast<osg::Vec3Array*>(mSourceGeometry->getVertexArray());
-    osg::Vec3Array* normalSrc = static_cast<osg::Vec3Array*>(mSourceGeometry->getNormalArray());
+    const osg::Vec3Array* normalSrc = static_cast<osg::Vec3Array*>(mSourceGeometry->getNormalArray());
-    osg::Vec4Array* tangentSrc = mSourceTangents;
+    const osg::Vec4Array* tangentSrc = mSourceTangents;
 
-    osg::Vec3Array* positionDst = static_cast<osg::Vec3Array*>(getVertexArray());
+    osg::Vec3Array* positionDst = static_cast<osg::Vec3Array*>(geom.getVertexArray());
-    osg::Vec3Array* normalDst = static_cast<osg::Vec3Array*>(getNormalArray());
+    osg::Vec3Array* normalDst = static_cast<osg::Vec3Array*>(geom.getNormalArray());
-    osg::Vec4Array* tangentDst = static_cast<osg::Vec4Array*>(getTexCoordArray(7));
+    osg::Vec4Array* tangentDst = static_cast<osg::Vec4Array*>(geom.getTexCoordArray(7));
 
     for (Bone2VertexMap::const_iterator it = mBone2VertexMap.begin(); it != mBone2VertexMap.end(); ++it)
     {
@@ -294,6 +229,10 @@ void RigGeometry::update(osg::NodeVisitor* nv)
         normalDst->dirty();
     if (tangentDst)
         tangentDst->dirty();
+
+    nv->pushOntoNodePath(&geom);
+    nv->apply(geom);
+    nv->popFromNodePath();
 }
 
 void RigGeometry::updateBounds(osg::NodeVisitor *nv)
@@ -365,5 +304,29 @@ void RigGeometry::setInfluenceMap(osg::ref_ptr<InfluenceMap> influenceMap)
     mInfluenceMap = influenceMap;
 }
 
+void RigGeometry::accept(osg::NodeVisitor &nv)
+{
+    if (!nv.validNodeMask(*this))
+        return;
+
+    nv.pushOntoNodePath(this);
+
+    if (nv.getVisitorType() == osg::NodeVisitor::CULL_VISITOR)
+        cull(&nv);
+    else if (nv.getVisitorType() == osg::NodeVisitor::UPDATE_VISITOR)
+        updateBounds(&nv);
+    else if (nv.getVisitorType() == osg::NodeVisitor::INTERSECTION_VISITOR)
+        nv.apply(*getGeometry(mLastFrameNumber));
+    else
+        nv.apply(*this);
+
+    nv.popFromNodePath();
+}
+
+osg::Geometry* RigGeometry::getGeometry(unsigned int frame) const
+{
+    return mGeometry[frame%2].get();
+}
+
 
 }

components/sceneutil/riggeometry.hpp

@@ -13,10 +13,9 @@ namespace SceneUtil
     /// @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.
-    /// @note To avoid race conditions, the rig geometry needs to be double buffered. This can be done
+    /// @note The internal Geometry used for rendering is double buffered, this allows updates to be done in a thread safe way while
-    /// using a FrameSwitch node that has two RigGeometry children. In the future we may want to consider implementing
+    /// not compromising rendering performance. This is crucial when using osg's default threading model of DrawThreadPerContext.
-    /// the double buffering inside RigGeometry.
+    class RigGeometry : public osg::Drawable
-    class RigGeometry : public osg::Geometry
     {
     public:
         RigGeometry();
@@ -24,6 +23,9 @@ namespace SceneUtil
 
         META_Object(SceneUtil, RigGeometry)
 
+        // At this point compileGLObjects() remains unimplemented, hard to avoid race conditions
+        // and there is limited value in compiling anyway since the data will change again for the next frame
+
         struct BoneInfluence
         {
             osg::Matrixf mInvBindMatrix;
@@ -45,13 +47,15 @@ namespace SceneUtil
 
         osg::ref_ptr<osg::Geometry> getSourceGeometry();
 
-        // Called automatically by our CullCallback
+        virtual void accept(osg::NodeVisitor &nv);
-        void update(osg::NodeVisitor* nv);
-
-        // Called automatically by our UpdateCallback
-        void updateBounds(osg::NodeVisitor* nv);
 
     private:
+        void cull(osg::NodeVisitor* nv);
+        void updateBounds(osg::NodeVisitor* nv);
+
+        osg::ref_ptr<osg::Geometry> mGeometry[2];
+        osg::Geometry* getGeometry(unsigned int frame) const;
+
         osg::ref_ptr<osg::Geometry> mSourceGeometry;
         osg::ref_ptr<osg::Vec4Array> mSourceTangents;
         Skeleton* mSkeleton;

components/sceneutil/serialize.cpp

@@ -50,7 +50,7 @@ class RigGeometrySerializer : public osgDB::ObjectWrapper
 {
 public:
     RigGeometrySerializer()
-        : osgDB::ObjectWrapper(createInstanceFunc<SceneUtil::RigGeometry>, "SceneUtil::RigGeometry", "osg::Object osg::Node osg::Drawable osg::Geometry SceneUtil::RigGeometry")
+        : osgDB::ObjectWrapper(createInstanceFunc<SceneUtil::RigGeometry>, "SceneUtil::RigGeometry", "osg::Object osg::Node osg::Drawable SceneUtil::RigGeometry")
     {
     }
 };

components/sceneutil/skeleton.cpp

@@ -38,8 +38,6 @@ Skeleton::Skeleton()
     , mNeedToUpdateBoneMatrices(true)
     , mActive(true)
     , mLastFrameNumber(0)
-    , mTraversedEvenFrame(false)
-    , mTraversedOddFrame(false)
 {
 
 }
@@ -50,8 +48,6 @@ Skeleton::Skeleton(const Skeleton &copy, const osg::CopyOp &copyop)
     , mNeedToUpdateBoneMatrices(true)
     , mActive(copy.mActive)
     , mLastFrameNumber(0)
-    , mTraversedEvenFrame(false)
-    , mTraversedOddFrame(false)
 {
 
 }
@@ -115,11 +111,6 @@ void Skeleton::updateBoneMatrices(unsigned int traversalNumber)
 
     mLastFrameNumber = traversalNumber;
 
-    if (mLastFrameNumber % 2 == 0)
-        mTraversedEvenFrame = true;
-    else
-        mTraversedOddFrame = true;
-
     if (mNeedToUpdateBoneMatrices)
     {
         if (mRootBone.get())
@@ -144,18 +135,14 @@ bool Skeleton::getActive() const
 
 void Skeleton::markDirty()
 {
-    mTraversedEvenFrame = false;
+    mLastFrameNumber = 0;
-    mTraversedOddFrame = false;
     mBoneCache.clear();
     mBoneCacheInit = false;
 }
 
 void Skeleton::traverse(osg::NodeVisitor& nv)
 {
-    if (!getActive() && nv.getVisitorType() == osg::NodeVisitor::UPDATE_VISITOR
+    if (!getActive() && nv.getVisitorType() == osg::NodeVisitor::UPDATE_VISITOR && mLastFrameNumber != 0)
-            // need to process at least 2 frames before shutting off update, since we need to have both frame-alternating RigGeometries initialized
-            // this would be more naturally handled if the double-buffering was implemented in RigGeometry itself rather than in a FrameSwitch decorator node
-            && mLastFrameNumber != 0 && mTraversedEvenFrame && mTraversedOddFrame)
         return;
     osg::Group::traverse(nv);
 }

components/sceneutil/skeleton.hpp

@@ -74,8 +74,6 @@ namespace SceneUtil
         bool mActive;
 
         unsigned int mLastFrameNumber;
-        bool mTraversedEvenFrame;
-        bool mTraversedOddFrame;
     };
 
 }