Changeset 14045

Show
Ignore:
Timestamp:
09/04/15 17:35:24 (8 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/ply/plyfile.cpp

    r13041 r14045  
    301301) 
    302302{ 
    303   PlyFile *plyfile; 
    304   char *name; 
    305   FILE *fp; 
    306  
    307  
    308   /* tack on the extension .ply, if necessary */ 
    309   name = (char *) myalloc (sizeof (char) * 
    310                            (static_cast<int>(strlen (filename)) + 5)); 
    311   strcpy (name, filename); 
    312   if (strlen (name) < 4 || 
    313       strcmp (name + strlen (name) - 4, ".ply") != 0) 
    314       strcat (name, ".ply"); 
    315  
    316303  /* open the file for writing */ 
    317304 
    318   fp = osgDB::fopen (name, "wb"); 
    319   free (name); //wjs remove memory leak// 
     305  FILE *fp = osgDB::fopen (filename, "wb"); 
    320306  if (fp == NULL) { 
    321307    return (NULL); 
     
    324310  /* create the actual PlyFile structure */ 
    325311 
    326   plyfile = ply_write (fp, nelems, elem_names, file_type); 
     312  PlyFile *plyfile = ply_write (fp, nelems, elem_names, file_type); 
    327313 
    328314  // If the plyfile could not load return NULL 
     
    947933  FILE *fp; 
    948934  PlyFile *plyfile; 
    949   char *name; 
    950  
    951   /* tack on the extension .ply, if necessary */ 
    952  
    953   name = (char *) myalloc (sizeof (char) * 
    954                            (static_cast<int>(strlen (filename) + 5))); 
    955   strcpy (name, filename); 
    956   if (strlen (name) < 4 || 
    957       strcmp (name + strlen (name) - 4, ".ply") != 0) 
    958       strcat (name, ".ply"); 
    959935 
    960936  /* open the file for reading */ 
    961  
    962   fp = osgDB::fopen (name, "rb"); 
    963   free(name); 
     937  fp = osgDB::fopen (filename, "rb"); 
    964938  if (fp == NULL) 
    965939    return (NULL); 
     
    971945  if(!plyfile) 
    972946  { 
    973     std::cout<<"Ply File Error : Could not read file"<<std::endl; 
     947    std::cout<<"Ply File Error : Could not read file " << filename <<std::endl; 
    974948    return NULL; 
    975949  }