fix: owner changing ownership causing identical previous and current ids when using a distributed authority network topology #3347
Conversation
This fixes an issue with the synchronization of NetworkVariables when changing ownership where it was possible to have the previous owner be equal to the current owner when using a distributed authority network topology and the owner handled changing the ownership. This includes an additional check for ditry states of any collections base NetworkVariables. This bug was introduced as a regression bug when resolving the issue that any owner read permission NetworkVariables were fully synchronized with the new owner.
|
|
NoelStephensUnity
changed the title
…invalid-previous-and-current-clientids
…invalid-previous-and-current-clientids
| { | ||
| if (NetworkManager.LogLevel <= LogLevel.Developer) | ||
| { | ||
| Debug.LogWarning($"[{nameof(NetworkSpawnManager)}][{nameof(ChangeOwnership)}] Attempting to change ownership to Client-{clientId} when the owner is already {networkObject.OwnerClientId}! (Ignoring)"); |
There was a problem hiding this comment.
This check is already happening on line 524 of this function, though the check is a little different:
if (networkObject.OwnerClientId == clientId && networkObject.PreviousOwnerId == clientId)We should probably consolidate the two checks.
There was a problem hiding this comment.
The issue was that there are several places that invoke NetworkSpawnManager.ChangeOwnership:
I decided to put a check at the funnel point. However, at some point in the future we might look at all of the invocations to determine if it makes sense to consolidate the various checks for this condition... for now we know that we will at least see warnings...maybe I should make that an error so it will fail any PRs that introduce this condition?
There was a problem hiding this comment.
I think the bigger thing is that the check is happening twice in the same function. So the same code path is going to check the same check twice.
…invalid-previous-and-current-clientids
…invalid-previous-and-current-clientids
…invalid-previous-and-current-clientids
| { | ||
| if (NetworkManager.LogLevel <= LogLevel.Developer) | ||
| { | ||
| Debug.LogWarning($"[{nameof(NetworkSpawnManager)}][{nameof(ChangeOwnership)}] Attempting to change ownership to Client-{clientId} when the owner is already {networkObject.OwnerClientId}! (Ignoring)"); |
There was a problem hiding this comment.
I think the bigger thing is that the check is happening twice in the same function. So the same code path is going to check the same check twice.
NoelStephensUnity
deleted the
fix/da-change-ownership-results-in-invalid-previous-and-current-clientids
branch
This fixes an issue with the synchronization of NetworkVariables when changing ownership where it was possible to have the previous owner be equal to the current owner when using a distributed authority network topology and the owner handled changing the ownership.
This includes an additional check for ditry states of any collections base NetworkVariables.
fix: #3343
close: #3343
Changelog
NetworkTransformto fail to change ownership which would leave the previous owner still subscribed to network tick events.Testing and Documentation
NetworkObjectOwnershipTests.TestAuthorityChangingOwnership.