From 115e563a7ab82ffb7679d0427f6952fff95e8b23 Mon Sep 17 00:00:00 2001 From: scrawl Date: Thu, 19 Jan 2017 22:03:52 +0100 Subject: [PATCH] Fix race conditions caused by Array <-> GLBufferObject interactions (Bug #3580) The first part of the fix is to assign VBO/EBO's upon loading the array in the Nif reader. This avoids triggering the 'addVertexBufferObjectIfRequired' code path in osg::Geometry which has the race condition when two threads add the same Array at the same time. Essentially, we want the Arrays to be 'const' when they come out of the Nif reader. The second part of the fix is to make sure not to create empty arrays in the Nif reader (importantly, not assigning a VBO to the empty array). This empty array would be deleted when the NIFFile is cleaned up, and the detachment of the VBO assigned to it (which is still in use by other arrays) would cause threading issues. This rare crash bug was first introduced with commit a7c5beb7c5d495446e5d7facaa780fbf851d1848. When using OSG dev version 3.5 the crashes were a little more prevalent, because 'addVertexBufferObjectIfRequired' in osg::Geometry is now used even when VBO's are disabled (as part of the VAO support changes). --- components/nif/data.cpp | 37 ++++++++++++++++++------ components/nifbullet/bulletnifloader.cpp | 7 ++--- components/nifosg/nifloader.cpp | 16 ++++++---- 3 files changed, 41 insertions(+), 19 deletions(-) diff --git a/components/nif/data.cpp b/components/nif/data.cpp index 1de7d0966..082b5aa63 100644 --- a/components/nif/data.cpp +++ b/components/nif/data.cpp @@ -40,32 +40,45 @@ void ShapeData::read(NIFStream *nif) { int verts = nif->getUShort(); - vertices = new osg::Vec3Array; - if(nif->getInt()) + // Assign a VBO right off the bat to avoid race conditions later, in case two threads load this data into a Geometry at the same time + osg::ref_ptr vbo = new osg::VertexBufferObject; + + if(nif->getInt() && verts) + { + vertices = new osg::Vec3Array; nif->getVector3s(vertices, verts); + vertices->setVertexBufferObject(vbo); + } - normals = new osg::Vec3Array(osg::Array::BIND_PER_VERTEX); - if(nif->getInt()) + if(nif->getInt() && verts) + { + normals = new osg::Vec3Array(osg::Array::BIND_PER_VERTEX); nif->getVector3s(normals, verts); + normals->setVertexBufferObject(vbo); + } center = nif->getVector3(); radius = nif->getFloat(); - colors = new osg::Vec4Array(osg::Array::BIND_PER_VERTEX); - if(nif->getInt()) + if(nif->getInt() && verts) + { + colors = new osg::Vec4Array(osg::Array::BIND_PER_VERTEX); nif->getVector4s(colors, verts); + colors->setVertexBufferObject(vbo); + } // Only the first 6 bits are used as a count. I think the rest are // flags of some sort. int uvs = nif->getUShort(); uvs &= 0x3f; - if(nif->getInt()) + if(nif->getInt() && verts) { uvlist.resize(uvs); for(int i = 0;i < uvs;i++) { osg::Vec2Array* list = uvlist[i] = new osg::Vec2Array(osg::Array::BIND_PER_VERTEX); + list->setVertexBufferObject(vbo); nif->getVector2s(list, verts); // flip the texture coordinates to convert them to the OpenGL convention of bottom-left image origin @@ -86,8 +99,14 @@ void NiTriShapeData::read(NIFStream *nif) // We have three times as many vertices as triangles, so this // is always equal to tris*3. int cnt = nif->getInt(); - triangles = new osg::DrawElementsUShort(osg::PrimitiveSet::TRIANGLES); - nif->getUShorts(triangles, cnt); + if (cnt) + { + triangles = new osg::DrawElementsUShort(osg::PrimitiveSet::TRIANGLES); + nif->getUShorts(triangles, cnt); + + // Assign an EBO right off the bat to avoid race conditions later, in case two threads load this data into a Geometry at the same time + triangles->setElementBufferObject(new osg::ElementBufferObject); + } // Read the match list, which lists the vertices that are equal to // vertices. We don't actually need need this for anything, so diff --git a/components/nifbullet/bulletnifloader.cpp b/components/nifbullet/bulletnifloader.cpp index 9ac544c42..6a369931a 100644 --- a/components/nifbullet/bulletnifloader.cpp +++ b/components/nifbullet/bulletnifloader.cpp @@ -268,7 +268,9 @@ void BulletNifLoader::handleNiTriShape(const Nif::NiTriShape *shape, int flags, if (shape->data.empty()) return; - if (shape->data->triangles->empty()) + + const Nif::NiTriShapeData *data = shape->data.getPtr(); + if (!data->triangles || !data->vertices) return; if (isAnimated) @@ -278,8 +280,6 @@ void BulletNifLoader::handleNiTriShape(const Nif::NiTriShape *shape, int flags, btTriangleMesh* childMesh = new btTriangleMesh(); - const Nif::NiTriShapeData *data = shape->data.getPtr(); - childMesh->preallocateVertices(data->vertices->size()); childMesh->preallocateIndices(data->triangles->size()); @@ -320,7 +320,6 @@ void BulletNifLoader::handleNiTriShape(const Nif::NiTriShape *shape, int flags, mStaticMesh = new btTriangleMesh(false); // Static shape, just transform all vertices into position - const Nif::NiTriShapeData *data = shape->data.getPtr(); const osg::Vec3Array& vertices = *data->vertices; const osg::DrawElementsUShort& triangles = *data->triangles; diff --git a/components/nifosg/nifloader.cpp b/components/nifosg/nifloader.cpp index a0001d6fd..66575699f 100644 --- a/components/nifosg/nifloader.cpp +++ b/components/nifosg/nifloader.cpp @@ -894,6 +894,10 @@ namespace NifOsg osg::BoundingBox box; + osg::Vec3Array* verts = particledata->vertices; + if (!verts) + return; + int i=0; for (std::vector::const_iterator it = partctrl->particles.begin(); iactiveCount && it != partctrl->particles.end(); ++it, ++i) @@ -908,11 +912,11 @@ namespace NifOsg // Note this position and velocity is not correct for a particle system with absolute reference frame, // which can not be done in this loader since we are not attached to the scene yet. Will be fixed up post-load in the SceneManager. created->setVelocity(particle.velocity); - const osg::Vec3f& position = particledata->vertices->at(particle.vertex); + const osg::Vec3f& position = verts->at(particle.vertex); created->setPosition(position); osg::Vec4f partcolor (1.f,1.f,1.f,1.f); - if (particle.vertex < int(particledata->colors->size())) + if (particledata->colors.valid() && particle.vertex < int(particledata->colors->size())) partcolor = particledata->colors->at(particle.vertex); float size = particledata->sizes.at(particle.vertex) * partctrl->size; @@ -1074,7 +1078,7 @@ namespace NifOsg geometry->setVertexArray(data->vertices); - if (!data->normals->empty()) + if (data->normals) geometry->setNormalArray(data->normals); int textureStage = 0; @@ -1092,10 +1096,10 @@ namespace NifOsg geometry->setTexCoordArray(textureStage, data->uvlist[uvSet]); } - if (!data->colors->empty()) + if (data->colors) geometry->setColorArray(data->colors); - if (!data->triangles->empty()) + if (data->triangles) geometry->addPrimitiveSet(data->triangles); // osg::Material properties are handled here for two reasons: @@ -1104,7 +1108,7 @@ namespace NifOsg // above the actual renderable would be tedious. std::vector drawableProps; collectDrawableProperties(triShape, drawableProps); - applyDrawableProperties(parentNode, drawableProps, composite, !data->colors->empty(), animflags, false); + applyDrawableProperties(parentNode, drawableProps, composite, data->colors.valid(), animflags, false); } void handleTriShape(const Nif::NiTriShape* triShape, osg::Group* parentNode, SceneUtil::CompositeStateSetUpdater* composite, const std::vector& boundTextures, int animflags)