From cb3396b0e5522ce521ec5fe457db6dab2e4093d3 Mon Sep 17 00:00:00 2001 From: Robert Osfield Date: Fri, 4 Sep 2015 15:35:24 +0000 Subject: [PATCH] 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. " git-svn-id: http://svn.openscenegraph.org/osg/OpenSceneGraph/trunk@15129 16af8721-9629-0410-8352-f15c8da7e697 --- src/osg/Geometry.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/osg/Geometry.cpp b/src/osg/Geometry.cpp index c1e20d094..5597350e8 100644 --- a/src/osg/Geometry.cpp +++ b/src/osg/Geometry.cpp @@ -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); } }