Show
Ignore:
Timestamp:
09/04/15 17:35:24 (6 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.
"

Files:
1 modified

Legend:

Unmodified
Added
Removed
  • OpenSceneGraph/trunk/examples/osgimpostor/osgimpostor.cpp

    r7648 r13488  
    4444 
    4545// 
    46 osg::Group * Root = 0; 
     46osg::ref_ptr<osg::Group> Root = 0; 
    4747 
    4848const int HOUSES_SIZE = 25000;        // total number of houses 
     
    8282 
    8383    // use the same color, normal and indices for all houses. 
    84         osg::Vec4Array* colors = new osg::Vec4Array(1); 
     84    osg::ref_ptr<osg::Vec4Array> colors = new osg::Vec4Array(1); 
    8585    (*colors)[0] = osg::Vec4(1.0f, 1.0f, 1.0f, 1.0f); 
    8686 
    8787    // normals 
    88     osg::Vec3Array * normals = new osg::Vec3Array(16); 
     88    osg::ref_ptr<osg::Vec3Array> normals = new osg::Vec3Array(16); 
    8989    (*normals)[0] = osg::Vec3( 0.0f,  -0.0f, -1.0f); 
    9090    (*normals)[1] = osg::Vec3( 0.0f,  -0.0f, -1.0f); 
     
    105105 
    106106    // coordIndices 
    107     osg::UByteArray* coordIndices = new osg::UByteArray(48,indices); 
    108  
    109         // share the primitive set. 
    110         osg::PrimitiveSet* primitives = new osg::DrawArrays(osg::PrimitiveSet::TRIANGLES,0,48); 
     107    osg::ref_ptr<osg::UByteArray> coordIndices = new osg::UByteArray(48,indices); 
     108 
     109    // share the primitive set. 
     110    osg::PrimitiveSet* primitives = new osg::DrawArrays(osg::PrimitiveSet::TRIANGLES,0,48); 
    111111 
    112112    for (int q = 0; q < HOUSES_SIZE; q++) 
     
    122122        float scale = 10.0f; 
    123123                 
    124                 osg::Vec3 offset(xPos,yPos,0.0f); 
     124        osg::Vec3 offset(xPos,yPos,0.0f); 
    125125 
    126126        // coords 
    127         osg::Vec3Array* coords = new osg::Vec3Array(10); 
     127        osg::ref_ptr<osg::Vec3Array> coords = new osg::Vec3Array(10); 
    128128        (*coords)[0] = osg::Vec3( 0.5f, -0.7f, 0.0f); 
    129129        (*coords)[1] = osg::Vec3( 0.5f,  0.7f, 0.0f); 
     
    144144 
    145145        // create geometry 
    146         osg::Geometry * geometry = new osg::Geometry(); 
     146        osg::ref_ptr<osg::Geometry> geometry = new osg::Geometry(); 
    147147                 
    148148        geometry->addPrimitiveSet(primitives); 
    149149                 
    150         geometry->setVertexArray(coords); 
    151                 geometry->setVertexIndices(coordIndices); 
    152                  
    153         geometry->setColorArray(colors); 
     150        geometry->setVertexArray(coords.get()); 
     151        geometry->setVertexIndices(coordIndices.get()); 
     152                 
     153        geometry->setColorArray(colors.get()); 
    154154        geometry->setColorBinding(osg::Geometry::BIND_OVERALL); 
    155155                 
    156         geometry->setNormalArray(normals); 
     156        geometry->setNormalArray(normals.get()); 
    157157        geometry->setNormalBinding(osg::Geometry::BIND_PER_PRIMITIVE); 
    158158 
    159         osg::Geode * geode = new osg::Geode(); 
    160         geode->addDrawable(geometry); 
     159        osg::ref_ptr<osg::Geode> geode = new osg::Geode(); 
     160        geode->addDrawable(geometry.get()); 
    161161         
    162         nodes.push_back(geode); 
     162        nodes.push_back(geode.get()); 
    163163    } 
    164164}