Skip to content

Appearance fixes #3492

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 9 commits into
base: archive/develop
Choose a base branch
from

Conversation

KittyBarnett
Copy link
Contributor

@KittyBarnett KittyBarnett commented Jan 31, 2025

This PR resolves a number of bugs related to avatar attachments and COF item tracking. These aren't new, but have existed in their current form dating back to at least when I filed VWR-18512.

Attached are a number of screengrabs that illustrate the various issues. It's not all of them, but they are the ones that are easy to reproduce on demand to verify that the PR actually does fix the underlying issues (retested today with tip of develop branch).

This PR is a prerequisite for the RLVa wardrobe support since it makes use of the LLAppearanceMgr/LLAttachmentMgr functionalities. @vir-linden


I've tried to split the commits across domain responsibilities, but at least some of them are interdependent in order to fix all the repros above.

  • Clean up unused code (n

    • This code hasn't been used for >10 years
  • LLViewerObject::getAttachmentItemID() sometimes returns the NULL UUID for the avatar's own attachments

    • If the object is already attached then LLViewerJointAttachment::removeObject() ends up getting called, which sets the object's item to the NULL UUID so we need to extract it the item id after the block, not before
    • Not easy to reproduce, but tracing execution makes it obvious that extractAttachmentItemID should be called after removeObject()
  • LLAppearanceMgr::updateAppearanceFromCOF() doesn't properly filter items collected from folder links

    • When a folder or outfit uses folder links you can end up picking the same inventory item multiple times, so deduping is needed
  • Resolve race condition when removing/add attachments inside of a LLWearableHoldingPattern

    • there's no guarantee that wearable holding patterns will complete in order, e.g. the first wear could be require an asset fetch while the second wear could be cached locally, the second will then complete first and when the first completes it will remove the attachments added by the second
    • if the asset fetch takes longer then one or more attachments could already be added/remove since its creation causing user frustration when removed things reattach or added things detach
  • Resolve issues tracking worn attachments that aren't in COF yet (see PR for more details)

    • Improves the tracking of "worn but not in COF" attachment and makes sure that they're included in each subsequent appearance change (until their COF link finally gets created)
  • If multiple LLWearableHoldingPattern instances are waiting for completion, only the most recently created one should progress

    • Smallest surface level fix
  • Accurately reflect the worn status of inventory items (the reasons for why this was needed have been resolved in previous commits)

    • Treating COF links as 'worn' was added as a way to address some of the issues in the screengrabs above, but it causes some issues of its own (and the underlying issues should be resolved by the code above)
  • Attaching a rezzed object incorrectly delays attachment link creation by 5 seconds

    • LLSelectNode::mItemID is NULL for a newly created in-world object or the old inventory item UUID if it was rezzed from inventory
    • a new inventory UUID will be assigned so there's no use in waiting for an item that will never come (and we should never wait on the NULL UUID)

Repros:

  1. Rez a prim
  2. Open inventory with Current Outfits visible
  3. Right-click the prim and Wear/Add
  4. Not that there's no COF item being created
  5. Wear any clothing layer

Observed: the box detaches
Post fix: the box remains attached

  1. Rez a prim and take it into inventory
  2. Duplicate it three times + rename so you can keep them apart
  3. Quickly double-click each one in succession (this may take 2-3 tries)
  4. Observe that only one cube is attached (they all attach to the same pointo so that's correct)

Observed: incorrect COF (one or more of the objects are linked in COF but aren't worn)

  1. Detach the one you are wearing

Observed: the detach happens, but another (unexpected to an end user) attachment pops into existence
Post-fix: COF and appearance are sync'ed (the code in llattachmentmgr largely caused this issue)


  1. Create some wearables (can be default)
  2. Create some attachments
  3. Select both wearables and attachments and then pick 'Wear'

Observed: the attachments come on, then the wearables, then the attachments detach
Post-fix: everything is worn as you'd expect


  1. Find a wearable that isn't cached locally (you'll have a bit more time for the third double click if the asset needs to fetch - see video)
  2. Create two attachments (not attaching to the same point)
  3. Double-click the first attachment
  4. Double-click the wearble
  5. Double-click the second attachment

Observed: perfect example of why syncing attachment inside of a wearable holding pattern is wrong
Post-fix: everything is correctly worn and attached


  1. Have a prim with the script in it ready to go (accept the permission prompt)
  2. Have an outfit ready to go (that isn't local wearable and simulator attachment asset fetched)
  3. Replace wear the outfit
  4. Immediately click the prim to attach it

Observed: the prim gets lots in the outfit change
Post-fix: the prim remains attached

This has the same cause as the others above but illustrates it's not just limited to single inventory wears. The worst case of this one is when you quickly switch between two outfits and end up wearing some of either and some linked in COF but unworn.

@github-actions github-actions bot added the c/cpp label Jan 31, 2025
@Ansariel
Copy link
Contributor

Ansariel commented Jan 31, 2025

This is 95% the code I removed from Firestorm because it randomly caused stuck attachments when changing outfits a lot. Also unclear if the same problems have been fixed in the meantime for RLVa itself: https://jira.firestormviewer.org/browse/FIRE-16497

The actual problem revolves around the way attachments get attached/detached via simulator messages and the delay until the correc status is reflected in COF. However, this usually isn't that much of an issue unless you add several attachments really fast - which is apparently what the RLVa wardrobe does. A different solution would probably that RLVa would collect the attachment changes, calculate the changes to COF and then use the AIS function to slam the new COF contents to COF in one go instead of sending attach/detach simulator messages each time such a RLVa command is getting processed.

@KittyBarnett
Copy link
Contributor Author

KittyBarnett commented Jan 31, 2025

This is 95% the code I removed from Firestorm because it randomly caused stuck attachments when changing outfits a lot. Also unclear if the same problems have been fixed in the meantime for RLVa itself: https://jira.firestormviewer.org/browse/FIRE-16497

Repro or it didn't happen. Sorry, but you've been banging on this drum and refusing to actually provide anyone with any reason. It just feels spiteful at this point.

I provided repros and explanations of what the changes do, please have the courtesy to counter with tangible facts.

These fixes also have absolutely nothing to do with RLVa; all repros involve performing actions residents do on a continuous basis.

@Ansariel
Copy link
Contributor

Ansariel commented Jan 31, 2025

I already explained it can happen when you change a lot of outfits - I mean a lot of outfit changes!

These fixes also have absolutely nothing to do with RLVa; all repros involve performing actions residents do on a continuous basis.

Quote from the PR description:

This PR is a prerequisite for the RLVa wardrobe support since it makes use of the LLAppearanceMgr/LLAttachmentMgr functionalities.

The issues itself do of course have nothing to do with RLVa, but the RLVa wardrobe relies on adding attachments/wearables in quick succession which then suffers from a lot of the issues above. However, I already explained an alternate solution for the RLVa wardrobe which doesn't involve spamming attach object simulator messages and rather use the same method that is used when changing outfits and that uses AIS, circumventing the race conditions and COF desync that happens when quickly adding or detaching attachments!

I'm not saying the existing bugs shouldn't be fixed, but I am saying these changes can result in stuck attachments. Some issues could be solved even better with a combination of server and viewer side changes, e.g. the whole process of sending a simulator message to attach an object, then wait for the attachment to arrive before the viewer itself updates COF to include the attachment which is the primary reason quickly attach/detaching attachments results in an indeterministic outcome. In fact, most of the issues in the description stem from this fact.

@KittyBarnett
Copy link
Contributor Author

I'm not saying the existing bugs shouldn't be fixed, but I am saying these changes can result in stuck attachments. Some issues could be solved even better with a combination of server and viewer side changes, e.g. the whole process of sending a simulator message to attach an object, then wait for the attachment to arrive before the viewer itself updates COF to include the attachment which is the primary reason quickly attach/detaching attachments results in an indeterministic outcome. In fact, most of the issues in the description stem from this fact.

Agreed that COF has always been problematic, but I would definitely challenge that any of these changes would make a practical difference to the problem of switching back and forth between outfit folders in quick succession. Also keep in mind that Firestorm has more changes than just these, it's a subset of, not the same.

And we don't get to make simulator PRs so this is the best we can do 😋 .

I also never said that none of the RLVa code won't change either either as it gets refactored, but the bugs above are a problem that needs addressing first, and they've been there for an eternity.

@Ansariel
Copy link
Contributor

Ansariel commented Feb 4, 2025

The reason I mentioned server side changes here is because unlike trying to make things somehow work solely in the viewer to sail around the broken process itself, contributing changes might have the chance of a combined effort together with LL to eliminate the actual source of the problem, namely the old legacy way how attachments are handled outside of a full outfit change - looking at @vir-linden right now...

Copy link
Contributor

@marchcat marchcat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code cleanup is always good, thanks!
However, we still have to decide how to address the issues mentioned above.

@akleshchev akleshchev requested review from bennettgoble and kylelinden and removed request for vir-linden February 12, 2025 12:24
@bennettgoble bennettgoble removed their request for review April 9, 2025 16:00
@akleshchev akleshchev requested a review from Geenz April 23, 2025 14:35
@Geenz
Copy link
Collaborator

Geenz commented Apr 28, 2025

@KittyBarnett Just checking in - any updates on the rebase work so we can keep this moving forward?

@Geenz Geenz marked this pull request as draft May 2, 2025 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants