Skip to content

feat!: remove client network permissions [MTT-1019]#1051

Closed
mattwalsh-unity wants to merge 3 commits intodevelopfrom
feature/container_cleanup
Closed

feat!: remove client network permissions [MTT-1019]#1051
mattwalsh-unity wants to merge 3 commits intodevelopfrom
feature/container_cleanup

Conversation

@mattwalsh-unity
Copy link
Copy Markdown
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
Copy Markdown
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?

}

if (networkManager.IsServer && !networkVariableList[i].CanClientWrite(clientId))
if (networkManager.IsServer)// ?? && !networkVariableList[i].CanClientWrite(clientId))
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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