-
Notifications
You must be signed in to change notification settings - Fork 459
chore: Optimize NetworkObject access from NetworkBehaviour #3687
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
chore: Optimize NetworkObject access from NetworkBehaviour #3687
Conversation
When you will be ready you can ping me and I can organize the Playtest. Just note that I'm off next week |
/// Invokes the <see cref="ChildNetworkBehaviours"/> <see cref="NetworkBehaviour.OnLostOwnership"/> and <see cref="NetworkBehaviour.OnGainedOwnership"/> events. | ||
/// <see cref="NetworkSpawnManager.UpdateOwnershipTable"/> is called to update the ownership in-between the two callbacks. | ||
/// </summary> | ||
internal void InvokeBehaviourOnOwnershipChanged(ulong originalOwnerClientId, ulong newOwnerClientId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done!
Collapsed, organized, and unified.
🥇
{ | ||
networkObject.InvokeBehaviourOnGainedOwnership(); | ||
} | ||
// Notify lost ownership, update the ownership, then notify gained ownership for the network behaviours |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flex the unification!
// Always notify locally on the server when a new owner is assigned | ||
networkObject.InvokeBehaviourOnGainedOwnership(); | ||
// Notify lost ownership, update the ownership, then notify gained ownership for the network behaviours | ||
networkObject.InvokeBehaviourOnOwnershipChanged(originalOwner, clientId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔔
protected IEnumerator WaitForSpawnedOnAllOrTimeOut(NetworkObject networkObject, TimeoutHelper timeOutHelper = null) | ||
{ | ||
var networkObjectId = networkObject.GetComponent<NetworkObject>().NetworkObjectId; | ||
var networkObjectId = networkObject.NetworkObjectId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice addition!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice.... 👍
Changing #3687 adjustment to use the version of SpawnNetworkObjectLocally that accepts a SceneObject which doesn't invoke pre-spawn but does invoke post spawn and processes deferred messages. These were moved in this PR to assure post spawn was invoked after the object was 100% done locally spawning and as a last step process any deferred messages targeting this object.
Purpose of this PR
Methods inside of properties are much less performant to access that fields or methods without properties.
This PR focusses on:
NetworkBehaviour.NetworkObject
from withinNetworkBehaviour
. Instead usingm_NetworkObject
anywhere that will be called after theNetworkBehaviour
is spawned.a. Ensure that the
NetworkObject
explicitly setsm_NetworkManager
when it calculates whether or not aNetworkBehaviour
belongs in it'sChildNetworkBehaviours
list.NetworkBehaviour
lifecycle callbacks are invokeda. Simplifies the callsites around
NetworkBehaviour.OnLostOwnership
andNetworkBehaviour.OnGainedOwnership
b. Reduces loop duplication around
NetworkBehaviour.OnNetworkSpawn
Jira ticket
Link to related jira ticket (Use the smart commits). Short version (e.g. MTT-123) also works and gets auto-linked
to be created.
Changelog
NetworkBehaviour
performance.Documentation
Testing & QA (How your changes can be verified during release Playtest)
This change needs a full play of astroids, and potentially the OwnershipChanging demo. Everything here should be covered by the automated tests, but this is a hot enough codepath that playtesting is definitely needed here.
Functional Testing
Manual testing :
Manual testing done
Automated tests:
Covered by existing automated tests
Covered by new automated tests
Does 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
This is a pure performance improvement and so doesn't need a backport.