Skip to content

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

Conversation

NoelStephensUnity
Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity commented May 29, 2025

Summary

MTTB-1337

This addresses the Forum-Support-1646996 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.

Because NetworkObject.NetworkShow invocations are actually queued and handled at the end of the netcode frame, the ChangeOwnershipMessage would proceed the CreateObjectMesage(NetworkShow). Since the CreateObjectMesage is created at the end of the frame for each queued NetworkObject.NetworkShow invocation, when created it would use the new owner assigned earlier and ownership would be applied upon showing/spawning the NetworkObject on the target client side. Upon the NetworkObject becoming "visible"/spawned, the deferred ChangeOwnershipMessage (because it was processed before the NetworkObjct was created) would be handled on the target client side and would detect the newly spawned NetworkObject 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:

  • Use the above properties when forwarding the ChangeOwnershipMessage for a full ownership change.

Changelog

  • 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 test NetworkObjectOwnershipTests.NetworkShowAndChangeOwnership.
  • No documentation changes or additions were necessary.

Backport

Partial backport is required (#3493)

This resolves the NetworkShow, ChangeOwnership, and then the additional ChangeOwnershipMessage issue for host, server, and DAHost.

For the CMB service, we need to adjust ChangeOwnershipMessage to contain the list of target client identifiers the message should be sent to.
This is Emma's test added to the NetworkShowHideTests.
Comment adjustment.
Renaming the test.
white space removal.
Removing original quick troubleshooting script no longer used.
Reverting previous name change.
Applying name change to the correct test...
>.<
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.
Removing debug logging.
Removing exception and using error log and continue processing sending to any remaining clients.
Starting to look at the reoccurring OSX failures with some transform tests.
@NoelStephensUnity NoelStephensUnity marked this pull request as ready for review May 31, 2025 13:39
@NoelStephensUnity NoelStephensUnity requested a review from a team as a code owner May 31, 2025 13:39
@NoelStephensUnity NoelStephensUnity marked this pull request as draft May 31, 2025 13:39
This update utilizes some already existing properties within the ChangeOwnershipMessage to define which clients should receive the change in ownership message.
adding change log entry
@NoelStephensUnity NoelStephensUnity marked this pull request as ready for review May 31, 2025 16:50
@NoelStephensUnity NoelStephensUnity enabled auto-merge (squash) May 31, 2025 17:34
Updating the logic behind the list of client identifiers that should receive the ChangeOwnershipMessage when running a distributed authority network topology.
Copy link
Collaborator

@EmandM EmandM left a 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)
Copy link
Collaborator

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;
Copy link
Collaborator

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?

@NoelStephensUnity NoelStephensUnity merged commit 964e0ca into develop-2.0.0 Jun 2, 2025
42 checks passed
@NoelStephensUnity NoelStephensUnity deleted the fix/networkshow-ownership-change-extra-ownership-message branch June 2, 2025 15:34
@NoelStephensUnity NoelStephensUnity added the port:1.x-completed This issue was ported to 1.X branch label Jun 9, 2025
NoelStephensUnity added a commit that referenced this pull request Jun 10, 2025
…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.
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
port:1.x-completed This issue was ported to 1.X branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants