Changeset 14037

Show
Ignore:
Timestamp:
09/04/15 17:35:24 (21 hours ago)
Author:
robert
Message:

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.

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.
"

Location:
OpenSceneGraph/trunk
Files:
2 modified

Legend:

Unmodified
Added
Removed
  • OpenSceneGraph/trunk/include/osg/ComputeBoundsVisitor

    r13307 r14037  
    4949    void applyDrawable(osg::Drawable* drawable); 
    5050 
    51 protected: 
     51    void applyBoundingBox(const osg::BoundingBox&); 
    5252 
    5353    typedef std::vector<osg::Matrix> MatrixStack; 
    5454 
     55    const MatrixStack& getMatrixStack() const { return _matrixStack; } 
     56 
     57protected: 
    5558    MatrixStack         _matrixStack; 
    5659    osg::BoundingBox    _bb; 
  • OpenSceneGraph/trunk/src/osg/ComputeBoundsVisitor.cpp

    r13041 r14037  
    7878void ComputeBoundsVisitor::applyDrawable(osg::Drawable* drawable) 
    7979{ 
    80     if (_matrixStack.empty()) _bb.expandBy(drawable->getBound()); 
    81     else 
     80    applyBoundingBox(drawable->getBound()); 
     81} 
     82 
     83void ComputeBoundsVisitor::applyBoundingBox(const osg::BoundingBox& bbox) 
     84{ 
     85    if (_matrixStack.empty()) _bb.expandBy(bbox); 
     86    else if (bbox.valid()) 
    8287    { 
    83         osg::Matrix& matrix = _matrixStack.back(); 
    84         const osg::BoundingBox& dbb = drawable->getBound(); 
    85         if (dbb.valid()) 
    86         { 
    87             _bb.expandBy(dbb.corner(0) * matrix); 
    88             _bb.expandBy(dbb.corner(1) * matrix); 
    89             _bb.expandBy(dbb.corner(2) * matrix); 
    90             _bb.expandBy(dbb.corner(3) * matrix); 
    91             _bb.expandBy(dbb.corner(4) * matrix); 
    92             _bb.expandBy(dbb.corner(5) * matrix); 
    93             _bb.expandBy(dbb.corner(6) * matrix); 
    94             _bb.expandBy(dbb.corner(7) * matrix); 
    95         } 
     88        const osg::Matrix& matrix = _matrixStack.back(); 
     89        _bb.expandBy(bbox.corner(0) * matrix); 
     90        _bb.expandBy(bbox.corner(1) * matrix); 
     91        _bb.expandBy(bbox.corner(2) * matrix); 
     92        _bb.expandBy(bbox.corner(3) * matrix); 
     93        _bb.expandBy(bbox.corner(4) * matrix); 
     94        _bb.expandBy(bbox.corner(5) * matrix); 
     95        _bb.expandBy(bbox.corner(6) * matrix); 
     96        _bb.expandBy(bbox.corner(7) * matrix); 
    9697    } 
    9798}