Skip to content
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: networkvariable collections can be modified without write permissions [backport] #3126

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

NoelStephensUnity
Copy link
Collaborator

This is a backport of #3081.

This PR resolves the issue where a client or server could modify a collection with no write permissions. The underlying issue is that default .NET collections have no way to be notified when the collection is modified. Since polling each instance is not an option, the lowest risk fix that continues to support standard .NET collections was to:

  • Add a 3rd internal duplicated current value property that can be used to compare against the current internal value reading (applying) a delta or field value. This helps to avoid edge case scenarios where the current collection value on a client with no write permissions could be modified and a state update is received.
  • Update NetworkVariable.CheckDirtyState and NetworkVariable.IsDirty such that the same form of comparison occurs to provide users with an additional way to check collection integrity on clients without write permissions while also being able to control how often the collection is checked.

This PR also resolves:

  • The issue where when one or more clients is/are already connected, entries are added by the current owner, the ownership changes to one of the connected clients, and finally the new client owner adds an entry. Since the new client's previous value always lags behind the last update, the new client will send both the new entry and the previously added entry which causes an exception on non-owner clients (duplicate key entry) and/or additional duplicate entries could be added (i.e. a list).
  • The issue where changing ownership would mark all NetworkVariables dirty as opposed to:
    • Marking any owner read permissions NetworkVariables as dirty when sending to the new owner (both within NetworkSpawnManager for server-side update and within the NetworkVariableDeltaMessage).
    • Sending any pending updates to all clients prior to sending the change in ownership message to assure pending updates are synchronized as the owner.
  • The issue where a newly synchronizing client would be synchronized with the current NetworkVariable values always which could cause issues with collections if there were any pending state updates. Now, when initially synchronizing a client, if a NetworkVariable has a pending state update it will serialize the previously known value(s) to the synchronizing client so when the pending updates are sent they don't duplicate values.

This PR also changes:
NetworkVariableDeltaMessage now includes the ability for a server to immediately forward delta state updates (to valid clients on a per NetworkVariable field basis) without having to queue them for end of frame (i.e. since messages are processed at the beginning of a frame, this shaves off the state replication time to other clients by close to the total frame time...which depending upon your project could mean 10-16ms faster).

Changelog

  • Changed: NetworkVariableDeltaMessage so the server now forwards delta state updates (owner write permission based from a client) to other clients immediately as opposed to keeping a NetworkVariable or NetworkList dirty and processing them at the end of the frame or potentially on the next network tick.
  • Fixed: Issue where a newly synchronizing client would be synchronized with the current NetworkVariable values always which could cause issues with collections if there were any pending state updates. Now, when initially synchronizing a client, if a NetworkVariable has a pending state update it will serialize the previously known value(s) to the synchronizing client so when the pending updates are sent they aren't duplicate values on the newly connected client side.
  • Fixed: Issue where changing ownership would mark every NetworkVariable dirty. Now, it will only mark any NetworkVariable with owner read permissions as dirty and will send/flush any pending updates to all clients prior to sending the change in ownership message.
  • Fixed: Issue with NetworkVariable collections where transferring ownership to another client would not update the new owner's previous value to the most current value which could cause the last/previous added value to be detected as a change when adding or removing an entry (as long as the entry removed was not the last/previously added value).
  • Fixed: Issue where a client (or server) with no write permissions for a NetworkVariable using a standard .NET collection type could still modify the collection which could cause various issues depending upon the modification and collection type.

Testing and Documentation

Backporting #3081 to NGO v1
Back porting #3081 test updates (where applicable).
adding changelog entries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant