feat: tool to enable monobehaviours only on server or client #477

LPLafontaineB · 2022-02-10T15:18:18Z

Description (*)

This PR adds a NetworkBehaviour that can be placed on a NetworkObject to make sure certain MonoBehaviours can only be enabled on active servers or active clients by using specific interfaces that those MonoBehaviours would inherit. This would remove that responsibility from those MonoBehaviours. It would also allow us to make some of them not be NetworkBehaviours anymore since it was mostly used for the OnNetworkSpawn to enable/disable themselves.

As an example, I have included in this draft PR an implementation of this feature with the interactive door.

Some pros of this solution:

Provides a unified solution for something we currently need to do in a lot of different places
Allows us to change some NetworkBehaviours back to just MonoBehaviours
Provides a single point on which to configure which MonoBehaviours are executed on clients and which are executred on servers

Cons:

Adds weight to the actual prefabs and in-scene GameObjects
Needs us to make sure that all MonoBehaviours that should only be enabled on a client/server implement the correct interface and that this NetworkBehaviour is added to every prefab and in-scene GameObject that uses it

Related Pull Requests

Issue Number(s) (*)

Fixes issue(s): When implemented, should fix MTT-1683 & MTT-2433

Manual testing scenarios

...
...

Questions or comments

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds are green)

Inheriting from this abstract class insteead of a NetworkBehaviour allows a script to be disabled on Awake and only enabled in a client or a server specifically

This reverts commit 6080134.

…e/disable

LPLafontaineB · 2022-02-10T16:13:41Z

Assets/BossRoom/Scripts/Shared/OnSpawnBehaviourEnabler.cs

+using Unity.Netcode;
+using UnityEngine;
+
+namespace Unity.Multiplayer.Samples.Utilities


This file will be moved to the Utilities package when the PR will leave the draft status. I kept it here to make testing and iterating easier since ParrelSync doesn't sync the packages folder automatically

LPLafontaineB · 2022-02-25T16:35:15Z

The issue this PR fixes could also be the cause of MTT-2475, but hard to verify since we don't have consistent reproduction steps for that bug yet

SamuelBellomo · 2022-02-25T19:52:34Z

Assets/BossRoom/Scripts/Shared/OnSpawnBehaviourEnabler.cs

