Hi Jannik, I've reviewed your changes and agree with your analysis and while the suggest change is a bit hacky, the original code was as well, but to me your change looks safe. I've merged it with svn/trunk and the OSG-3.4 branch.
Cheers, Robert. On 30 August 2015 at 19:42, Jannik Heller <[email protected]> wrote: > Hi Robert, > > 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. > > Note I haven't been able to test if this fixes the problem, because the > crashes were random and only encountered by users, not me. It looks like a > cleaner and more efficient solution in any case, though. > > Thank you! > > Cheers, > Jannik > > ------------------ > Read this topic online here: > http://forum.openscenegraph.org/viewtopic.php?p=64944#64944 > > > > > _______________________________________________ > osg-submissions mailing list > [email protected] > > http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org > >
_______________________________________________ osg-submissions mailing list [email protected] http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org
