-
Notifications
You must be signed in to change notification settings - Fork 449
fix: NetworkShow followed by ChangeOwnership generates ownership message error on target client #3468
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
fix: NetworkShow followed by ChangeOwnership generates ownership message error on target client #3468
Conversation
The server no longer sends the OwnershipChangeMessage to itself. The TrackOwnershipChangeSentMetric test was processing that extra message to just get to the actual ChangeOwnershipMessage metric. I removed that portion of this test.
…tra-ownership-message
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 fix!
/// <param name="clientId">the client to check</param> | ||
/// <param name="networkObject">the <see cref="NetworkObject"/> to check if it is pending show</param> | ||
[System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)] | ||
internal bool IsObjectVisibilityPending(ulong clientId, ref NetworkObject networkObject) |
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.
I like this!
@@ -80,6 +80,8 @@ namespace Unity.Netcode.RuntimeTests | |||
internal class NetworkTransformTests : NetworkTransformBase | |||
{ | |||
protected const int k_TickRate = 60; | |||
|
|||
protected const int k_DefaultTimeTravelFrames = 1000; |
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.
Just out of interest, what does this change do?
…Show with ChangeOwnership fixes [Backport] (#3493) The PR resolves the issue discovered when investigating the [Forum-1651980 issue](https://discussions.unity.com/t/netcode-for-gameobjects-2-4-0-released/1651980/2). The initial client synchronization pre-serialization preparation was not excluding any spawned `NetworkObject` instances that had a pending visibility update for the client being synchronized. This fix adds this check to that process. [MTTB-1372](https://jira.unity3d.com/browse/MTTB-1372) This addresses the [Forum-Support-1646996](https://discussions.unity.com/t/does-networkshow-assign-ownership-internally-in-unity-netcode/1646996/10) issue where it was determined if you invoke `NetworkObject.NetworkShow` and then immediately invoke `NetworkObject.ChangeOwnership` (or somewhere within the same callstack for that frame) the target client will get an error regarding an unnecessary `ChangeOwnershipMessage`. [MTTB-1337](https://jira.unity3d.com/browse/MTTB-1337) ## Changelog - Fixed: Issue where the initial client synchronization pre-serialization process was not excluding spawned `NetworkObjects` that already had pending visibility for the client being synchronized. - Fixed: Issue where invoking `NetworkObject.NetworkShow` and `NetworkObject.ChangeOwnership` consecutively within the same call stack location could result in an unnecessary change in ownership error message generated on the target client side. ## Testing and Documentation - Includes integration tests. - No Documentation changes were required. <!-- Uncomment and mark items off with a * if this PR deprecates any API: ### Deprecated API - [ ] An `[Obsolete]` attribute was added along with a `(RemovedAfter yyyy-mm-dd)` entry. - [ ] An [api updater] was added. - [ ] Deprecation of the API is explained in the CHANGELOG. - [ ] The users can understand why this API was removed and what they should use instead. --> ## Backport This is a back port of #3488. This is a partial back port of #3468. <!-- If this is a backport: - Add the following to the PR title: "\[Backport\] ..." . - Link to the original PR. If this needs a backport - state this here If a backport is not needed please provide the reason why. If the "Backports" section is not present it will lead to a CI test failure. -->
Summary
MTTB-1337
This addresses the Forum-Support-1646996 issue where it was determined if you invoke
NetworkObject.NetworkShow
and then immediately invokeNetworkObject.ChangeOwnership
(or somewhere within the same callstack for that frame) the target client will get an error regarding an unnecessaryChangeOwnershipMessage
.Because
NetworkObject.NetworkShow
invocations are actually queued and handled at the end of the netcode frame, theChangeOwnershipMessage
would proceed theCreateObjectMesage
(NetworkShow). Since theCreateObjectMesage
is created at the end of the frame for each queuedNetworkObject.NetworkShow
invocation, when created it would use the new owner assigned earlier and ownership would be applied upon showing/spawning theNetworkObject
on the target client side. Upon theNetworkObject
becoming "visible"/spawned, the deferredChangeOwnershipMessage
(because it was processed before theNetworkObjct
was created) would be handled on the target client side and would detect the newly spawnedNetworkObject
was already owned by the client and log an error message.CMB Service Update: (Jira ticket required)
This PR includes a modification to the
ChangeOwnershipMessage
generated when a change in ownership occurs during a CMB Service hosted distributed authority session. These modifications should be compatible with current and previous versions of the CMB Service.The NGO-SDK Change:
ChangeOwnershipMessage.ClientIds
is populated with a list of all clients that should receive the message.ChangeOwnershipMessage.ClientIdCount
provides the number of clients (size of the array).CMB Service Update:
ChangeOwnershipMessage
for a full ownership change.Changelog
NetworkObject.NetworkShow
andNetworkObject.ChangeOwnership
consecutively within the same call stack location could result in an unnecessary change in ownership error message generated on the target client side.Testing and Documentation
NetworkObjectOwnershipTests.NetworkShowAndChangeOwnership
.Backport
Partial backport is required (#3493)