Skip to content

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

Closed
wants to merge 3 commits into from

Conversation

mattwalsh-unity
Copy link
Contributor

No description provided.

@mattwalsh-unity mattwalsh-unity requested a review from 0xFA11 August 13, 2021 17:19
@mattwalsh-unity mattwalsh-unity force-pushed the feature/container_cleanup branch 2 times, most recently from c3ad039 to aeb320c Compare August 13, 2021 22:23
@mattwalsh-unity mattwalsh-unity changed the title Feature/container cleanup feat!: remove client network permissions [MTT-1019] Aug 13, 2021
@@ -524,8 +524,7 @@ private void NetworkVariableUpdate(ulong clientId, int behaviourIndex)

// if I'm dirty AND a client, write (server always has all permissions)
Copy link
Contributor

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))
Copy link
Contributor

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?

@andrews-unity
Copy link
Contributor

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.

@mattwalsh-unity mattwalsh-unity force-pushed the feature/container_cleanup branch from e405abd to a304dcc Compare August 19, 2021 20:52
@mattwalsh-unity
Copy link
Contributor Author

Ok, though my journey of cleanup / iteration my other PR has now become the real one. #1074

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.

4 participants