Merge pull request #298 from AlchemyViewer/rye/local-assets-review-fixes

Local Assets: post-merge defect-review fixes

Author: RyeMutt

indra/llcharacter/llcharacter.cpp

@@ -109,6 +109,14 @@ void LLCharacter::removeMotion( const LLUUID& id )
     mMotionController.removeMotion(id);
 }
 
+//-----------------------------------------------------------------------------
+// purgeMotionInstances()
+//-----------------------------------------------------------------------------
+void LLCharacter::purgeMotionInstances( const LLUUID& id )
+{
+    mMotionController.purgeMotionInstances(id);
+}
+
 //-----------------------------------------------------------------------------
 // findMotion()
 //-----------------------------------------------------------------------------

indra/llcharacter/llcharacter.h

@@ -133,6 +133,11 @@ class LLCharacter
 
     void removeMotion( const LLUUID& id );
 
+    // removes ALL instances of a motion -- the canonical one AND any deprecated
+    // duplicates still easing out -- for when the motion's backing keyframe
+    // data is about to be destroyed (see LLMotionController::purgeMotionInstances)
+    void purgeMotionInstances( const LLUUID& id );
+
     // returns an instance of a registered motion, creating one if necessary
     LLMotion* createMotion( const LLUUID &id );
 

indra/llcharacter/llmotioncontroller.cpp

@@ -335,6 +335,27 @@ void LLMotionController::removeMotionInstance(LLMotion* motionp)
     }
 }
 
+//-----------------------------------------------------------------------------
+// purgeMotionInstances()
+//-----------------------------------------------------------------------------
+void LLMotionController::purgeMotionInstances(const LLUUID& id)
+{
+    removeMotion(id);
+    for (motion_set_t::iterator iter = mDeprecatedMotions.begin();
+         iter != mDeprecatedMotions.end(); )
+    {
+        motion_set_t::iterator cur_iter = iter++;
+        LLMotion* motionp = *cur_iter;
+        if (motionp->getID() == id)
+        {
+            // The same complete excision deactivateMotionInstance() applies to
+            // deprecated motions; removeMotionInstance() deactivates if needed.
+            removeMotionInstance(motionp); // does not touch mDeprecatedMotions
+            mDeprecatedMotions.erase(cur_iter);
+        }
+    }
+}
+
 //-----------------------------------------------------------------------------
 // createMotion()
 //-----------------------------------------------------------------------------

indra/llcharacter/llmotioncontroller.h

@@ -122,6 +122,15 @@ class LLMotionController
     // returns true if successful
     bool stopMotionLocally( const LLUUID &id, bool stop_immediate );
 
+    // immediately deactivates and deletes EVERY instance of a motion id --
+    // the canonical instance and any deprecated duplicates still easing out.
+    // For use when the motion's backing data is about to be destroyed (e.g. a
+    // locally previewed animation whose globally cached keyframe data is purged
+    // on live reload/removal): removeMotion() only reaches the canonical
+    // instance, and a deprecated instance left easing out keeps dereferencing
+    // the freed data every frame.
+    void purgeMotionInstances( const LLUUID& id );
+
     // Move motions from loading to loaded
     void updateLoadingMotions();
 

indra/llprimitive/llmodelloader.cpp

@@ -32,6 +32,7 @@
 #include "llsdserialize.h"
 #include "lljoint.h"
 #include "llcallbacklist.h"
+#include "workqueue.h"
 
 #include "llmatrix4a.h"
 #include <boost/bind.hpp>
@@ -192,9 +193,24 @@ void LLModelLoader::run()
         setLoadState(ERROR_PARSING);
     }
 
-    // todo: we are inside of a thread, push this into main thread worker,
-    // not into doOnIdleOneTime that laks tread safety
-    doOnIdleOneTime(boost::bind(&LLModelLoader::loadModelCallback,this));
+    // Hand completion back to the main thread. run() executes on the loader's
+    // worker thread, and the LLCallbackList behind doOnIdleOneTime() is not
+    // thread-safe -- post through the main loop's WorkQueue instead.
+    LL::WorkQueue::ptr_t main_queue = LL::WorkQueue::getInstance("mainloop");
+    if (main_queue)
+    {
+        main_queue->post(boost::bind(&LLModelLoader::loadModelCallback, this));
+    }
+    else
+    {
+        // No main-loop queue registered. The viewer always has one (a global,
+        // gMainloopWork), so this branch can only run in a binary with no main
+        // loop concurrently using LLCallbackList -- where the idle list is the
+        // only delivery left. loadModelCallback() cannot be invoked inline
+        // here: it blocks until this thread is stopped and then deletes the
+        // loader, which would deadlock run().
+        doOnIdleOneTime(boost::bind(&LLModelLoader::loadModelCallback, this));
+    }
 }
 
 // static

