feat: owner-writable `NetworkVariable<T>` #1762

0xFA11 · 2022-03-01T21:55:35Z

By adding enum NetworkVariableWritePermission and Owner/Server options, we're also implementing owner-writable network variables which allows owning clients to change value immediately and replicate that across the network via server.

MTT-2845

Changelog

com.unity.netcode.gameobjects

Added NetworkVariableWritePermission to NetworkVariableBase and implemented Owner writable netvars.

Testing and Documentation

Added NetworkVariablePermissionTests
Documentation will be done by MTT-2846 separately

NoelStephensUnity · 2022-03-16T23:19:35Z

com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableTests.cs

+    [TestFixtureSource(nameof(TestDataSource))]
+    public class NetworkVariablePermissionTests : NetcodeIntegrationTest
+    {
+        public static IEnumerable<TestFixtureData> TestDataSource()


NoelStephensUnity · 2022-03-16T23:21:02Z

com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableTests.cs

+            yield return null;
+        }
+
+        private void AssertOwnerWritableAreEqualOnAll()


jeffreyrainy · 2022-03-17T16:35:31Z

com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs

@@ -595,9 +593,11 @@ private void NetworkVariableUpdate(ulong clientId, int behaviourIndex)
                    var shouldSend = false;
                    for (int k = 0; k < NetworkVariableFields.Count; k++)
                    {
-                        if (NetworkVariableFields[k].ShouldWrite(clientId, IsServer))
+                        var networkVariable = NetworkVariableFields[k];
+                        if (networkVariable.IsDirty() && networkVariable.CanClientRead(targetClientId))


What is the rationale for copying the code from ShouldWrite and editing it here, as opposed to improving/adjusting/modifying ShouldWrite? Just wondering what will make the code more easily maintainable.

because there is no ShouldWrite anymore. there are CanClientRead and CanClientWrite for perm checks, whether or not to write (send) is determined outside, here.

jeffreyrainy · 2022-03-17T16:38:47Z

com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs

@@ -429,8 +425,7 @@ internal void SnapshotDespawn()
        internal void SnapshotDespawn(ulong clientId)
        {
            var command = GetDespawnCommand();
-            command.TargetClientIds = new List<ulong>();
-            command.TargetClientIds.Add(clientId);
+            command.TargetClientIds = new List<ulong> { clientId };


Ok, yeah, those are great changes, but they make the PR harder to process and possibly harder to merge later. We should try to have PRs that address just one thing and side clean-up PRs are fine too :-). < /nitpick> < /jeffshouldgethiscoffee>

jeffreyrainy · 2022-03-17T16:40:17Z

com.unity.netcode.gameobjects/Runtime/Messaging/Messages/NetworkVariableDeltaMessage.cs

@@ -31,9 +31,9 @@ public void Serialize(FastBufferWriter writer)
            writer.WriteValue(NetworkObjectId);
            writer.WriteValue(NetworkBehaviourIndex);

-            for (int k = 0; k < NetworkBehaviour.NetworkVariableFields.Count; k++)
+            for (int i = 0; i < NetworkBehaviour.NetworkVariableFields.Count; i++)


:headscratch:

NoelStephensUnity · 2022-03-17T17:49:05Z

com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkList.cs

-        /// </summary>
-        /// <param name="values">The initial value to use for the NetworkList</param>
-        public NetworkList(IEnumerable<T> values)
+        public NetworkList(IEnumerable<T> values = default,


NoelStephensUnity · 2022-03-17T17:49:37Z

com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariable.cs

-        /// </summary>
-        /// <param name="value">The initial value to use for the NetworkVariable</param>
-        public NetworkVariable(T value)
+        public NetworkVariable(T value = default,


com.unity.netcode.gameobjects/CHANGELOG.md

NoelStephensUnity

Other than adding a PR number to the end of your changelog entry, it looks good to me!

SamuelBellomo · 2022-03-17T18:54:39Z

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -10,9 +10,12 @@ Additional documentation and release notes are available at [Multiplayer Documen

 ### Added

+- Added `NetworkVariableWritePermission` to `NetworkVariableBase` and implemented `Owner` writable netvars. (#1762)


dunno if we want to mention this adds client driven capabilities? Just so users do the association?

SamuelBellomo · 2022-03-17T19:18:14Z

com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableTests.cs

+            testCompServer.OwnerWritable_Position.Value = newValue;
+            yield return NetcodeIntegrationTestHelpers.WaitForTicks(m_ServerNetworkManager, 2);
+
+            AssertOwnerWritableAreEqualOnAll();


if for whatever reason server side Value gets reverted back to it's original value and then client side the Value never changes, this test would still pass :(
with these, I usually prefer to set very specific values and make sure they are the ones expected on arrival.

@SamuelBellomo Ok, I see what you are saying...
@MFatihMAR So maybe you could do this:

private void AssertServerWritableAreEqualOnAll(Vector3 expectedPosition) { var testObjServer = m_ServerNetworkManager.SpawnManager.SpawnedObjects[m_TestObjId]; var testCompServer = testObjServer.GetComponent<NetVarPermTestComp>(); foreach (var clientNetworkManager in m_ClientNetworkManagers) { var testObjClient = clientNetworkManager.SpawnManager.SpawnedObjects[m_TestObjId]; var testCompClient = testObjClient.GetComponent<NetVarPermTestComp>(); Assert.AreEqual(testCompServer.ServerWritable_Position.Value, expectedPosition); Assert.AreEqual(testCompServer.ServerWritable_Position.Value, testCompClient.ServerWritable_Position.Value); Assert.AreEqual(testCompServer.ServerWritable_Position.ReadPerm, testCompClient.ServerWritable_Position.ReadPerm); Assert.AreEqual(testCompServer.ServerWritable_Position.WritePerm, testCompClient.ServerWritable_Position.WritePerm); } }

And update your tests to pass in the expected new value to AssertServerWritableAreEqualOnAll?

technically that's being caught but the fact that it's hard to imply made me add an extra assert to compare .Value against newValue

SamuelBellomo · 2022-03-17T19:21:12Z

com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableTests.cs

+            yield return NetcodeIntegrationTestHelpers.WaitForTicks(m_ClientNetworkManagers[clientManagerIndex], 4);
+
+            AssertOwnerWritableAreEqualOnAll();
+        }


I would also check that a non owner can't write on that netvar, but both owner and non-owner can still read

I would also check that the server can't write to those, in addition to another client that's not the owner.

implemented NetworkVariablePermissionTests.ServerCannotChangeOwnerWritableNetVar() test

SamuelBellomo · 2022-03-17T19:23:03Z

com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableTests.cs

+            yield return NetcodeIntegrationTestHelpers.WaitForTicks(m_ClientNetworkManagers[clientManagerIndex], 4);
+            Assert.AreEqual(testCompServer.ServerWritable_Position.Value, oldValue);
+
+            AssertServerWritableAreEqualOnAll();


same here, if the value did change, even with throwing the exception, this would still pass

com.unity.netcode.gameobjects/Tests/Runtime/Components/NetworkVariableTestComponent.cs

…ableNetVar` test

enum NetworkVariableWritePermission

d174d40

0xFA11 force-pushed the feat/ownernetworkvariable branch from 07aed2e to d174d40 Compare March 12, 2022 00:50

0xFA11 changed the title ~~feat: OwnerNetworkVariable~~ feat: owner-writable NetworkVariable<T> Mar 12, 2022

Unity-Technologies deleted a comment from becksebenius-unity Mar 12, 2022

Unity-Technologies deleted a comment from jeffreyrainy Mar 12, 2022

0xFA11 force-pushed the feat/ownernetworkvariable branch from a3562e4 to d174d40 Compare March 13, 2022 02:48

0xFA11 added 12 commits March 13, 2022 02:48

Merge branch 'develop' into feat/ownernetworkvariable

d874f80

clientId -> targetClientId

cf748af

OwnerOnly -> Owner

de83e62

mvp

e863de6

Merge branch 'develop' into feat/ownernetworkvariable

a17eac1

minor refactor

d797a24

minor fixes

7b3590f

Merge branch 'develop' into feat/ownernetworkvariable

46eb120

syntax fix

b248189

bug fix

ea4f2da

implement NetworkVariablePermissionTests

9eabf22

Merge branch 'develop' into feat/ownernetworkvariable

a5542b9

NoelStephensUnity reviewed Mar 16, 2022

View reviewed changes

Merge branch 'develop' into feat/ownernetworkvariable

770c176

0xFA11 marked this pull request as ready for review March 17, 2022 15:12

0xFA11 added 4 commits March 17, 2022 15:26

delete ctor tests

9726583

update changelog

533dd6a

minor changelog update

b1c2b64

fix NetworkVariableTestComponent

a4fc4bc

0xFA11 requested review from SamuelBellomo and jeffreyrainy March 17, 2022 16:26

jeffreyrainy reviewed Mar 17, 2022

View reviewed changes

jeffreyrainy approved these changes Mar 17, 2022

View reviewed changes

NoelStephensUnity reviewed Mar 17, 2022

View reviewed changes

com.unity.netcode.gameobjects/CHANGELOG.md Outdated Show resolved Hide resolved

NoelStephensUnity approved these changes Mar 17, 2022

View reviewed changes

add PR number to the changelog entry

a646f46

SamuelBellomo reviewed Mar 17, 2022

View reviewed changes

com.unity.netcode.gameobjects/Tests/Runtime/Components/NetworkVariableTestComponent.cs Show resolved Hide resolved

0xFA11 added 3 commits March 17, 2022 19:37

assert on newvalue change

aaffe62

Merge branch 'develop' into feat/ownernetworkvariable

6327e3c

implement `NetworkVariablePermissionTests.ServerCannotChangeOwnerWrit…

07afa2d

…ableNetVar` test

0xFA11 enabled auto-merge (squash) March 17, 2022 21:11

0xFA11 merged commit 6695fbc into develop Mar 17, 2022

0xFA11 deleted the feat/ownernetworkvariable branch March 17, 2022 22:42

0xFA11 mentioned this pull request Mar 18, 2022

feat: client network variables #1522

Closed

3 tasks

JesseOlmer mentioned this pull request Aug 10, 2022

Add client write support to NetworkVariable #1489

Closed

		@@ -10,9 +10,12 @@ Additional documentation and release notes are available at [Multiplayer Documen

		### Added

		- Added `NetworkVariableWritePermission` to `NetworkVariableBase` and implemented `Owner` writable netvars. (#1762)

feat: owner-writable NetworkVariable<T> #1762

feat: owner-writable NetworkVariable<T> #1762

Uh oh!

Conversation

0xFA11 commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog

com.unity.netcode.gameobjects

Testing and Documentation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffreyrainy Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffreyrainy Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NoelStephensUnity left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

feat: owner-writable `NetworkVariable<T>` #1762

feat: owner-writable `NetworkVariable<T>` #1762

0xFA11 commented Mar 1, 2022 •

edited

Loading

jeffreyrainy Mar 17, 2022 •

edited

Loading

jeffreyrainy Mar 17, 2022 •

edited

Loading