Show
Ignore:
Timestamp:
09/04/15 17:35:24 (21 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/fbx/fbxMaterialToOsgStateSet.cpp

    r13041 r13466  
    66 
    77 
    8 static osg::Texture::WrapMode convertWrap(KFbxFileTexture::EWrapMode wrap) 
    9 { 
    10     return wrap == KFbxFileTexture::eREPEAT ? 
     8static osg::Texture::WrapMode convertWrap(FbxFileTexture::EWrapMode wrap) 
     9{ 
     10    return wrap == FbxFileTexture::eRepeat ? 
    1111        osg::Texture2D::REPEAT : osg::Texture2D::CLAMP_TO_EDGE; 
    1212} 
    1313 
    1414StateSetContent 
    15 FbxMaterialToOsgStateSet::convert(const KFbxSurfaceMaterial* pFbxMat) 
     15FbxMaterialToOsgStateSet::convert(const FbxSurfaceMaterial* pFbxMat) 
    1616{ 
    1717    FbxMaterialMap::const_iterator it = _fbxMaterialMap.find(pFbxMat); 
     
    2626    result.material = pOsgMat; 
    2727 
    28     fbxString shadingModel = pFbxMat->ShadingModel.Get(); 
    29  
    30     const KFbxSurfaceLambert* pFbxLambert = KFbxCast<KFbxSurfaceLambert>(pFbxMat); 
     28    FbxString shadingModel = pFbxMat->ShadingModel.Get(); 
     29 
     30    const FbxSurfaceLambert* pFbxLambert = FbxCast<FbxSurfaceLambert>(pFbxMat); 
    3131 
    3232    // diffuse map... 
    33     const KFbxProperty lProperty = pFbxMat->FindProperty(KFbxSurfaceMaterial::sDiffuse); 
     33    const FbxProperty lProperty = pFbxMat->FindProperty(FbxSurfaceMaterial::sDiffuse); 
    3434    if (lProperty.IsValid()) 
    3535    { 
    36         int lNbTex = lProperty.GetSrcObjectCount(KFbxFileTexture::ClassId); 
    37         for (int lTextureIndex = 0; lTextureIndex < lNbTex; lTextureIndex++) 
    38         { 
    39             KFbxFileTexture* lTexture = KFbxCast<KFbxFileTexture>(lProperty.GetSrcObject(KFbxFileTexture::ClassId, lTextureIndex)); 
     36        int lNbTex = lProperty.GetSrcObjectCount<FbxFileTexture>(); 
     37        for (int lTextureIndex = 0; lTextureIndex < lNbTex; lTextureIndex++) 
     38        { 
     39            FbxFileTexture* lTexture = FbxCast<FbxFileTexture>(lProperty.GetSrcObject<FbxFileTexture>(lTextureIndex)); 
    4040            if (lTexture) 
    4141            { 
     
    5252 
    5353    // opacity map... 
    54     const KFbxProperty lOpacityProperty = pFbxMat->FindProperty(KFbxSurfaceMaterial::sTransparentColor); 
     54    const FbxProperty lOpacityProperty = pFbxMat->FindProperty(FbxSurfaceMaterial::sTransparentColor); 
    5555    if (lOpacityProperty.IsValid()) 
    5656    { 
    57         int lNbTex = lOpacityProperty.GetSrcObjectCount(KFbxFileTexture::ClassId); 
    58         for (int lTextureIndex = 0; lTextureIndex < lNbTex; lTextureIndex++) 
    59         { 
    60             KFbxFileTexture* lTexture = KFbxCast<KFbxFileTexture>(lOpacityProperty.GetSrcObject(KFbxFileTexture::ClassId, lTextureIndex)); 
     57        int lNbTex = lOpacityProperty.GetSrcObjectCount<FbxFileTexture>(); 
     58        for (int lTextureIndex = 0; lTextureIndex < lNbTex; lTextureIndex++) 
     59        { 
     60            FbxFileTexture* lTexture = FbxCast<FbxFileTexture>(lOpacityProperty.GetSrcObject<FbxFileTexture>(lTextureIndex)); 
    6161            if (lTexture) 
    6262            { 
     
    7575 
    7676    // reflection map... 
    77     const KFbxProperty lReflectionProperty = pFbxMat->FindProperty(KFbxSurfaceMaterial::sReflection); 
     77    const FbxProperty lReflectionProperty = pFbxMat->FindProperty(FbxSurfaceMaterial::sReflection); 
    7878    if (lReflectionProperty.IsValid()) 
    7979    { 
    80         int lNbTex = lReflectionProperty.GetSrcObjectCount(KFbxFileTexture::ClassId); 
    81         for (int lTextureIndex = 0; lTextureIndex < lNbTex; lTextureIndex++) 
    82         { 
    83             KFbxFileTexture* lTexture = KFbxCast<KFbxFileTexture>(lReflectionProperty.GetSrcObject(KFbxFileTexture::ClassId, lTextureIndex)); 
     80        int lNbTex = lReflectionProperty.GetSrcObjectCount<FbxFileTexture>(); 
     81        for (int lTextureIndex = 0; lTextureIndex < lNbTex; lTextureIndex++) 
     82        { 
     83            FbxFileTexture* lTexture = FbxCast<FbxFileTexture>(lReflectionProperty.GetSrcObject<FbxFileTexture>(lTextureIndex)); 
    8484            if (lTexture) 
    8585            { 
    8686                // support only spherical reflection maps... 
    87                 if (KFbxFileTexture::eUMT_ENVIRONMENT == lTexture->CurrentMappingType.Get()) 
     87                if (FbxFileTexture::eUMT_ENVIRONMENT == lTexture->CurrentMappingType.Get()) 
    8888                { 
    8989                    result.reflectionTexture = fbxTextureToOsgTexture(lTexture); 
     
    9898 
    9999    // emissive map... 
    100     const KFbxProperty lEmissiveProperty = pFbxMat->FindProperty(KFbxSurfaceMaterial::sEmissive); 
     100    const FbxProperty lEmissiveProperty = pFbxMat->FindProperty(FbxSurfaceMaterial::sEmissive); 
    101101    if (lEmissiveProperty.IsValid()) 
    102102    { 
    103         int lNbTex = lEmissiveProperty.GetSrcObjectCount(KFbxFileTexture::ClassId); 
    104         for (int lTextureIndex = 0; lTextureIndex < lNbTex; lTextureIndex++) 
    105         { 
    106             KFbxFileTexture* lTexture = KFbxCast<KFbxFileTexture>(lEmissiveProperty.GetSrcObject(KFbxFileTexture::ClassId, lTextureIndex)); 
     103        int lNbTex = lEmissiveProperty.GetSrcObjectCount<FbxFileTexture>(); 
     104        for (int lTextureIndex = 0; lTextureIndex < lNbTex; lTextureIndex++) 
     105        { 
     106            FbxFileTexture* lTexture = FbxCast<FbxFileTexture>(lEmissiveProperty.GetSrcObject<FbxFileTexture>(lTextureIndex)); 
    107107            if (lTexture) 
    108108            { 
     
    120120    if (pFbxLambert) 
    121121    { 
    122         fbxDouble3 color = pFbxLambert->Diffuse.Get(); 
     122        FbxDouble3 color = pFbxLambert->Diffuse.Get(); 
    123123        double factor = pFbxLambert->DiffuseFactor.Get(); 
    124124        pOsgMat->setDiffuse(osg::Material::FRONT_AND_BACK, osg::Vec4( 
     
    147147        result.diffuseFactor = pFbxLambert->DiffuseFactor.Get(); 
    148148 
    149         if (const KFbxSurfacePhong* pFbxPhong = KFbxCast<KFbxSurfacePhong>(pFbxLambert)) 
     149        if (const FbxSurfacePhong* pFbxPhong = FbxCast<FbxSurfacePhong>(pFbxLambert)) 
    150150        { 
    151151            color = pFbxPhong->Specular.Get(); 
     
    183183 
    184184osg::ref_ptr<osg::Texture2D> 
    185 FbxMaterialToOsgStateSet::fbxTextureToOsgTexture(const KFbxFileTexture* fbx) 
     185FbxMaterialToOsgStateSet::fbxTextureToOsgTexture(const FbxFileTexture* fbx) 
    186186{ 
    187187    ImageMap::iterator it = _imageMap.find(fbx->GetFileName());