indra/newview/llfloaterlocalassets.cpp

@@ -402,16 +402,16 @@ void LLPanelLocalAssetBase::onRemoveBtn()
 
     for (const auto& entry : selected)
     {
-        if (!entry.first.empty())
-        {
-            LLLocalAssetPaths::getInstance()->removePath(assetType(), entry.first); // forget the path
-        }
-        // Unload every decoded unit backing this row. For a multi-material glTF file
-        // that's all of the file's material units, not just the selected row.
+        // Unload every decoded unit backing this row FIRST and forget the path
+        // only once they are all gone. delUnit() fires the manager signals
+        // synchronously, and the add-only LLLocalAssetPaths::onUnitsChanged()
+        // re-records any path a still-loaded unit reports -- removePath() up
+        // front would be undone by the very first delUnit (mesh teardown, or a
+        // multi-material glTF file's surviving sibling units).
         std::vector<LLUUID> ids;
         if (!entry.first.empty())
         {
-            unitsForPath(entry.first, ids);
+            unitsForPath(entry.first, ids); // all of the file's units, not just the row
         }
         if (ids.empty() && entry.second.notNull())
         {
@@ -421,6 +421,10 @@ void LLPanelLocalAssetBase::onRemoveBtn()
         {
             delUnit(id); // unload the decoded unit (fires the manager signal)
         }
+        if (!entry.first.empty())
+        {
+            LLLocalAssetPaths::getInstance()->removePath(assetType(), entry.first); // forget the path
+        }
     }
     refresh(); // removePath() alone (undecoded rows) doesn't fire a manager signal
 }
@@ -723,11 +727,15 @@ void LLPanelLocalMesh::onRez()
         doSpawn(id); // always rez a new copy
         return;
     }
-    // Undecoded: load it and rez once it finishes (addAndSpawn handles the async load).
+    // Undecoded: load it and rez once it finishes (addAndSpawn handles the async
+    // load). Decode with the joint-position flag the artist saved for this file,
+    // like loadPath -- defaulting it would also make onUnitsChanged erase the
+    // saved flag.
     const std::string path = getSelectedPath();
     if (!path.empty())
     {
-        LLLocalMeshMgr::getInstance()->addAndSpawn(std::vector<std::string>(1, path));
+        LLLocalMeshMgr::getInstance()->addAndSpawn(std::vector<std::string>(1, path),
+                                                   LLLocalAssetPaths::getInstance()->getMeshJoints(path));
     }
 }
 
@@ -740,11 +748,12 @@ void LLPanelLocalMesh::onAttach()
         return;
     }
     // Undecoded row: load it and attach once it finishes loading (mirrors how Rez
-    // handles an undecoded row via addAndSpawn).
+    // handles an undecoded row via addAndSpawn), honoring the saved joint flag.
     const std::string path = getSelectedPath();
     if (!path.empty())
     {
-        LLLocalMeshMgr::getInstance()->addAndAttach(path, getComboAttachPoint());
+        LLLocalMeshMgr::getInstance()->addAndAttach(path, getComboAttachPoint(),
+                                                    LLLocalAssetPaths::getInstance()->getMeshJoints(path));
     }
 }
 
@@ -837,9 +846,13 @@ void LLPanelLocalMesh::doRemove(const LLUUID& tracking_id)
 {
     if (tracking_id.notNull())
     {
-        LLLocalAssetPaths::getInstance()->removePath(LLLocalAssetPaths::TYPE_MESH,
-                                                     pathForUnit(tracking_id));
+        // Resolve the path before the unit dies, but forget it only AFTER
+        // delUnit: the units-changed listeners fire during teardown and the
+        // add-only LLLocalAssetPaths::onUnitsChanged would re-record a path
+        // removed up front (see onRemoveBtn).
+        const std::string path = pathForUnit(tracking_id);
         delUnit(tracking_id); // units-changed signal -> refresh()
+        LLLocalAssetPaths::getInstance()->removePath(LLLocalAssetPaths::TYPE_MESH, path);
     }
 }
 
