FGFS-4.1/OpenSceneGraph
4
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

From Jannik Heller, "I've hit what I believe to be a bug (or at the very least, an unintuitive behaviour) in the osg::Geometry copy constructor. I noticed it when using osg::clone on a Geometry with vertex buffer objects, and the copy flags DEEP_COPY_ARRAYS. To be precise, I add a Geometry to an osgUtil::IncrementalCompileOperation, then osg::clone the Geometry. I was getting reports from users of random crashes happening.

Browse Source 
I believe the offending lines are in the osg::Geometry copy constructor:

    if ((copyop.getCopyFlags() & osg::CopyOp::DEEP_COPY_ARRAYS))
    {
        if (_useVertexBufferObjects)
        {
            // copying of arrays doesn't set up buffer objects so we'll need to force
            // Geometry to assign these, we'll do this by switching off VBO's then renabling them.
            setUseVertexBufferObjects(false);
            setUseVertexBufferObjects(true);
        }
    }

Toggling the vertex buffer objects off then on again actually touches not only the arrays controlled by DEEP_COPY_ARRAYS, but also the PrimitiveSets which are controlled by DEEP_COPY_PRIMITIVES. This means if the user has copyflags of only DEEP_COPY_ARRAYS, we are modifying arrays that belong to the original const Geometry& we are copying from. I believe this shouldn't be allowed to happen because we are using a const& specifier for the original Geometry.

In my case the osgUtil::IncrementalCompileOperation was trying to compile the geometry, while in the main thread a clone operation toggled the VBO's off and on, a crash ensues.

In the attached patch, you will find a more efficient handling of VBO's in the osg::Geometry copy constructor, so that only the Arrays that were actually deep copied have their VBO assigned, and no changes are made to Arrays that already had a valid VBO assigned. In addition, the DEEP_COPY_PRIMITIVES flag is now honored so that VBO's are set up correctly should a user copy a Geometry with only that flag.
"



git-svn-id: http://svn.openscenegraph.org/osg/OpenSceneGraph/trunk@15129 16af8721-9629-0410-8352-f15c8da7e697
This commit is contained in:
Robert Osfield
2015-09-04 15:35:24 +00:00
parent 34794c5c27
commit cb3396b0e5
1 changed files with 4 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
src/osg/Geometry.cpp
View File

@@ -62,13 +62,14 @@ Geometry::Geometry(const Geometry& geometry,const CopyOp& copyop):
_vertexAttribList.push_back(copyop(vitr->get()));
}
if ((copyop.getCopyFlags() & osg::CopyOp::DEEP_COPY_ARRAYS))
if ((copyop.getCopyFlags() & osg::CopyOp::DEEP_COPY_ARRAYS) || (copyop.getCopyFlags() & osg::CopyOp::DEEP_COPY_PRIMITIVES))
{
if (_useVertexBufferObjects)
{
// copying of arrays doesn't set up buffer objects so we'll need to force
// Geometry to assign these, we'll do this by switching off VBO's then renabling them.
setUseVertexBufferObjects(false);
// Geometry to assign these, we'll do this by changing the cached value to false then re-enabling.
// note do not use setUseVertexBufferObjects(false) as it might modify Arrays that we have not deep-copied.
_useVertexBufferObjects = false;
setUseVertexBufferObjects(true);
}
}