Local Assets: post-merge defect-review fixes#298
Conversation
doUpdates() freed the globally cached JointMotionList (removeKeyframeData) BEFORE stopping the motions playing it; LLKeyframeMotion::setStopTime and constraint teardown then dereferenced the freed list on every live reload while a preview anim was playing. Stop everything first, then purge. Stopping via mPlaying alone also missed instances the map no longer tracks: replaced/stopped motions still easing out and deprecated duplicates from a double Play. New LLMotionController/LLCharacter::purgeMotionInstances() immediately excises EVERY instance of an id (canonical + deprecated); the delUnit/reload paths sweep all characters with it before freeing the cache. Also: - prune mPlaying entries whose avatar died instead of counting them as a reapply failure -- a dead entry left the mtime unconsumed, re-parsing the file and restarting the anim on every live avatar every 3s, forever - reapplyToAvatar deserializes only on cache miss: each deserialize builds a new JointMotionList and addKeyframeData() overwrites the slot without freeing, leaking one list per extra avatar per reload - wrap last_write_time in fsyspath like the sibling managers; non-ASCII Windows paths silently disabled live reload and the deferred BVH re-decode Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
onBtnRemove snapshotted raw LLScrollListItem pointers, but delUnit() now fires the units-changed signal synchronously: the picker's own onLocalAssetsChanged() -> refreshLocalList() -> clearRows() frees every row item mid-loop, and the next iteration read freed memory. Snapshot the (tracking id, asset type) values instead and mutate the managers after. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
LoadContext held a raw LLVOAvatar*; the DAE loader resolves (and writes joint-position overrides onto) joints through it from the model loader's worker thread. A teleport away from the spawn region or a logout mid-parse (despawnObjectsInRegion/cleanup) markDead()s the preview avatar and drops the manager's reference, freeing it under the loader. Hold it by LLPointer: joints on a dead-but-referenced avatar stay valid, and the ref releases on the main thread when the load callback frees the context. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ttach Remove never stuck: removePath() ran first, then delUnit() fired manager signals while the dying unit was still listed (mesh: despawnUnit's signal + owned-import releases fire before the list erase; materials: a multi- material file's sibling units survive each per-unit delUnit) -- and the add-only LLLocalAssetPaths::onUnitsChanged() re-recorded the path from getFilenames(), resurrecting the row every session. - LLLocalMeshMgr::delUnit() now delists the unit before any teardown that fires signals, so listeners never see a half-deleted unit - the floater Remove paths delete the units first and forget the path last Rez/Attach on an undecoded (dimmed) row decoded via addUnit's default include_joints=false, bypassing the saved flag loadPath applies -- and the next onUnitsChanged then erased the saved flag to match. addAndSpawn/ addAndAttach take include_joints now; the floater passes the persisted one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- sendSelectionMove() (the joystick/spacenav build-move path) was the one transform sender without the all-local gate; it shipped MultipleObjectUpdate blocks with localid 0 for previews - the grab tool sent ObjectGrab/ObjectGrabUpdate/ObjectDeGrab and the ObjectSpin* stream for previews (they spawn with FLAGS_OBJECT_MOVE, so permMove() let the grab activate); gate every send on isLocalOnly - canSelectObject()'s local/real homogeneity check sat below the mForceSelection early-return, so temp/right-click (forced) selects could build a mixed local+real selection -- every whole-selection local gate then fails open (localid-0 blocks go out, and selectDelete stops routing previews to the local delete path). Check homogeneity ahead of force-selection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The same-inventory-item dedup in LLViewerJointAttachment::addObject was guarded only for the INCOMING object being a local preview. A real attachment with a null item id (a temporary attachment carries no AttachItemID) landing on the same point still matched the worn preview's null item id, markDead()'d the preview, and -- since previews carry FLAGS_OBJECT_YOU_OWNER -- sent an ObjectDetach with localid 0 to the sim. Skip the dedup when the matched object is local-only too. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A user-loaded texture/material and a mesh-owned import of the same file are deliberately distinct units, but getUnitID/getTrackingIDs/getWorldIDsByName matched by filename alone. Depending on load order, the Textures/Materials tabs could claim a mesh's hidden import as their row's unit and delete it (stripping the material a loaded local mesh still renders with, while the user's own unit survives and re-records the just-removed path), and a mesh could bind its faces to the user's deletable copies instead of its own imports. The lookups now take the ownership class explicitly: - the floater tabs and picker resolve user units only (also un-hides the dimmed saved row when only a mesh-owned twin of the file is loaded) - mesh texture import reuses its own prior import first, then a user copy - mesh face binding maps mesh-owned materials, with the user set as a fallback only if the mesh-owned import failed to load Also stop clobbering LLLocalGLTFMaterial::mMaterialName on a failed re-decode (file locked mid-save): the blanked name made getWorldIDsByName fall back to "mat<index>" and bind faces wrongly during the retry window. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
forceReload() only checked mReloading, so toggling Joint Positions on a still-decoding unit (ST_LOADING) launched a second concurrent parse whose completions crossed states with the first -- a late failure would even delUnit a unit whose first parse had succeeded. Skip the reload in that state: ingestScene() reads the build options when the in-flight parse lands, so the new flag is picked up anyway. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Ctrl+D / shift-drag-copy on a preview was a silent no-op: the all-local gate swallowed the ObjectDuplicate send, and the gate comment wrongly claimed LLLocalMeshMgr already handled duplication. selectDuplicate() now routes an all-local selection to LLLocalMeshMgr::duplicatePreview(), which rezzes a fresh instance at the source copy's transform plus the offset (same user-delta rotation capture as respawnInstancesInPlace), honoring select_copy. Worn copies still don't duplicate, matching the sim path; duplicate-on-ray stays intentionally dropped, and the gate comment now says so. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
getDecomposition()/fetchPhysicsShape() queued repo-thread requests for local preview ids (reachable from physics-shape rendering and the build floater's physics tab); with no header and no HTTP they just retried and parked the id in mLoadingDecompositions/mLoadingPhysicsShapes forever. hasPhysicsShape() triggered the same fetch. Short-circuit all three for local ids like the repo's other entry points. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
onLocalAssetsChanged() re-selected via setSelectedByValue() with the row's map-valued LLSD; an LLSD map stringifies to "" under that comparator, so the highlight always jumped to the first enabled row after any local-asset change elsewhere. Match the unit id + type against the row values instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
writeToDisk() truncate-wrote the file in place; a crash mid-write left a truncated file that reloadForAccount() silently normalized to an empty working set, losing the saved paths. Write to a sibling .tmp and rename it over the old file (std::filesystem::rename replaces in one step). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
LLModelLoader::run() posted its completion with doOnIdleOneTime() from the loader's worker thread, but LLCallbackList has no synchronization -- a race the local-assets live reload turned from a rare upload-floater event into a steady-state mechanism (the upstream TODO at the call site even called it out). Post through the thread-safe "mainloop" WorkQueue instead, falling back to the idle list when no queue exists (unit tests / headless). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The mesh repo culls cached skins on a pure refcount check (mSkinMap entries with getNumRefs() == 1, every 10 seconds), so the refcount IS the skin's lifetime contract -- but the per-frame matrix-palette uploads read the skin through a raw LLDrawInfo pointer the cull can't see. A volume that dropped its reference (mesh id change, or a local-mesh reload/delete releasing the unit and volume refs mid-frame) left the render path one cull tick away from reading freed memory. LLDrawInfo::mSkinInfo becomes an LLPointer like its mAvatar: every in-world uploadMatrixPalette() call site reads the skin through a draw info, so the draw call now pins what it renders. LLFace::mSkinInfo stays a raw mirror (matching its mAvatar): faces and their group's draw infos are (re)built together in rebuildGeom, so a face's skin is always pinned by its volume or by the co-generated draw-info references -- the one face-based reader outside rebuilds is the octree debug display, covered by the same invariant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The skin cull posted mThread->mSkinMap.erase(id) for EVERY skin in the main map -- the post sat outside the getNumRefs() == 1 eviction check. The repo thread's map is a deep-copy mirror kept so volume loads can compute per-joint bounding boxes without taking the main map's path; erasing it unconditionally emptied the mirror within one 10s tick of arrival, quietly defeating that cache for any volume loading later than that, and posted one work item per cached skin every tick. Erase the mirror only for skins actually evicted from the main map, and batch the evicted ids into a single posted work item. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… edits restorePreservedFaces() re-applied every captured TE wholesale after applyPartGeometry(), so a reload that changed a face's texture, color, fullbright or material in the SOURCE FILE was immediately overwritten with pre-reload state -- live material edits never showed on spawned copies, and each swap re-captured the stale state so it stuck for good. Worse, the capture ran after ingestScene had already released dropped imports, whose teardown resets live faces to IMG_DEFAULT -- so a rebound face could capture-and-restore the placeholder forever. The restore now diffs instead: the live face state and the OLD parts' imported materials are snapshotted in onLoadResult BEFORE ingest commits, and after the new file state is applied only fields that differ from the old import -- i.e. actual user edits -- are restored (texture compared by local-bitmap tracking id too, so world-id rotation on a texture's own reload doesn't read as an edit). Fields no import can author (glow, transforms, bump/shiny, Blinn-Phong maps, glTF overrides) restore as-is. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eshes Import-time dedup collapses content-identical glTF materials onto one unit and records the other binding names as aliases, but a live reload only re-decoded each unit's own material index -- the alias structure was frozen at import. Editing the file so a formerly identical material diverges left its binding name aliased at the stale canonical unit forever (and faces bound to it rendering the wrong material); added materials got no unit, removed ones left orphans retrying. LLLocalGLTFMaterialMgr::doUpdates now runs a structural pass per changed (file, ownership class) group: surviving units keep their tracking/world ids and re-decode as before, diverged or newly added indices get fresh units, removed indices drop theirs, and every alias is rebuilt from the current per-index content signatures. Local meshes bind faces to these materials by name only at ingest, so when a regroup moves the file's name -> world id mapping the mesh manager now re-binds: LLLocalMeshFaceMaterial keeps the glTF binding name, and rebindFaceMaterials() re-resolves the stored face materials and re-points spawned faces -- skipping faces the user re-materialed in-world (the same only-if-still-showing-the-import rule as the hot-swap diff-restore), and updating the stored import so later hot-swap diffs compare correctly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds purgeMotionInstances APIs and uses them for safe anim reloads, disambiguates mesh-owned vs user-owned local assets, refactors local-mesh hot-swap with pre-swap snapshots and diff-restore, gates interactions for client-only previews, and applies infra fixes (work-queue handoff, atomic local-asset writes, skin-info ownership). ChangesMotion Purging Foundation for Animation Reloads
Local Anim Reload Flow
Asset Ownership Disambiguation Across Managers
Mesh Reload Hot-swap and Diff-restore
Local-only Preview Object Interaction Control
Infrastructure and Supporting Systems
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)indra/llprimitive/llmodelloader.cppindra/llprimitive/llmodelloader.cpp:27:10: fatal error: 'linden_common.h' file not found ... [truncated 1095 characters] ... lib/clang/18/include" indra/newview/lllocalassetpaths.cppIn file included from indra/newview/lllocalassetpaths.cpp:25: ... [truncated 1168 characters] ... lib/clang/18/include" indra/newview/lltoolgrab.cppIn file included from indra/newview/lltoolgrab.cpp:27: ... [truncated 1147 characters] ... nstall/lib/clang/18/include"
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
indra/newview/lltoolgrab.cpp (1)
584-611:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing
isLocalOnly()guard leaksObjectSpinUpdatemessages for local previews.When spinning a local-only object:
startSpin()correctly returns early (line 342-346) without sendingObjectSpinStart- However,
mSpinGrabbingis unconditionally set tospin_grabbingat line 546- Subsequent hover events enter this block and send
ObjectSpinUpdateto the serverThis defeats the purpose of the guards added to
startSpin()/stopSpin().🐛 Proposed fix: add isLocalOnly guard to spin update path
if (mSpinGrabbing) { //------------------------------------------------------ // Handle spinning //------------------------------------------------------ + + // Client-only local previews have no sim object to spin. + if (objectp->isLocalOnly()) + { + // Still update local rotation for visual feedback + LLVector3 up(0.f, 0.f, 1.f); + LLQuaternion rotation_around_vertical( dx*RADIANS_PER_PIXEL_X, up ); + const LLVector3 &agent_left = LLViewerCamera::getInstance()->getLeftAxis(); + LLQuaternion rotation_around_left( dy*RADIANS_PER_PIXEL_Y, agent_left ); + mSpinRotation = mSpinRotation * rotation_around_vertical; + mSpinRotation = mSpinRotation * rotation_around_left; + } + else + { // x motion maps to rotation around vertical axis LLVector3 up(0.f, 0.f, 1.f); ... msg->sendMessage( objectp->getRegion()->getHost() ); + } }Alternatively, a simpler fix if local rotation feedback isn't needed:
- if (mSpinGrabbing) + if (mSpinGrabbing && !objectp->isLocalOnly()) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indra/newview/lltoolgrab.cpp` around lines 584 - 611, The spin-update block unconditionally sends ObjectSpinUpdate for local-only objects; guard it by checking the object's isLocalOnly() (or the same local-only condition used in startSpin()/stopSpin()) before building/sending the message. In practice, modify the mSpinGrabbing handling path in lltoolgrab.cpp (the block that computes rotation_around_vertical/left, updates mSpinRotation, and sends ObjectSpinUpdate) to early-return or skip the message send when objectp->isLocalOnly() (or equivalent local-only flag) so previews do not emit network spin updates; keep local mSpinRotation updates only if you still want local visual feedback. Ensure you reference mSpinGrabbing, mSpinRotation, startSpin()/stopSpin(), and ObjectSpinUpdate when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@indra/llprimitive/llmodelloader.cpp`:
- Around line 196-208: The else branch must not call doOnIdleOneTime from the
worker thread; instead ensure LLModelLoader::loadModelCallback is scheduled onto
the main thread via a thread-safe enqueue mechanism. Change the fallback so that
when LL::WorkQueue::getInstance("mainloop") returns null you push the bound
callback (boost::bind(&LLModelLoader::loadModelCallback, this)) into a
thread-safe queue or a new helper (e.g.,
LLModelLoader::enqueueMainThreadCallback or a global thread-safe list) and have
the main loop drain and execute that queue; do not call doOnIdleOneTime from
run() on the worker thread. Ensure the new enqueue API and the main-loop drain
run only on the main thread.
In `@indra/newview/lllocalassetpaths.cpp`:
- Line 115: The final LLFile::rename(tmp_path, path) call in
lllocalassetpaths.cpp is currently ignoring its return value so rename failures
leave stale data and a temp file; modify the code around
LLFile::rename(tmp_path, path) to check its boolean result, and on failure log
an error including tmp_path and path, attempt cleanup of tmp_path (e.g.,
LLFile::remove or equivalent), and if appropriate attempt a safe fallback (such
as copying tmp_path to path and removing tmp_path) to avoid leaving stale state
and orphaned temp files. Ensure the change touches the code that writes out the
new XML and uses the tmp_path/path symbols so failures are handled and reported.
In `@indra/newview/llselectmgr.cpp`:
- Around line 4711-4737: The early-return path inside
selectionAllLocalPreview(mSelectedObjects) creates new_root previews via
LLLocalMeshMgr::duplicatePreview but skips setting up duplicate bookkeeping
(mDuplicated, mDuplicatePos, mDuplicateRot) so repeatDuplicate() won't recognize
them; after successfully creating each new_root in the new_roots loop, populate
mDuplicated[new_root->getID()] = original_root->getID() (or equivalent keying
used elsewhere) and record the corresponding mDuplicatePos and mDuplicateRot
using the original object's position/rotation (same fields updated in the normal
path), then proceed with the select_copy handling
(deselectAll/selectObjectAndFamily) and return—ensure you reference and use the
same identifiers and update logic as used for duplicates in the main
(non-local-preview) path so repeatDuplicate() will treat these previews as
duplicates.
---
Outside diff comments:
In `@indra/newview/lltoolgrab.cpp`:
- Around line 584-611: The spin-update block unconditionally sends
ObjectSpinUpdate for local-only objects; guard it by checking the object's
isLocalOnly() (or the same local-only condition used in startSpin()/stopSpin())
before building/sending the message. In practice, modify the mSpinGrabbing
handling path in lltoolgrab.cpp (the block that computes
rotation_around_vertical/left, updates mSpinRotation, and sends
ObjectSpinUpdate) to early-return or skip the message send when
objectp->isLocalOnly() (or equivalent local-only flag) so previews do not emit
network spin updates; keep local mSpinRotation updates only if you still want
local visual feedback. Ensure you reference mSpinGrabbing, mSpinRotation,
startSpin()/stopSpin(), and ObjectSpinUpdate when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cd88c03f-5e12-4324-b161-21b2bbfaae19
📒 Files selected for processing (21)
indra/llcharacter/llcharacter.cppindra/llcharacter/llcharacter.hindra/llcharacter/llmotioncontroller.cppindra/llcharacter/llmotioncontroller.hindra/llprimitive/llmodelloader.cppindra/newview/llfloaterlocalassets.cppindra/newview/lllocalanim.cppindra/newview/lllocalassetpaths.cppindra/newview/lllocalbitmaps.cppindra/newview/lllocalbitmaps.hindra/newview/lllocalgltfmaterials.cppindra/newview/lllocalgltfmaterials.hindra/newview/lllocalmesh.cppindra/newview/lllocalmesh.hindra/newview/llmeshrepository.cppindra/newview/llselectmgr.cppindra/newview/llspatialpartition.hindra/newview/lltexturectrl.cppindra/newview/lltoolgrab.cppindra/newview/llviewerjointattachment.cppindra/newview/llviewerwindow.cpp
- gate the ObjectSpinUpdate send on isLocalOnly: handleHoverActive re-sets mSpinGrabbing from the mask every hover, so startSpin()'s early-out alone did not keep local previews out of the spin-update block - make repeat-duplicate work for previews: the local selectDuplicate branch now mirrors the sim path's mDuplicated/mDuplicatePos/mDuplicateRot bookkeeping (when not select_copy), and repeatDuplicate() routes the copy-in-place through duplicatePreview for an all-local selection so the shared move-by-delta chain behaves like the sim path - check the final rename in LLLocalAssetPaths::writeToDisk and remove the temp file on failure (the old file stays as the persisted state) - document why the no-mainloop-queue fallback in LLModelLoader keeps using the idle list: the viewer always has the queue (gMainloopWork is a global), and invoking loadModelCallback() inline would deadlock -- it waits for this thread to stop, then deletes the loader Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
indra/newview/llselectmgr.cpp (1)
4729-4749:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply duplicate bookkeeping after the
select_copyreselection too.When
select_copyistrue, this branch reselects the new local copies but never setsmDuplicated/mDuplicatePos/mDuplicateRoton those fresh nodes.repeatDuplicate()then treats them as non-duplicates, deselects them at Line 4800, and no-ops on the next repeat. Move the bookkeeping loop so it runs for whichever roots remain selected after the optional reselection.Suggested fix
if (select_copy && !new_roots.empty()) { deselectAll(); for (LLViewerObject* new_root : new_roots) { selectObjectAndFamily(new_root); } } - else if (!select_copy) + if (!select_copy || !new_roots.empty()) { // Mirror the sim path's bookkeeping below so repeatDuplicate() // recognizes these as duplicated and chains the copy/move sequence. for (LLObjectSelection::root_iterator it = getSelection()->root_begin(); it != getSelection()->root_end(); ++it)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indra/newview/llselectmgr.cpp` around lines 4729 - 4749, The bookkeeping that marks nodes as duplicates (LLSelectNode::mDuplicated, mDuplicatePos, mDuplicateRot) must run after the optional reselection so it applies to whichever roots remain selected; move the loop that iterates getSelection()->root_begin()..root_end() out of the else branch and run it unconditionally after the select_copy handling (use node->getObject()->getPositionGlobal() and node->getObject()->getRotation() to populate mDuplicatePos and mDuplicateRot), so repeatDuplicate() will recognize the freshly selected copies as duplicated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@indra/newview/llselectmgr.cpp`:
- Around line 4729-4749: The bookkeeping that marks nodes as duplicates
(LLSelectNode::mDuplicated, mDuplicatePos, mDuplicateRot) must run after the
optional reselection so it applies to whichever roots remain selected; move the
loop that iterates getSelection()->root_begin()..root_end() out of the else
branch and run it unconditionally after the select_copy handling (use
node->getObject()->getPositionGlobal() and node->getObject()->getRotation() to
populate mDuplicatePos and mDuplicateRot), so repeatDuplicate() will recognize
the freshly selected copies as duplicated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a79cd5f1-56a5-4e8c-b22b-454e062af03c
📒 Files selected for processing (4)
indra/llprimitive/llmodelloader.cppindra/newview/lllocalassetpaths.cppindra/newview/llselectmgr.cppindra/newview/lltoolgrab.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- indra/newview/lllocalassetpaths.cpp
- indra/newview/lltoolgrab.cpp
- indra/llprimitive/llmodelloader.cpp
|
@coderabbitai Declining the select_copy bookkeeping suggestion on llselectmgr.cpp deliberately: it would diverge from the simulator path. With select_copy, the sim-created copy arrives freshly selected with no duplicate bookkeeping either (the client cannot set it -- the copy arrives asynchronously), so local previews currently behave identically to real objects in that flow. The select_copy=true caller is LLSelectMgr::duplicate() (Ctrl+D), whose repeat gesture is pressing Ctrl+D again -- that re-duplicates the fresh selection and already works for previews. repeatDuplicate() belongs to the shift-drag-copy chain, which uses select_copy=false and gets the bookkeeping. |
|
Tip For best results, initiate chat on the files or code changes.
(ᵔᴥᵔ) ✏️ Learnings added
|
1 participant
Follow-up to #296: fixes for the defects found in a deep post-merge review of the Local Assets feature (use-after-free, server-traffic leaks, persistence and live-reload correctness), plus two engine-level issues the review surfaced along the way.
Crash / use-after-free fixes
doUpdates()freed the globally cachedJointMotionListbefore stopping the motions playing it;setStopTime/constraint teardown then read freed memory on every reload while a preview anim played. NewLLMotionController/LLCharacter::purgeMotionInstances()excises every instance of an id — including deprecated duplicates still easing out, whichremoveMotion()can't reach — swept across all characters before the cache purge. Also prunes dead avatars from the play map (a dead entry made the reload retry + restart the anim on every live avatar every 3s, forever) and fixes aJointMotionListleak per extra avatar per reload.delUnit()started firing the units-changed signal synchronously (refresh →clearRows()); snapshot values instead.LoadContextheld a rawLLVOAvatar*the DAE loader dereferences (and writes joint overrides through) on the worker thread while region teardown could free the preview avatar; now held byLLPointer.LLDrawInfo::mSkinInfois now owning (like itsmAvatar): the mesh repo culls cached skins on a puregetNumRefs() == 1check every 10s, so the refcount is the skin's lifetime contract — but the per-frame matrix-palette uploads read through a raw pointer the cull couldn't see.LLFace::mSkinInfostays a raw mirror deliberately: faces and their group's draw infos are (re)built together, so a face's skin is always pinned by its volume or the co-generated draw infos.Local previews must never talk to the simulator
sendSelectionMove()(joystick/spacenav build-move) was the one transform sender without the all-local gate.ObjectGrab/ObjectGrabUpdate/ObjectDeGrab/ObjectSpin*for previews (they spawn withFLAGS_OBJECT_MOVE, so the grab activated); every send is now gated onisLocalOnly.canSelectObject()'s local/real homogeneity check sat below themForceSelectionearly-return, so right-click/temp selects could build a mixed selection that fails every whole-selection gate open; the check now runs ahead of force-selection.ObjectDetachwith localid 0; the dedup now skips when the matched object is local-only.getDecomposition/fetchPhysicsShape/hasPhysicsShapeshort-circuit local ids instead of parking them in the repo's loading sets forever.duplicatePreview().Remove / persistence correctness
removePath()ran beforedelUnit(), whose teardown signals fired while the dying unit was still listed — the add-only path persistence re-recorded it (mesh teardown order, and multi-material glTF sibling units).delUnitnow delists before signal-firing teardown and the floater forgets the path last.local_assets.xmlis written via temp file + rename so a crash mid-write can't blank the saved working set.Live-reload correctness
ingestScenecommits (ingest's release of dropped imports already resets faces), and only fields that differ from the old file's import — actual user edits — are restored over the freshly applied file state.mMaterialNameis no longer blanked by a failed re-decode (mid-save retry window), which mis-bound faces via themat<index>fallback.fsyspathlike the sibling managers (non-ASCII Windows paths silently disabled live reload).Engine-level
LLModelLoader::run()posted its completion viadoOnIdleOneTime()from the worker thread;LLCallbackListhas no synchronization (the upstream TODO at the call site says as much). It now posts through the thread-safe"mainloop"WorkQueue, with the idle-list fallback for headless/tests.mThread->mSkinMap.erase(id)for every cached skin every tick — outside the refcount check — emptying the repo thread's deep-copy mirror within one tick of arrival (defeating its per-joint bounding-box use for late-loading volumes). It now erases only evicted ids, batched into a single work item.🤖 Generated with Claude Code