-
Notifications
You must be signed in to change notification settings - Fork 450
feat!: remove client network permissions [MTT-1019] #1051
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
c3ad039
to
aeb320c
Compare
com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariablePermission.cs
Show resolved
Hide resolved
@@ -524,8 +524,7 @@ private void NetworkVariableUpdate(ulong clientId, int behaviourIndex) | |||
|
|||
// if I'm dirty AND a client, write (server always has all permissions) |
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.
Is this comment still valid?
@@ -628,7 +627,7 @@ internal static void HandleNetworkVariableDeltas(List<INetworkVariable> networkV | |||
} | |||
} | |||
|
|||
if (networkManager.IsServer && !networkVariableList[i].CanClientWrite(clientId)) | |||
if (networkManager.IsServer)// ?? && !networkVariableList[i].CanClientWrite(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.
Is this even needed anymore? because essentially its checking to see if the client can actually write to this value and if not log it... I am not sure this would be possible anymore correct?
So one thing that I think is overlooked with the way things are now is that because of the changed of Collections to using unmanaged and the underlying container hasn't changed, and the access APIs haven't changed are we okay with the large amount of copying that will now happen? like before NetworkList[0] could be a copy but now its always a copy, NetworkList.Contains is always going to be a copy. Not that its an issue but I think its something to callout that the semantics of the collections have changed some. For example if you previously put strings or even bools in a NetworkSet that is not supported now since neither is a blittable type. So I think it would be worth wild making sure that we add to the changelog what the implications of this change is with regards to collections and how they work. |
e405abd
to
a304dcc
Compare
Ok, though my journey of cleanup / iteration my other PR has now become the real one. #1074 |
No description provided.