FIRE-31264: Temp attachments attached by experinece are ghosted.

[Error-LP0]

Experience detach request on region / parcel change occurs before fsExperimentalLostAttachmentsFixKillDelay expires. Added a bypass to FIRE-12004 if attached in a temporary slot.

[beqjanus]

Hiya,
Thanks for this, it looks like an interesting corner case but I'm not convinced the fix you are proposing does what it intends so you may need to explore a different solution.

I am not an expert in this area, @Ansariel might want to correct me if I am wrong here.
I believe that objectp->isAttachment() returns false for temp attachments as it is based on mAttachmentState, as such:
if it is a temp attachment the first clause will be false and the isTempAttachment test will never be evaluated.
if it is not a temp attachment but is an attachment then the second test is evaluated but will always be true (i.e. not a temp).

(note that if isAttachment did return true for temps then the second check would be unnecessary in the current code (and we see it in various other places in the code base.)

Also, an unrelated point on the change tagging.
If more than one line is modified you are best to wrap te change in a open/close tag pair with the original code as the first commented line(s) of the pair. That would give us something closer to this in the current change. I can do it post merge but its cleaner to do upfront if you can.

                //<FS:TT-lp0> FIRE-31264: Temp attachments attached by experinece are ghosted. 
                //(objectp->isAttachment() || objectp->isTempAttachment()) &&
                //Window for experience to request detach on change to non-experienced area is shorter than the default FixKillDelay.
                (objectp->isAttachment() && !objectp->isTempAttachment()) && 
                // </FS:TT-lp0>

[Ansariel]

PR looks good to me, but also checked to be sure: For temp attachments, both isAttachment() and isTempAttachment() are true and after a TP it seems to be fine to detach temp attachments (assuming temp attachments should always get detached after a TP)

[Error-LP0]

After a few tests with an experience attached object, was unable to cause the temporary object to detach on its own without issuing a llDetachFromAvatar()

[Ansariel]

We had some internal discussion which reminded me why the check is the way it is: Apparently there are two different different ways temporary attachments are supposed to be handled during a TP:

1. "Normal" temp attachments that got attached via script: These are supposed to stay attached until they either get explicitly removed, e.g. by user action, or until you logout.
2. Temp attachments from experiences: These are supposed to stay for the course of the experience and detach once you leave, e.g. by teleport.

Problem: So far I - at least - couldn't find a way to determine what kind of temp attachment the worn object is. So there had to be a decision made if temp attachments should be included in the viewer-side workaround for attachments getting erroneously detached (case 1) or excluded (case 2). Latter could of course mean you might lose a normal temp attachment during a TP because of the server-side bug.

[Error-LP0]

That was also something I noticed, the lack of information relating to an experience, except for a hint from a debug message mentioning that the attachment arrived unexpected or late. Started looking through the RLV part to see how it's handled, but if I understand RLV, commands are processed by the viewer, and attach/detach requests originate from the viewer, making them expected messages. Experience related attach/detach originate server side, and attach is reliably generating the unexpected / late attachment warning.

WARNING # newview/llattachmentsmgr.cpp(418) onAttachmentArrived : ATT Attachment was unexpected or arrived after 30 seconds:

[beqjanus]

So we're kinda stuck, I am not sure whether this change fixes more things than it breaks. I'm gonna reach out to LL and see if I can get more info.

[beqjanus]

Just to update, LL are looking into whether there is a way to determine this. I suspect this means there is not, in which case we need to somehow work out which of the two options is more likely to happen across the whole user base.