@@ -1217,7 +1230,9 @@ class LLPanelLocalTexture final : public LLPanelLocalApplyAsset
     }
     LLUUID unitForPath(const std::string& path) const override
     {
-        return LLLocalBitmapMgr::getInstance()->getUnitID(path);
+        // User units only: a mesh-owned import of the same file is a distinct,
+        // read-only unit this tab must neither claim as loaded nor delete.
+        return LLLocalBitmapMgr::getInstance()->getUnitID(path, /*mesh_owned=*/false);
     }
     std::string pathForUnit(const LLUUID& tracking_id) const override
     {
@@ -1271,8 +1286,9 @@ class LLPanelLocalMaterial final : public LLPanelLocalApplyAsset
     }
     LLUUID unitForPath(const std::string& path) const override
     {
-        // A file holds >= 1 material; treat it as loaded if its first material is.
-        return LLLocalGLTFMaterialMgr::getInstance()->getUnitID(path, 0);
+        // A file holds >= 1 material; treat it as loaded if its first USER material
+        // is (a mesh-owned import of the same file is a distinct, read-only set).
+        return LLLocalGLTFMaterialMgr::getInstance()->getUnitID(path, 0, /*mesh_owned=*/false);
     }
     std::string pathForUnit(const LLUUID& tracking_id) const override
     {
@@ -1283,8 +1299,10 @@ class LLPanelLocalMaterial final : public LLPanelLocalApplyAsset
     }
     void unitsForPath(const std::string& path, std::vector<LLUUID>& out) const override
     {
-        // One .gltf/.glb can decode to several material units; remove them all.
-        LLLocalGLTFMaterialMgr::getInstance()->getTrackingIDs(path, out);
+        // One .gltf/.glb can decode to several material units; act on all of the
+        // USER's. Mesh-owned imports of the same file belong to a loaded mesh and
+        // must not be deleted from this tab.
+        LLLocalGLTFMaterialMgr::getInstance()->getTrackingIDs(path, out, /*mesh_owned=*/false);
     }
     std::string iconName() const override
     {

indra/newview/lllocalanim.cpp

@@ -26,6 +26,7 @@
 
 #include "lllocalanim.h"
 
+#include "fsyspath.h"           // UTF-8-safe std::filesystem paths on Windows
 #include "llbvhloader.h"
 #include "llcharacter.h"        // LLCharacter::sInstances (resolve avatar by id)
 #include "lldatapacker.h"
@@ -210,7 +211,7 @@ LLUUID LLLocalAnimMgr::loadAnim(const std::string& filename)
     if (!alias_deferred)
     {
         std::error_code ec;
-        anim.mLastModified = std::filesystem::last_write_time(filename, ec);
+        anim.mLastModified = std::filesystem::last_write_time(fsyspath(filename), ec);
     }
 
     const size_t bytes = anim.mData.size();
@@ -253,23 +254,29 @@ void LLLocalAnimMgr::delUnit(LLUUID tracking_id)
         return;
     }
 
-    // Stop it wherever it's playing, purge the cached motion, and drop the play map.
+    // Drop the play-map entries, then stop and delete the motion EVERYWHERE
+    // before freeing its cached keyframe data. The sweep covers instances
+    // mPlaying no longer tracks -- replaced/stopped ones still easing out and
+    // deprecated duplicates -- which would otherwise keep dereferencing the
+    // freed JointMotionList every frame.
     for (auto pit = mPlaying.begin(); pit != mPlaying.end(); )
     {
         if (pit->second == tracking_id)
         {
-            if (LLVOAvatar* av = resolve_avatar(pit->first))
-            {
-                av->stopMotion(tracking_id, true);
-                av->removeMotion(tracking_id);
-            }
             pit = mPlaying.erase(pit);
         }
         else
         {
             ++pit;
         }
     }
+    for (LLCharacter* character : LLCharacter::sInstances)
+    {
+        if (character)
+        {
+            character->purgeMotionInstances(tracking_id);
+        }
+    }
 
     LLKeyframeDataCache::removeKeyframeData(tracking_id);
     mAnims.erase(iter);
@@ -354,24 +361,27 @@ bool LLLocalAnimMgr::reapplyToAvatar(const LLUUID& av_id, const LLUUID& anim_id)
         return false;
     }
 
-    // Purge the stale parsed motion instance so createMotion() yields a fresh one
-    // that deserializes the new bytes. The id is keyed per-avatar, so the old motion
-    // must go before a new one can take its place.
-    av->stopMotion(anim_id, true);
-    av->removeMotion(anim_id);
-
+    // The caller (doUpdates) already purged the stale motion instances and the
+    // cached keyframe data, so createMotion() yields a fresh motion. The first
+    // avatar's deserialize() rebuilds and globally re-caches the new data;
+    // later avatars must NOT deserialize again -- every deserialize builds
+    // another JointMotionList and addKeyframeData() overwrites the cache slot
+    // without freeing it, leaking one list per extra avatar per reload.
     LLKeyframeMotion* motionp = dynamic_cast<LLKeyframeMotion*>(av->createMotion(anim_id));
     if (!motionp)
     {
         return false;
     }
-    LLDataPackerBinaryBuffer dp(iter->second.mData.data(), (S32)iter->second.mData.size());
-    if (motionp->deserialize(dp, anim_id, false))
+    if (!LLKeyframeDataCache::getKeyframeData(anim_id))
     {
-        av->startMotion(anim_id);
-        return true;
+        LLDataPackerBinaryBuffer dp(iter->second.mData.data(), (S32)iter->second.mData.size());
+        if (!motionp->deserialize(dp, anim_id, false))
+        {
+            return false;
+        }
     }
-    return false;
+    av->startMotion(anim_id);
+    return true;
 }
 
 void LLLocalAnimMgr::doUpdates()
@@ -385,7 +395,7 @@ void LLLocalAnimMgr::doUpdates()
         LocalAnim&    anim = entry.second;
 
         std::error_code ec;
-        const auto mtime = std::filesystem::last_write_time(anim.mFilename, ec);
+        const auto mtime = std::filesystem::last_write_time(fsyspath(anim.mFilename), ec);
         if (ec || mtime == anim.mLastModified)
         {
             continue;
@@ -398,16 +408,44 @@ void LLLocalAnimMgr::doUpdates()
         }
         anim.mData = std::move(fresh);
 
-        // Refresh the cached keyframe data so the next play uses the new bytes,
-        // and re-apply live to any avatar currently playing this id.
-        LLKeyframeDataCache::removeKeyframeData(id);
-        bool reapply_ok = true;
-        for (const auto& play : mPlaying)
+        // Collect the avatars to re-apply to, pruning entries whose avatar is
+        // gone -- counting a dead avatar as a reapply failure would leave the
+        // mtime unconsumed and re-parse + restart the anim on every live avatar
+        // every heartbeat, forever.
+        std::vector<LLUUID> replaying;
+        for (auto pit = mPlaying.begin(); pit != mPlaying.end(); )
         {
-            if (play.second == id)
+            if (pit->second == id)
             {
-                reapply_ok = reapplyToAvatar(play.first, id) && reapply_ok;
+                if (!resolve_avatar(pit->first))
+                {
+                    pit = mPlaying.erase(pit);
+                    continue;
+                }
+                replaying.push_back(pit->first);
             }
+            ++pit;
+        }
+
+        // Stop and delete every live instance BEFORE freeing the cached keyframe
+        // data: stopping dereferences the JointMotionList (setStopTime, constraint
+        // teardown), and instances mPlaying no longer tracks (replaced/stopped
+        // ones still easing out, deprecated duplicates) would keep reading the
+        // freed data every frame.
+        for (LLCharacter* character : LLCharacter::sInstances)
+        {
+            if (character)
+            {
+                character->purgeMotionInstances(id);
+            }
+        }
+        LLKeyframeDataCache::removeKeyframeData(id);
+
+        // Re-apply live to any avatar currently playing this id.
+        bool reapply_ok = true;
+        for (const LLUUID& av_id : replaying)
+        {
+            reapply_ok = reapplyToAvatar(av_id, id) && reapply_ok;
         }
 
         // Consume the mtime only once the swap is fully live AND the decode used joint