+
+namespace Unity.Multiplayer.Samples.Utilities
+{
+    public interface IClientOnlyMonoBehaviour


So if those interfaces are tightly coupled to monobehaviour and netcode, any reason we wouldn't just have an abstract class that inherits from NetworkBehaviour and has SetEnabled as abstract method?

An earlier version of this used inheritance, with a single class inheriting NetworkBehavior with a protected field telling if it was meant for a server or a client. The disabling/enabling was handled by that abstract class in its Awake, OnSpawn and OnDespwn events.
The advantage was that we did not need a second script to enable/disable it, but it came with more restrictions that I wasn't really happy with. First, we would need to make sure to call the base methods inside those three methods if we wanted to override it in daughter classes. Also, it would mean that if a specific class needs that behaviour, all its parents would have to have it too, which might not make sense. i.e. class A is a client-only netbehaviour inheriting from class B that doesn't need to know if it is on a client or on a server. In this case, if we want to restrict A to only clients, we would have to have B inherit this abstract class, even though B doesn't require it.

I see. Yeah makes sense. I wonder if there could be a way to dynamically add the enabler without requiring us to remember to add it. It's too bad "RequireComponent" doesn't work on interfaces.
Or we could have some editor script magic with some reflection...?

@LukeStampfli @pdeschain @fernando-cortez what do you guys think?

We could add "RequireComponent" to the classes implementing these interfaces themselves I suppose, but it's still not as plug-and-play as I hoped it could be

Thinking back on it, would adding this recommendation as remarks in the interface descriptions be sufficient?

I think using an interface here is better than using inheritance. With the updated xml docs on the interfaces I think it is easy enough to understand how to use this.

SamuelBellomo · 2022-02-25T19:56:28Z

Assets/BossRoom/Scripts/Shared/OnSpawnBehaviourEnabler.cs

+        {
+            if (IsClient)
+            {
+                foreach (var behaviour in gameObject.GetComponents<IClientOnlyMonoBehaviour>())


OnNetworkSpawn would be called with network pooling correct? This has the potential to be a very costly spawn, GetComponent isn't performant

especially if there's multiple multiple spawns with pooling

Oh you're right, that would be super costly. Maybe I should cache all of this in its Awake method? Another solution could be to expose a list to the editor, but that would require us to manually enter all netbehaviours, and that would be even more work than what this tool is trying to simplify

yeah caching would be good

…client

SamuelBellomo · 2022-03-08T19:17:31Z

on hold until we have true solution

SamuelBellomo · 2022-05-12T19:41:51Z

Adding this for all PRs that have been opened since this new flow has been added. Please add your changelog here project changelog file and/or package changelog file

LPLafontaineB added 3 commits February 10, 2022 09:47

Added a solution based on inheritance

6080134

Inheriting from this abstract class insteead of a NetworkBehaviour allows a script to be disabled on Awake and only enabled in a client or a server specifically

Added a script that handles the disabling/enabling of others

cd026e6

Revert "Added a solution based on inheritance"

e673ac9

This reverts commit 6080134.

LPLafontaineB requested review from LukeStampfli, SamuelBellomo and fernando-cortez February 10, 2022 15:18

LPLafontaineB added 4 commits February 10, 2022 10:30

Adding namespace

864b789

fixed typo in class name

fb6a563

Replaced fields with interfaces to determine which behaviour to enabl…

ada0a1d

…e/disable

Add implementation for the interactive door

e1e5528

LPLafontaineB commented Feb 10, 2022

View reviewed changes

LPLafontaineB mentioned this pull request Feb 10, 2022

fix: client writing to netvar error #468

Merged

4 tasks

SamuelBellomo added the 5-Design Review Draft to review design ideas before committing to future work label Feb 17, 2022

SamuelBellomo reviewed Feb 25, 2022

View reviewed changes

SamuelBellomo added the GDC-cherrypick label Mar 1, 2022

LPLafontaineB added 3 commits March 4, 2022 16:09

Merge branch 'develop' into feature/tool-to-enable-only-on-server-or-…

21410ee

…client

Added caching of components in Awake

7e88dda

Added comments to recommend the use of RequireComponent attribute

cb365f2

LPLafontaineB requested a review from SamuelBellomo March 7, 2022 16:40

Adding RequireComponent to ServerSwitchedDoor

2e55d32

LukeStampfli approved these changes Mar 8, 2022

View reviewed changes

SamuelBellomo added 4-On Hold PR can't proceed because it's blocked or is otherwise waiting on something. and removed GDC-cherrypick labels Mar 8, 2022

LPLafontaineB closed this Aug 24, 2022

LPLafontaineB deleted the feature/tool-to-enable-only-on-server-or-client branch October 3, 2022 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: tool to enable monobehaviours only on server or client #477

feat: tool to enable monobehaviours only on server or client #477

Uh oh!

LPLafontaineB commented Feb 10, 2022 •

edited

Loading

Uh oh!

LPLafontaineB Feb 10, 2022

Uh oh!

LPLafontaineB commented Feb 25, 2022

Uh oh!

SamuelBellomo Feb 25, 2022

Uh oh!

LPLafontaineB Feb 28, 2022

Uh oh!

SamuelBellomo Mar 1, 2022

Uh oh!

SamuelBellomo Mar 1, 2022

Uh oh!

LPLafontaineB Mar 4, 2022

Uh oh!

LPLafontaineB Mar 7, 2022

Uh oh!

LukeStampfli Mar 8, 2022

Uh oh!

SamuelBellomo Feb 25, 2022

Uh oh!

SamuelBellomo Feb 25, 2022

Uh oh!

LPLafontaineB Feb 28, 2022 •

edited

Loading

Uh oh!

SamuelBellomo Mar 1, 2022

Uh oh!

SamuelBellomo commented Mar 8, 2022

Uh oh!

SamuelBellomo commented May 12, 2022

Uh oh!

Uh oh!

feat: tool to enable monobehaviours only on server or client #477

feat: tool to enable monobehaviours only on server or client #477

Uh oh!

Conversation

LPLafontaineB commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description (*)

Related Pull Requests

Issue Number(s) (*)

Manual testing scenarios

Questions or comments

Contribution checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LPLafontaineB commented Feb 25, 2022

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!

LPLafontaineB Feb 28, 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!

SamuelBellomo commented Mar 8, 2022

Uh oh!

SamuelBellomo commented May 12, 2022

Uh oh!

Uh oh!

LPLafontaineB commented Feb 10, 2022 •

edited

Loading

LPLafontaineB Feb 28, 2022 •

edited

Loading