Show
Ignore:
Timestamp:
09/04/15 17:35:24 (3 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/src/osgPlugins/normals/Normals.cpp

    r13041 r13502  
    6060        if( geom ) 
    6161        { 
     62            if (geom->containsDeprecatedData()) geom->fixDeprecatedData(); 
     63             
    6264            Vec3Array *coords   = dynamic_cast<Vec3Array*>(geom->getVertexArray()); 
    6365            if( coords == 0L ) 
     
    8688                _local_coords->push_back( (v + n)); 
    8789            } 
    88             else // BIND_PER_PRIMITIVE_SET, BIND_PER_PRIMITIVE, BIND_PER_VERTEX 
     90            else // BIND_PER_PRIMITIVE_SET, BIND_PER_VERTEX 
    8991            { 
    9092                Geometry::PrimitiveSetList& primitiveSets = geom->getPrimitiveSetList(); 
     
    122124                                    _processPrimitive( 3, coord_index, normals_index, binding ); 
    123125                                    coord_index += 3; 
    124                                     if( binding == Geometry::BIND_PER_PRIMITIVE ) 
    125                                         normals_index++; 
    126                                     else 
    127                                         normals_index+=3; 
     126                                    normals_index+=3; 
    128127                                } 
    129128                                break; 
     
    151150                                    _processPrimitive( 4, coord_index, normals_index, binding ); 
    152151                                    coord_index += 4; 
    153                                     if( binding == Geometry::BIND_PER_PRIMITIVE ) 
    154                                         normals_index++; 
    155                                     else 
    156                                         normals_index+=4; 
     152                                    normals_index +=4; 
    157153                                } 
    158154                                break; 
     
    170166                                        _processPrimitive(num_prim, coord_index, normals_index, binding); 
    171167                                        coord_index += num_prim; 
    172                                         if (binding == Geometry::BIND_PER_PRIMITIVE) { 
    173                                             ++normals_index; 
    174                                         } else { 
    175                                             normals_index += num_prim; 
    176                                         } 
     168                                        normals_index += num_prim; 
    177169                                    } 
    178170                                } 
     
    199191    Vec3 v(0,0,0); 
    200192    Vec3 n(0,0,0); 
    201     if( _mode == SurfaceNormals || binding == Geometry::BIND_PER_PRIMITIVE ) 
     193    if( _mode == SurfaceNormals ) 
    202194    { 
    203         if( binding == Geometry::BIND_PER_PRIMITIVE ) 
    204         { 
    205             n = *(normals++); 
    206         } 
    207         else if( binding == Geometry::BIND_PER_VERTEX ) 
     195        if( binding == Geometry::BIND_PER_VERTEX ) 
    208196        { 
    209197            for( unsigned int i = 0; i < nv; i++ )