Skip to content

fix: Attachables destroy order of operations #3931

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.

Sign up for GitHub

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
NoelStephensUnity wants to merge 1 commit into
base: develop-2.0.0
Choose a base branch
from
+31 −7
Draft

fix: Attachables destroy order of operations #3931

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,8 @@ internal void ForceComponentChange(bool isAttaching, bool forcedChange)

foreach (var componentControllerEntry in ComponentControllers)
{
if (componentControllerEntry.AutoTrigger.HasFlag(triggerType))
// Only if the component controller still exists and has the appropriate flag.
if (componentControllerEntry.ComponentController && componentControllerEntry.AutoTrigger.HasFlag(triggerType))
{
componentControllerEntry.ComponentController.ForceChangeEnabled(componentControllerEntry.EnableOnAttach ? isAttaching : !isAttaching, forcedChange);
}
Expand Down Expand Up @@ -459,7 +460,14 @@ internal void InternalDetach()
{
if (m_AttachableNode)
{
if (m_DefaultParent)
// TODO-FIX: We might track if something has been "destroyed" in order
// to be able to be 100% sure in the event a user disables the world item
// when detatched. Otherwise, we keep this in place and make note of it
// in documentation.
// Issue:
// Edge-case where the parent could be in the middle of being destroyed.
// If not active in the hierarchy, then don't attempt to set the parent.
if (m_DefaultParent && m_DefaultParent.activeInHierarchy)
{
// Set the original parent and origianl local position and rotation
transform.SetParent(m_DefaultParent.transform, false);
Expand Down
26 changes: 21 additions & 5 deletions com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,20 +71,36 @@ public override void OnNetworkPreDespawn()
{
for (int i = m_AttachedBehaviours.Count - 1; i >= 0; i--)
{
if (!m_AttachedBehaviours[i])
var attachable = m_AttachedBehaviours[i];
if (!attachable)
{
continue;
}
// If we don't have authority but should detach on despawn,
// then proceed to detach.
if (!m_AttachedBehaviours[i].HasAuthority)
if (!attachable.HasAuthority)
{
m_AttachedBehaviours[i].ForceDetach();
attachable.ForceDetach();
}
else
{
// Detach the normal way with authority
m_AttachedBehaviours[i].Detach();
// TODO-FIX: We might track if something has been "destroyed" in order
// to be able to be 100% sure this is specific to being destroyed.
// Otherwise, we keep this in place and make note of it
// in documentation that you cannot detatch from something already despawned.
// Issue: When trying to detatch if the thing attached is no longer
// spawned. Instantiation order recently changed such that
// the attachable =or= the attach node target could be despawned
// and in the middle of being destroyed. Resolution for this
// is to skip over destroyed object (default) and then only sort
// through the things the local NetworkManager instance has authority
// over. Even then, we have to check if the attached object is still
// spawned before attempting to detatch it.
if (attachable.IsSpawned)
{
// Detach the normal way with authority
attachable.Detach();
}
}
}
}
Expand Down