#3713 Crash at updateGLTFMaterials

Author: akleshchev

indra/newview/llfetchedgltfmaterial.cpp

@@ -199,6 +199,7 @@ bool LLFetchedGLTFMaterial::replaceLocalTexture(const LLUUID& tracking_id, const
     {
         mTrackingIdToLocalTexture.erase(tracking_id);
     }
+    updateLocalTexDataDigest();
 
     return res;
 }

indra/newview/lllocalbitmaps.cpp

@@ -293,8 +293,9 @@ void LLLocalBitmap::addGLTFMaterial(LLGLTFMaterial* mat)
         return;
     }
 
-    mat_list_t::iterator end = mGLTFMaterialWithLocalTextures.end();
-    for (mat_list_t::iterator it = mGLTFMaterialWithLocalTextures.begin(); it != end;)
+    mat->addLocalTextureTracking(getTrackingID(), getWorldID());
+
+    for (mat_list_t::iterator it = mGLTFMaterialWithLocalTextures.begin(); it != mGLTFMaterialWithLocalTextures.end();)
     {
         if (it->get() == mat)
         {
@@ -304,15 +305,12 @@ void LLLocalBitmap::addGLTFMaterial(LLGLTFMaterial* mat)
         if ((*it)->getNumRefs() == 1)
         {
             it = mGLTFMaterialWithLocalTextures.erase(it);
-            end = mGLTFMaterialWithLocalTextures.end();
         }
         else
         {
             it++;
         }
     }
-
-    mat->addLocalTextureTracking(getTrackingID(), getWorldID());
     mGLTFMaterialWithLocalTextures.push_back(mat);
 }
 
@@ -628,16 +626,16 @@ void LLLocalBitmap::updateUserLayers(LLUUID old_id, LLUUID new_id, LLWearableTyp
 void LLLocalBitmap::updateGLTFMaterials(LLUUID old_id, LLUUID new_id)
 {
     // Might be a better idea to hold this in LLGLTFMaterialList
-    mat_list_t::iterator end = mGLTFMaterialWithLocalTextures.end();
-    for (mat_list_t::iterator it = mGLTFMaterialWithLocalTextures.begin(); it != end;)
+    for (mat_list_t::iterator it = mGLTFMaterialWithLocalTextures.begin(); it != mGLTFMaterialWithLocalTextures.end();)
     {
         if ((*it)->getNumRefs() == 1)
         {
             // render and override materials are often recreated,
             // clean up any remains
             it = mGLTFMaterialWithLocalTextures.erase(it);
-            end = mGLTFMaterialWithLocalTextures.end();
         }
+        // Render material consists of base and override materials, make sure replaceLocalTexture
+        // gets called for base and override before applyOverride
         else if ((*it)->replaceLocalTexture(mTrackingID, old_id, new_id))
         {
             it++;
@@ -647,43 +645,47 @@ void LLLocalBitmap::updateGLTFMaterials(LLUUID old_id, LLUUID new_id)
             // Matching id not found, no longer in use
             // material would clean itself, remove from the list
             it = mGLTFMaterialWithLocalTextures.erase(it);
-            end = mGLTFMaterialWithLocalTextures.end();
         }
     }
 
-    // Render material consists of base and override materials, make sure replaceLocalTexture
-    // gets called for base and override before applyOverride
-    end = mGLTFMaterialWithLocalTextures.end();
-    for (mat_list_t::iterator it = mGLTFMaterialWithLocalTextures.begin(); it != end;)
+    // Updating render materials calls updateTextureTracking which can modify
+    // mGLTFMaterialWithLocalTextures, so precollect all entries that need to be updated
+    std::set<LLTextureEntry*> update_entries;
+    for (LLGLTFMaterial* mat : mGLTFMaterialWithLocalTextures)
     {
-        LLFetchedGLTFMaterial* fetched_mat = dynamic_cast<LLFetchedGLTFMaterial*>((*it).get());
+        // mGLTFMaterialWithLocalTextures includes overrides that are not 'fetched'
+        // and don't have texture entries (they don't need to since render material does).
+        LLFetchedGLTFMaterial* fetched_mat = dynamic_cast<LLFetchedGLTFMaterial*>(mat);
         if (fetched_mat)
         {
             for (LLTextureEntry* entry : fetched_mat->mTextureEntires)
             {
-                // Normally a change in applied material id is supposed to
-                // drop overrides thus reset material, but local materials
-                // currently reuse their existing asset id, and purpose is
-                // to preview how material will work in-world, overrides
-                // included, so do an override to render update instead.
-                LLGLTFMaterial* override_mat = entry->getGLTFMaterialOverride();
-                if (override_mat)
-                {
-                    // do not create a new material, reuse existing pointer
-                    LLFetchedGLTFMaterial* render_mat = dynamic_cast<LLFetchedGLTFMaterial*>(entry->getGLTFRenderMaterial());
-                    if (render_mat)
-                    {
-                            *render_mat = *fetched_mat;
-                        render_mat->applyOverride(*override_mat);
-                    }
-                    else
-                    {
-                        LL_WARNS_ONCE() << "Failed to apply local material override, render material not found" << LL_ENDL;
-                    }
-                }
+                update_entries.insert(entry);
+            }
+        }
+    }
+
+
+    for (LLTextureEntry* entry : update_entries)
+    {
+        // Normally a change in applied material id is supposed to
+        // drop overrides thus reset material, but local materials
+        // currently reuse their existing asset id, and purpose is
+        // to preview how material will work in-world, overrides
+        // included, so do an override to render update instead.
+        LLGLTFMaterial* override_mat = entry->getGLTFMaterialOverride();
+        LLGLTFMaterial* mat = entry->getGLTFMaterial();
+        if (override_mat && mat)
+        {
+            // do not create a new material, reuse existing pointer
+            // so that mTextureEntires remains untouched
+            LLGLTFMaterial* render_mat = entry->getGLTFRenderMaterial();
+            if (render_mat)
+            {
+                *render_mat = *mat;
+                render_mat->applyOverride(*override_mat); // can update mGLTFMaterialWithLocalTextures
             }
         }
-        ++it;
     }
 }
 

indra/newview/lllocalbitmaps.h

@@ -106,7 +106,7 @@ class LLLocalBitmap
 
         // Store a list of accosiated materials
         // Might be a better idea to hold this in LLGLTFMaterialList
-        typedef std::vector<LLPointer<LLGLTFMaterial> > mat_list_t;
+        typedef std::list<LLPointer<LLGLTFMaterial> > mat_list_t;
         mat_list_t mGLTFMaterialWithLocalTextures;
 
 };