feat: NetworkObject Parenting #855

0xFA11 · 2021-05-24T22:06:49Z

This PR implements RFC#17: NetworkObject Parenting (tracked by Issue #876)

PR also includes minor refactors and cleanups around the codebase.

… IsSpawned)

com.unity.multiplayer.mlapi/Runtime/Core/NetworkObject.cs

SamuelBellomo · 2021-05-31T18:07:53Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkObject.cs

+            if (!NetworkManager.IsServer)
+            {
+                transform.parent = m_CachedParent;
+                Debug.LogException(new NotServerException($"Only the server can reparent {nameof(NetworkObject)}s"));


how would this work with a client authoritative NetworkTransform?

NetworkTransform is a NetworkBehaviour and a component — similarly, NetworkBehaviour conceptually a component to NetworkObject. So, in this context, we're talking about reparenting a NetworkObject, not a NetworkTransform or anything.
NetworkTransform could still replicate its state (pos,rot,sca etc.) however it wants. If it requires a parental change, it has to ask server, more specifically NetworkObject to change its parent.

mmm so the NetworkTransform has to wait a full RTT for the server to confirm the parent change?
if my character is jumping on the back of a moving predicted car, I wouldn't be able to use parenting client side until RTT ms? I'd have a full RTT where my character would jitter around on the car until parenting is activated? that's not good.

you could have 2 NetworkObjects and enable one, disable the other and keep going without any issues and potentially even without any parenting during the transition.

So if I have 100 players and 100 cars, each car would need 100 disabled players as children on them in case the player jumps on them? And I'd need to copy my player state to each of these cars?
That doesn't seem very users first :(

I can say, it's not in our scope or in any of our acceptance criteria etc.

you're explaining a specific scenario we may or may not support (which could be a good user-need to start with)

there is no authority model in MLAPI other than Ownership, except the ad-hoc NetworkTransform internal one — and I'm hesitant to expand the scope of this feature/delivery.

MLAPI has supported client authority for a while though. It's not formalized like we're aiming to do it, but it's still supported. Dropping that support is kind of a big deal.
I'm guessing since you're arguing against it there's a good technical reason for not supporting client authority in that reparenting case?
To be noted, I'm not talking about shared authority, more about owner authority, where only an owner could change the parent, but that change could come either from the client or from the server. I understand with shared authority it can become a mess very fast, but would it be that complex to allow clients changing parenting too?

MLAPI has supported client authority for a while though.

where specifically?

Dropping that support is kind of a big deal.

checkout the ownership code, look into NetworkObject::ChangeOwnership(ulong newOwnerClientId) API.
following your definition, MLAPI already dropped client authority long before then.

I'm guessing since you're arguing against it there's a good technical reason for not supporting client authority in that reparenting case?

I'm not arguing against anything here. I'm double-checking your assumptions and understandings.

To be noted, I'm not talking about shared authority, more about owner authority, where only an owner could change the parent, but that change could come either from the client or from the server. I understand with shared authority it can become a mess very fast, but would it be that complex to allow clients changing parenting too?

ownership model (IsOwner, OwnerClientId, ChangeOwnership() etc.) does not play a role here. the code user writes on the server-side can decide to change with regards to the object's owner or regardless of it, we do not assume or try to guess. however, ChangeOwnership() API works only on the server-side because the server is the ultimate source of truth. we are following a very similar concept here: which NetworkObject is parent/child of which NetworkObject is determined, controlled and stored by the server — depending on what user writes, it may or may not be affected by ownership or any other arbitrary conditions but the execution must happen on the server-side as it is for ownership.

So since AutoObjectParentSync is a per-NetworkObject property, could one enable the kind of client-authoritative parenting @SamuelBellomo is describing by controlling this flag on the desired objects?

you can opt-out from AutoObjectParentSync and do whatever you want (the goal of this flag was to give experienced developers full-control and get out their way, as outlined in the RFC), yes. however, client-driven/authoritative object parenting sounds like a completely different feature to me (and I'd even argue if that's needed given that we're a client-server model).

SamuelBellomo · 2021-05-31T18:11:15Z

About adding samples for this feature, we already have assets for a character moving a barrel around, we'd just need to add this feature in boss room.

SamuelBellomo · 2021-05-31T18:18:23Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkObject.cs

+                {
+                    if (NetworkLog.CurrentLogLevel <= LogLevel.Error)
+                    {
+                        NetworkLog.LogError($"Invalid parenting, {nameof(NetworkObject)} moved under a non-{nameof(NetworkObject)} parent, will fix and move to the scene root");


Why not cancel the reparenting and use the cached parent? It'd be consistent with the other error handling we have above

oh, I think you're right! :)

SamuelBellomo · 2021-05-31T18:19:25Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkObject.cs

+                }
+                else
+                {
+                    if (NetworkLog.CurrentLogLevel <= LogLevel.Error)


it's not really part of your review, but this if should really be part of NetworkLog.LogError

to me, I'd throw away the NetworkLog entirely LOL

meh I do like the developer level logs. I agree eventually we should have network traces to replace them, but it's a good thing we can enable/disable them.

we already have developer-level logs, it's called Debug.Log suite and if you were to look at NetworkLog closely, it's just a thin layer on top of Debug.Log which serves very little value IMHO.

SamuelBellomo · 2021-05-31T18:27:38Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkObject.cs

+                using (var writer = PooledNetworkWriter.Get(buffer))
+                {
+                    writer.WriteUInt64Packed(NetworkObjectId);
+                    WriteNetworkParenting(writer, m_IsReparented, m_LatestParent);


If we reparent and move (NetworkTransform) in the same frame, do we have a guarantee the reparent and move will happen at the same time and in the right order client side?

we (still) have no guarantees or message delivery orders. well, we kind of have but it is very implicit and opaque. if you were to use the same reliable ordered fragmented channel (or whatever it's called), you could guarantee that they'd be in order — but, that's very subtle and not reliable IMHO.
so, the answer is yes but actually no. also NetworkObject parenting doesn't necessarily look into NetworkTransform compatibility since it's lower-level than NetworkTransform. if things break and/or don't look good, we might even consider reworking NetworkTransform after NetworkObject changes based on new rules and nuances.

there's plans to have grouping for netvars, enabling a guarantee these netvars will be sent together. @jeffreyrainy for visibility.
using netvars would help in this case.

I'm not a part of NetVars convos nor snapshotting convos. I'm living in my own silo here and delivering what I'm supposed to deliver.

So assuming we have that grouping, that'd make it great to use with this no?

again, I have no idea what you folks are talking about or planning. however, I'd still be opinionated about avoiding coupling as much as possible. I'm also hesitant to suggest any architectural or technical changes either in netvars/snapshots space or MLAPI in general. if you have anything fancy in your mind, I'm all ears.

SamuelBellomo · 2021-05-31T18:30:01Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkObject.cs

+                    var targetClientId = NetworkManager.ConnectedClientsList[i].ClientId;
+                    if (Observers.Contains(targetClientId))
+                    {
+                        NetworkManager.MessageSender.Send(targetClientId, NetworkConstants.PARENT_SYNC, NetworkChannel.Internal, buffer);


out of curiosity, why aren't we using netvars to track this?
Custom messages makes this harder to track in any eventual AoI / Prediction / Server side rewind, no?

I don't think NetVar is the right tool for this job.

Any reason why not?

Custom messages makes this harder to track in any eventual AoI / Prediction / Server side rewind, no?

what you see inline above is not a "custom message", custom messages are something completely different.

Any reason why not?

I'd argue why so?

NetVar is being checked every-frame, does some other things and will do some other things (that I don't fully understand yet, maybe I don't have to?) which is not needed here because we precisely control when parent changes and know exactly when it happens.
NetworkObject parenting could live without NetVar coupling and I believe de-coupling is a good thing.

I need to look more closely but I don't want to forget this thought...so with this change, is the NetworkTransform always sending the relative-to-parent or global position as before?

Regardless, my intuition is suggesting that I suggest this - what if we atomically sent either just a position change (if the parent hasn't changed, as today) or parent change plus position/rot change. That way you never have the weird case where the parent changed with a mis-matched transform position causing jumping, etc. This parenting only applies to transform (yes?) so it doesn't seem like a bolt on.

things could've done better for sure but things are being done this way due to our current MLAPI architecture (NetworkObject-NetworkBehaviour relationship, their relation to scene hierarchy and NetworkTransform doing transform-related stuff. I think both of you know where I stand in terms of NO, NB & NT — NO & NB should not even form any transform-based hierarchy at all, if any at all).

apart from that, we can still do things in NetworkTransform realm. we do have a OnNetworkObjectParentChanged() callback when NetworkObject reparenting happens (on both server and client sides). that could be used to switch between local vs world relative positioning or any other custom optimization at NetworkTransform-level.

I added a placeholder method implementation into NetworkTransform.cs where I put a comment saying:

// TODO: handle parent NetworkObject change and potentially optimize pos/rot/scale replication?

that'd hopefully guide us later in the future (if and) when we decide to discover optimization possibilities on NetworkTransform.

com.unity.multiplayer.mlapi/Prototyping/NetworkTransform.cs

com.unity.multiplayer.mlapi/Runtime/Core/NetworkManager.cs

com.unity.multiplayer.mlapi/Runtime/Core/NetworkObject.cs

mattwalsh-unity · 2021-06-07T03:42:59Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkObject.cs

+            return TrySetParent(parent.GetComponent<NetworkObject>(), worldPositionStays);
+        }
+
+        public bool TrySetParent(NetworkObject parent, bool worldPositionStays = true)


So, we want to silently fail in these cases or what about throwing an exception?

I don't think returning a bool back from a method called TryXXX is silencing fail cases. it is pretty much like .NET Dictionary.TryAdd() API.

mattwalsh-unity · 2021-06-07T04:06:03Z

I had some thoughts as I did my first read through...

how would you think about handling things like prioritization & AOI? For example, if object A is out of range of object B when it becomes a child of C, then when B comes into range of A (and C) how would it get updated on the B/C relationship?
If we did a prioritization filter later, how would we make sure that the transform pos/rot line up with parent changes? I guess there's another question which is, how would we marshal the sending in a priority system anyway
ownership - is there any restriction on whether 2 objects owned by 2 different players are parented? If I own A and you own B, can B be a child of A? And if so, and A despawns, what happens to B? Does it just pop to what my parent was?
What happens if you have a mix of NetworkObjects in a parented relationship when some have AutoObjectParentSync on and some don't?

mattwalsh-unity

I don't have specific changes yet, this is my signal to help gain more understanding before the commit

0xFA11 · 2021-06-07T16:18:13Z

@mattwalsh-unity

how would you think about handling things like prioritization & AOI? For example, if object A is out of range of object B when it becomes a child of C, then when B comes into range of A (and C) how would it get updated on the B/C relationship?

network spawn/despawn happens on NetworkShow & NetworkHide — we might follow the same logic here. obv, I think we need a bigger picture and plan but for the time being, we could rely on this behaviour (NetworkObjects are being re-serialized & replicated on network spawn due to NetworkShow()).

If we did a prioritization filter later, how would we make sure that the transform pos/rot line up with parent changes? I guess there's another question which is, how would we marshal the sending in a priority system anyway

this is a good question that I'd like to answer differently: I think this implementation should not even exist at this level (NetworkObject-NetworkBehaviour level) but here we are. until we land AOI, filtering, prioritization, NetworkObject-Behaviour-Transform- changes, I'd see this feature exist as a temporary solution (to deliver on the user-need in the roadmap).

ownership - is there any restriction on whether 2 objects owned by 2 different players are parented? If I own A and you own B, can B be a child of A? And if so, and A despawns, what happens to B? Does it just pop to what my parent was?

owner can't despawn an object, it can just call ServerRpcs on it (and potentially apply ownership rules on other features in the future). apart from that, parenting doesn't care. A might be owned by Client X and child object B might be owned by Client Y then everything would still work just fine. no limitations or restrictions between ownership and parenting here.

What happens if you have a mix of NetworkObjects in a parented relationship when some have AutoObjectParentSync on and some don't?

AutoObjectParentSync effects yourself only. you might be moved under another NetworkObject with that flag disabled and that wouldn't be synced even though your parent had that flag enabled. the other way around, let's say you have the flag enabled but your parent doesn't, you'd replicate your parenting change just fine. AutoObjectParentSync is applied to yourself when you make a parental change in the transform hierarchy — it doesn't require both parent and child to have the flag enabled, just the subject matter NetworkObject is enough.

NoelStephensUnity

This looks real nice Fatih.
Like the updates to the comments...the new parent changed notification callback...and your test is insane!

com.unity.multiplayer.mlapi/Runtime/Core/NetworkObject.cs

0xFA11 · 2021-06-08T19:48:02Z

com.unity.multiplayer.mlapi/Runtime/Spawning/NetworkSpawnManager.cs

+            // Move child NetworkObjects to the root when parent NetworkObject is destroyed
+            foreach (var networkObject in SpawnedObjectsList)
+            {
+                var (isReparented, latestParent) = networkObject.GetNetworkParenting();
+                if (isReparented && latestParent == networkObjectId)
+                {
+                    networkObject.gameObject.transform.parent = null;
+
+                    if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
+                    {
+                        NetworkLog.LogWarning($"{nameof(NetworkObject)} #{networkObject.NetworkObjectId} moved to the root because its parent {nameof(NetworkObject)} #{networkObjectId} is destroyed");
+                    }
+                }
+            }


@LukeStampfli

…o be fixed by OrphanChildren mechanism

com.unity.multiplayer.mlapi/Tests/Runtime/ObjectParenting/NetworkObjectParentingTests.cs

0xFA11 · 2021-06-09T14:17:34Z

com.unity.multiplayer.mlapi/Prototyping/NetworkTransform.cs

@@ -221,6 +213,11 @@ void SetupVar<T>(NetworkVariable<T> v, T initialValue, ref T oldVal)
            }
        }

+        public override void OnNetworkObjectParentChanged(NetworkObject parentNetworkObject)
+        {
+            // TODO: handle parent NetworkObject change and potentially optimize pos/rot/scale replication?


@SamuelBellomo

* develop: (21 commits) feat: NetworkObject Parenting (#855) refactor: move RpcMethodId serialization from ILPP to Core (#910) fix: NetworkPrefabs container's elements invalidated in the NetworkManager after relaunching Unity Project (#905) feat!: OnNetworkSpawn / OnNetworkDespawn (#865) feat: Add missing XMLdoc comment (#897) refactor: upgrade ILPP backend, drop 2019.4 support, rename types/fields (#895) fix: do not access/render runtime info if not playing in the editor (#898) feat: Add name property for network variables (#891) chore: delete PhilTestResults.xml (#894) feat: MultiInstanceHelpers to use fixed FrameRate by default (#893) test: General MultiInstanceHelper improvements (#885) refactor: isKinematic set to true for rigid bodies of non-authorized instances (#886) docs: adding more info to help debug on network transform error message (#892) feat: Add RPC Name Lookup Table Provided by NetworkBehaviourILPP (#875) fix: remove OnClientConnectedCallback registration from StatsDisplay (#882) feat: Add profiling decorator pattern (#878) refactor: Removing dead code for NETWORK_VARIABLE_UPDATE (#880) fix: update package version to 0.2.0 because of unity minversion change (#881) docs: Fix typo in changelog version title docs: Hotfix Changelog for 0.1.1 and manual update (#873) ... # Conflicts: # com.unity.multiplayer.mlapi/Runtime/Core/NetworkBehaviour.cs # com.unity.multiplayer.mlapi/Runtime/Core/NetworkManager.cs # com.unity.multiplayer.mlapi/Tests/Runtime/BaseMultiInstanceTest.cs # com.unity.multiplayer.mlapi/Tests/Runtime/BaseMultiInstanceTest.cs.meta

* develop: (67 commits) feat: NetworkObject Parenting (#855) refactor: move RpcMethodId serialization from ILPP to Core (#910) fix: NetworkPrefabs container's elements invalidated in the NetworkManager after relaunching Unity Project (#905) feat!: OnNetworkSpawn / OnNetworkDespawn (#865) feat: Add missing XMLdoc comment (#897) refactor: upgrade ILPP backend, drop 2019.4 support, rename types/fields (#895) fix: do not access/render runtime info if not playing in the editor (#898) feat: Add name property for network variables (#891) chore: delete PhilTestResults.xml (#894) feat: MultiInstanceHelpers to use fixed FrameRate by default (#893) test: General MultiInstanceHelper improvements (#885) refactor: isKinematic set to true for rigid bodies of non-authorized instances (#886) docs: adding more info to help debug on network transform error message (#892) feat: Add RPC Name Lookup Table Provided by NetworkBehaviourILPP (#875) fix: remove OnClientConnectedCallback registration from StatsDisplay (#882) feat: Add profiling decorator pattern (#878) refactor: Removing dead code for NETWORK_VARIABLE_UPDATE (#880) fix: update package version to 0.2.0 because of unity minversion change (#881) docs: Fix typo in changelog version title docs: Hotfix Changelog for 0.1.1 and manual update (#873) ... # Conflicts: # com.unity.multiplayer.mlapi/Tests/Runtime/com.unity.multiplayer.mlapi.runtimetests.asmdef # testproject/ProjectSettings/EditorBuildSettings.asset

0xFA11 added 11 commits May 23, 2021 17:30

networkobject parent tracking

e73a720

NetworkConstants.PARENT_SYNC = 16

ea6b6e7

minor nullptr fix

68c1812

minor cleanup

2df134d

sync networkobject reparenting props

a175ef3

parent sync with orphan children

3c03ddb

sync reparenting at runtime

dac2e0f

working poc

e161d73

more networkobject parenting restrictions (IsListening && IsServer &&…

e5eaf2a

… IsSpawned)

minor fix

33fb6ac

cleanup

342495b

0xFA11 requested review from jeffreyrainy, mattwalsh-unity, SamuelBellomo and NoelStephensUnity May 24, 2021 22:06

Merge branch 'develop' into feat/reparent

8151633

NoelStephensUnity reviewed May 24, 2021

View reviewed changes

com.unity.multiplayer.mlapi/Runtime/Core/NetworkObject.cs Show resolved Hide resolved

NoelStephensUnity reviewed May 24, 2021

View reviewed changes

com.unity.multiplayer.mlapi/Runtime/Core/NetworkObject.cs Show resolved Hide resolved

0xFA11 added 3 commits May 25, 2021 05:44

minor update: exception message

1766b58

nullptr check

86a1532

reset networkobject.orphanchildren set on networkmanager.initialize

d7a4a3c

CertifiedRice approved these changes May 27, 2021

View reviewed changes

NetworkBehaviour.OnNetworkObjectParentChanged

3588afe

SamuelBellomo reviewed May 31, 2021

View reviewed changes

Merge branch 'develop' into feat/reparent

91a5358

mattwalsh-unity reviewed Jun 7, 2021

View reviewed changes

com.unity.multiplayer.mlapi/Prototyping/NetworkTransform.cs Show resolved Hide resolved

mattwalsh-unity reviewed Jun 7, 2021

View reviewed changes

com.unity.multiplayer.mlapi/Runtime/Core/NetworkManager.cs Show resolved Hide resolved

mattwalsh-unity reviewed Jun 7, 2021

View reviewed changes

com.unity.multiplayer.mlapi/Runtime/Core/NetworkObject.cs Show resolved Hide resolved

mattwalsh-unity reviewed Jun 7, 2021

View reviewed changes

com.unity.multiplayer.mlapi/Runtime/Core/NetworkObject.cs Outdated Show resolved Hide resolved

mattwalsh-unity reviewed Jun 7, 2021

View reviewed changes

mattwalsh-unity suggested changes Jun 7, 2021

View reviewed changes

0xFA11 added 2 commits June 7, 2021 23:56

add placeholder OnNetworkObjectParentChanged callback handler

33bf226

more comments and warning log

235aea6

NoelStephensUnity approved these changes Jun 7, 2021

View reviewed changes

hey hey Yamato

6df1324

0xFA11 commented Jun 8, 2021

View reviewed changes

com.unity.multiplayer.mlapi/Runtime/Core/NetworkObject.cs Show resolved Hide resolved

0xFA11 added 2 commits June 8, 2021 20:45

move child NetworkObject to the root on parent NetworkObject destroy

50d1cf3

Merge branch 'develop' into feat/reparent

309398f

0xFA11 commented Jun 8, 2021

View reviewed changes

downgrade PARENT_SYNC log to Developer from Warning as we expect it t…

0ea6dec

…o be fixed by OrphanChildren mechanism

0xFA11 commented Jun 9, 2021

View reviewed changes

com.unity.multiplayer.mlapi/Tests/Runtime/ObjectParenting/NetworkObjectParentingTests.cs Outdated Show resolved Hide resolved

0xFA11 commented Jun 9, 2021

View reviewed changes

0xFA11 added 4 commits June 9, 2021 16:40

Merge branch 'develop' into feat/reparent

4c6534e

remove Application.targetFrameRate trick

7217098

Merge branch 'develop' into feat/reparent

f2876d2

Merge branch 'develop' into feat/reparent

8619915

mattwalsh-unity approved these changes Jun 18, 2021

View reviewed changes

merge branch 'develop' into 'feat/reparent'

467fe99

0xFA11 enabled auto-merge (squash) June 18, 2021 19:40

0xFA11 merged commit 1a7c145 into develop Jun 18, 2021

0xFA11 deleted the feat/reparent branch June 18, 2021 19:45

feat: NetworkObject Parenting #855

feat: NetworkObject Parenting #855

Uh oh!

Conversation

0xFA11 commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

0xFA11 Jun 7, 2021 • 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!

SamuelBellomo commented May 31, 2021

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!

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!

0xFA11 May 31, 2021 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattwalsh-unity commented Jun 7, 2021

0xFA11 commented May 24, 2021 •

edited

Loading

0xFA11 Jun 7, 2021 •

edited

Loading

0xFA11 May 31, 2021 •

edited

Loading