chore: simplify accessors network object #3831
Conversation
com.unity.netcode.gameobjects/Runtime/Components/NetworkTransform.cs
Outdated
Show resolved
Hide resolved
com.unity.netcode.gameobjects/Runtime/Components/NetworkTransform.cs
Outdated
Show resolved
Hide resolved
| // Only dynamically spawned NetworkObjects that are not already in the newly assigned active scene will migrate | ||
| // and update their scene handles | ||
| if (IsSceneObject.HasValue && !IsSceneObject.Value && gameObject.scene != next && gameObject.transform.parent == null) | ||
| if (IsSceneObject.HasValue && !IsSceneObject.Value && m_SceneOrigin != next && m_CachedParent == null) |
There was a problem hiding this comment.
m_SceneOrigin is the actual one or the starting one (is a ref or a copy)
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
But are they for transform.parent just bellow?
| // Only dynamically spawned NetworkObjects that are not already in the newly assigned active scene will migrate | ||
| // and update their scene handles | ||
| if (IsSceneObject.HasValue && !IsSceneObject.Value && gameObject.scene != next && gameObject.transform.parent == null) | ||
| if (IsSceneObject.HasValue && !IsSceneObject.Value && m_SceneOrigin != next && m_CachedParent == null) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
noellie-velez
force-pushed
the
chore/simplify-accessors-network-object
branch
from
02d061d to
aab3f4d
Compare
| // Early exit if the NetworkManager is shutting down, the NetworkObject | ||
| // is not spawned, or an in-scene placed NetworkObject | ||
| if (NetworkManager == null || NetworkManager.ShutdownInProgress || !IsSpawned || IsSceneObject != false) | ||
| if (!IsSpawned || IsSceneObject != false || NetworkManager.ShutdownInProgress) |
There was a problem hiding this comment.
IsSceneObject is a bool? type which means the value can be null | true | false so this check needs to return true when the value is null | true but false when the value is false.
That is to say, this check here is already a contraction of (IsSceneObject == null || IsSceneObject.HasValue). Unfortunately we can't contract it further.
| } | ||
|
|
||
| if (transform.parent.parent != null) | ||
| if (m_CachedParent.parent != null) |
There was a problem hiding this comment.
m_CachedParent is not guaranteed to be the same thing as transform.parent here. This is not a safe swap to make
| public void DeferDespawn(int tickOffset, bool destroy = true) | ||
| { | ||
| if (!NetworkManager.DistributedAuthorityMode) | ||
| if (!IsSpawned) |
There was a problem hiding this comment.
I synced with Noel. For this function we actually want to make the if (!DistributedAuthorityMode) check before we make the IsSpawned check because this functionality is not valid to be used in DA mode.
The relevancy of error messages in this spot should be:
- Things that indicate an underlying issue with how the game is structured. Here that'd be trying to use a DA feature in non DA mode.
- Runtime issues that could feasibly happen in gameplay
| NetworkLog.LogError($"This method is only available in distributed authority mode."); | ||
| if (NetworkManager.LogLevel == LogLevel.Error) | ||
| { | ||
| NetworkLog.LogErrorServer("[Attempted deferred despawn while un-spawned]"); |
There was a problem hiding this comment.
We want to avoid changing the error messages if we can help it.
Due to the amount of chaos in this codebase, the consistency of messages that we want is:
- Be consistent within this function - if this function has a style we should follow it
- Match the history. Avoid changing anything user facing if we don't need to
- Consistency within the whole class or file.
If at some point we decide what logs we want we can do a big push at one time and change everything. Otherwise, it's better to keep the changes small
| } | ||
|
|
||
| // Do the safety loop first to prevent putting the netcode in an invalid state. | ||
| for (int i = 0; i < networkObjects.Count; i++) |
There was a problem hiding this comment.
Fun fact, we can remove this whole loop, as all these checks should be done in the underlying function.
I think the most important next task for you is to pull out these exceptions!
| } | ||
|
|
||
| // Do the safety loop first to prevent putting the netcode in an invalid state. | ||
| for (int i = 0; i < networkObjects.Count; i++) |
There was a problem hiding this comment.
Same thing here, just remove this whole pre-process loop
| // With distributed authority, we need to track "valid authoritative" parenting changes. | ||
| // So, either the authority or AuthorityAppliedParenting is considered a "valid parenting change". | ||
| // If we do not have authority and we are spawned | ||
| if (!(HasAuthority || AuthorityAppliedParenting || (AllowOwnerToParent && IsOwner))) |
There was a problem hiding this comment.
So sorry for the churn, turns out that this is definitely easier to read with the variable assignment before the if check. That one's completely my bad 😸
| // With distributed authority, we need to track "valid authoritative" parenting changes. | |
| // So, either the authority or AuthorityAppliedParenting is considered a "valid parenting change". | |
| // If we do not have authority and we are spawned | |
| if (!(HasAuthority || AuthorityAppliedParenting || (AllowOwnerToParent && IsOwner))) | |
| // With distributed authority, we need to track "valid authoritative" parenting changes. | |
| // So, either the authority or AuthorityAppliedParenting is considered a "valid parenting change". | |
| var isParentingAuthority = HasAuthority || AuthorityAppliedParenting || (AllowOwnerToParent && IsOwner); | |
| // If we do not have authority and we are spawned | |
| if (!isParentingAuthority) |
| if (!AlwaysReplicateAsRoot && obj.HasParent) | ||
| { | ||
| var parentNetworkObject = transform.parent.GetComponent<NetworkObject>(); | ||
| var parentNetworkObject = m_CachedParent.GetComponent<NetworkObject>(); |
There was a problem hiding this comment.
m_CachedParent is also not necessarily the same thing here. Safer to leave it the slow way than to risk a bug
| if (networkBehaviour.IsSpawned && IsSpawned) | ||
| { | ||
| if (NetworkManager?.LogLevel == LogLevel.Developer) | ||
| if (NetworkManagerOwner?.LogLevel == LogLevel.Developer) |
There was a problem hiding this comment.
Tiny nit, it really doesn't matter, but we don't need the nullable invoke if we're spawned because NetworkManagerOwner should be trusted as not null
| if (NetworkManagerOwner?.LogLevel == LogLevel.Developer) | |
| if (NetworkManagerOwner.LogLevel == LogLevel.Developer) |
Purpose of this PR
Jira ticket
MTT-13657
Changelog
Documentation
Testing & QA (How your changes can be verified during release Playtest)
Functional Testing
Manual testing :
Manual testing doneAutomated tests:
Covered by existing automated testsCovered by new automated testsDoes the change require QA team to:
Review automated tests?Execute manual tests?Provide feedback about the PR?If any boxes above are checked the QA team will be automatically added as a PR reviewer.
Backports