feat: Adding support for netsim to be able to work with Tools adapter #2184

Rosme · 2022-09-08T20:18:11Z

This adds an internal callback to tell the tools adapter that the transport is initialized and ready so we can act on it and be aware of it.

Testing and Documentation

No tests have been added.

com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs

ian-m-unity · 2022-09-08T20:24:37Z

com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs

@@ -346,6 +346,10 @@ private struct PacketLossCache
            public float PacketLoss;
        };

+        internal static event Action<UnityTransport> TransportInitialized;
+        internal static event Action<UnityTransport> TransportDisposed;


Should a reference to the Transport be included in the disposed message? Is it still valid at that point?

Yes, since we are using the Instance ID to track the adapter.

Can you expand on what the value is of tracking dispose instead of shutdown? Does this need to be an event or can you use a weak reference?

To expand on Ian's concern and my other comment about internals vs public, at the point you have this being invoked the object is essentially "off limits" to other objects as far as best practices go - regardless of how this may be used right now this feels dangerous and dependent on tribal knowledge about the control flow. If all you need is the ID, why not pass that as the event parameter instead of the entire (mostly disposed) object?

There's more to it. We are using the whole UnityTransport to access the Network Driver. Arguably, we could provide the Instance ID and the Network Driver only as arguments. I am not against this change. As for the Disposed, we definitely only need the Instance ID and I can change it in the same manner if you'd prefer that.

com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs

+ adding back missing parameters

com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs

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

com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs

JesseOlmer · 2022-09-13T15:14:20Z

com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs

@@ -346,6 +346,10 @@ private struct PacketLossCache
            public float PacketLoss;
        };

+        internal static event Action<UnityTransport> TransportInitialized;
+        internal static event Action<UnityTransport> TransportDisposed;


Can you expand on what the value is of tracking dispose instead of shutdown? Does this need to be an event or can you use a weak reference?

To expand on Ian's concern and my other comment about internals vs public, at the point you have this being invoked the object is essentially "off limits" to other objects as far as best practices go - regardless of how this may be used right now this feels dangerous and dependent on tribal knowledge about the control flow. If all you need is the ID, why not pass that as the event parameter instead of the entire (mostly disposed) object?

JesseOlmer · 2022-09-13T15:17:18Z

com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs


-#if UNITY_EDITOR || DEVELOPMENT_BUILD
+#if UNITY_EDITOR || DEVELOPMENT_BUILD || UNITY_MP_TOOLS_NETSIM_IMPLEMENTATION_ENABLED


Is UNITY_EDITOR || DEVELOPMENT_BUILD here because the NETSIM define is 2.0 only? Can't we simplify these cases in the first define?

We need to maintain the existing Debug Simulator as well if the Tools are not present. But since the Network Simulator can also be included in Release builds if the user wished so, we need all three.

com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs

Co-authored-by: Jesse Olmer <jesseo@unity3d.com>

much stuff around

Rosme · 2022-09-14T18:59:00Z

@JesseOlmer @becksebenius-unity Per our alignment, I've updated the PR to remove Debug Simulator if we are using UTP 2. I split the stuff to avoid a mixtures of weird ifdef all over the place.

This also updating the callback for the adapter to avoid passing the whole Unity Transport.

simon-lemay-unity · 2022-09-14T19:03:33Z

Per our alignment, I've updated the PR to remove Debug Simulator if we are using UTP 2. I split the stuff to avoid a mixtures of weird ifdef all over the place.

I understand there's already alignment on this at a higher level than me, but we're okay introducing a breaking API change if someone happens to have UTP 2.0 installed?

Rosme · 2022-09-14T19:07:21Z

Per our alignment, I've updated the PR to remove Debug Simulator if we are using UTP 2. I split the stuff to avoid a mixtures of weird ifdef all over the place.

I understand there's already alignment on this at a higher level than me, but we're okay introducing a breaking API change if someone happens to have UTP 2.0 installed?

As we see it, UTP 2 is already considered a breaking change. Technically, you need to get UTP2 to have this breaking change. If you just get the latest of NGO, it's not a breaking change. If you are willing to go to UTP2, you are accepting that change.

simon-lemay-unity · 2022-09-14T19:14:07Z

As we see it, UTP 2 is already considered a breaking change. Technically, you need to get UTP2 to have this breaking change. If you just get the latest of NGO, it's not a breaking change. If you are willing to go to UTP2, you are accepting that change.

I'm not sure I understand what you mean by UTP 2.0 being a breaking change. With the recent changes that landed, someone installing UTP 2.0 will get something that still works without having to modify their code. All breaking API changes introduced by UTP 2.0 are handled by NGO through version defines. But with the proposed changes here, someone could install UTP 2.0 and get a non-compiling project without any indication as to what happened.

I understand (and support) the rationale of not wanting to support the existing debug simulator if UTP 2.0 is installed. That's fine. Users with UTP 2.0 should install the tools package to get the new and improved network simulator. But straight out removing the API gives them absolutely no indication that this is what is expected of them. At least change the implementation to throw an exception or log an error or something so that users won't end up with a non-compiling application and no idea of how to proceed. Or at the very least add an entry to the changelog indicating how users are expected to handle this.

becksebenius-unity · 2022-09-14T19:47:19Z

As we see it, UTP 2 is already considered a breaking change. Technically, you need to get UTP2 to have this breaking change. If you just get the latest of NGO, it's not a breaking change. If you are willing to go to UTP2, you are accepting that change.

I'm not sure I understand what you mean by UTP 2.0 being a breaking change. With the recent changes that landed, someone installing UTP 2.0 will get something that still works without having to modify their code. All breaking API changes introduced by UTP 2.0 are handled by NGO through version defines. But with the proposed changes here, someone could install UTP 2.0 and get a non-compiling project without any indication as to what happened.

I understand (and support) the rationale of not wanting to support the existing debug simulator if UTP 2.0 is installed. That's fine. Users with UTP 2.0 should install the tools package to get the new and improved network simulator. But straight out removing the API gives them absolutely no indication that this is what is expected of them. At least change the implementation to throw an exception or log an error or something so that users won't end up with a non-compiling application and no idea of how to proceed. Or at the very least add an entry to the changelog indicating how users are expected to handle this.

An alternative to this would to be use [HideInInspector] to remove it from the inspector UI, and then use [Obsolete] tag to indicate to users that the API should not be used if they have UTP2.0 installed. I believe this would satisfy semantic versioning and give better feedback to users if they're relying on that API.

Rosme · 2022-09-14T21:20:33Z

As we see it, UTP 2 is already considered a breaking change. Technically, you need to get UTP2 to have this breaking change. If you just get the latest of NGO, it's not a breaking change. If you are willing to go to UTP2, you are accepting that change.

I'm not sure I understand what you mean by UTP 2.0 being a breaking change. With the recent changes that landed, someone installing UTP 2.0 will get something that still works without having to modify their code. All breaking API changes introduced by UTP 2.0 are handled by NGO through version defines. But with the proposed changes here, someone could install UTP 2.0 and get a non-compiling project without any indication as to what happened.
I understand (and support) the rationale of not wanting to support the existing debug simulator if UTP 2.0 is installed. That's fine. Users with UTP 2.0 should install the tools package to get the new and improved network simulator. But straight out removing the API gives them absolutely no indication that this is what is expected of them. At least change the implementation to throw an exception or log an error or something so that users won't end up with a non-compiling application and no idea of how to proceed. Or at the very least add an entry to the changelog indicating how users are expected to handle this.

An alternative to this would to be use [HideInInspector] to remove it from the inspector UI, and then use [Obsolete] tag to indicate to users that the API should not be used if they have UTP2.0 installed. I believe this would satisfy semantic versioning and give better feedback to users if they're relying on that API.

I believe the issue that @JesseOlmer was pointing out though is that we still have an existing API that doesn't do anything in that situation, though the [Obsolete] part could indicate that.

com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs

JesseOlmer · 2022-09-21T17:01:28Z

com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs

+        internal static event Action<int, NetworkDriver> TransportInitialized;
+        internal static event Action<int> TransportDisposed;


Any reason we can't make these public and then remove the internalsvisibleto? I'd love to get rid of "hidden" public apis.

com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs

Co-authored-by: Jesse Olmer <jesseo@unity3d.com>

…Unity-Technologies#2184) * Adding support for netsim to be able to work with Tools adapter * Setting up Simulator Pipeline stage if NetSim is available * Deprecate DebugSimulator when using UTP2 Co-authored-by: Jesse Olmer <jesseo@unity3d.com> Co-authored-by: DenninDalke <dennindalke@gmail.com>

Adding support for netsim to be able to work with Tools adapter

7c1ebbc

Rosme requested a review from a team September 8, 2022 20:18

Rosme requested review from 0xFA11 and a team as code owners September 8, 2022 20:18

Rosme commented Sep 8, 2022

View reviewed changes

com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs Outdated Show resolved Hide resolved

ian-m-unity reviewed Sep 8, 2022

View reviewed changes

simon-lemay-unity reviewed Sep 8, 2022

View reviewed changes

com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs Outdated Show resolved Hide resolved

ian-m-unity reviewed Sep 8, 2022

View reviewed changes

com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs Outdated Show resolved Hide resolved

Adding the simulator only if active in build inside unity transport.

92de75a

+ adding back missing parameters

simon-lemay-unity approved these changes Sep 12, 2022

View reviewed changes

becksebenius-unity reviewed Sep 12, 2022

View reviewed changes

com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs Outdated Show resolved Hide resolved

Rosme requested a review from JesseOlmer September 12, 2022 18:09

JS Fauteux added 2 commits September 12, 2022 17:26

Setting up Simulator Pipeline stage if NetSim is available

49a0ceb

Merge branch 'develop' into feature/tools-netsim-support

95ba089

JesseOlmer suggested changes Sep 13, 2022

View reviewed changes

JS Fauteux and others added 2 commits September 14, 2022 10:27

Apply suggestions from code review

d748605

Co-authored-by: Jesse Olmer <jesseo@unity3d.com>

Removal of Debug Simulator in UTP1, updated callback to avoid moving to

23a0f5b

much stuff around

JS Fauteux added 2 commits September 14, 2022 15:00

Merge branch 'develop' into feature/tools-netsim-support

1461103

Fixed compiling issues

55a131c

DenninDalke added 2 commits September 19, 2022 12:13

Fix compiler error typo: "river" should be "driver"

87144f0

Deprecate DebugSimulator when using UTP2

3093b22

DenninDalke requested a review from JesseOlmer September 19, 2022 19:16

simon-lemay-unity reviewed Sep 19, 2022

View reviewed changes

com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs Outdated Show resolved Hide resolved

DenninDalke requested a review from simon-lemay-unity September 20, 2022 17:20

DenninDalke requested a review from becksebenius-unity September 20, 2022 17:20

DenninDalke force-pushed the feature/tools-netsim-support branch from ca9696b to 752ddd7 Compare September 20, 2022 21:32

Deprecate SetDebugSimulatorParameters on UTP1

838485b

DenninDalke force-pushed the feature/tools-netsim-support branch from 752ddd7 to 838485b Compare September 20, 2022 21:33

Merge branch 'develop' into feature/tools-netsim-support

3abb7b0

simon-lemay-unity approved these changes Sep 21, 2022

View reviewed changes

JesseOlmer approved these changes Sep 21, 2022

View reviewed changes

DenninDalke and others added 4 commits September 21, 2022 11:08

Add tool suggestion to DebugSimulator deprecation

9263fd1

Co-authored-by: Jesse Olmer <jesseo@unity3d.com>

Add tool suggestion for DebugSimulator deprecation and comment

afbcdcc

Move comments out the parameter list to avoid CI failure

5063a87

Fix formatting

61e04e5

DenninDalke force-pushed the feature/tools-netsim-support branch from 5b05dda to 61e04e5 Compare September 22, 2022 22:21

DenninDalke removed the request for review from becksebenius-unity September 22, 2022 23:19

Merge branch 'develop' into feature/tools-netsim-support

af4f757

0xFA11 approved these changes Sep 23, 2022

View reviewed changes

DenninDalke enabled auto-merge (squash) September 23, 2022 02:13

DenninDalke approved these changes Sep 23, 2022

View reviewed changes

DenninDalke merged commit 5989836 into develop Sep 23, 2022

DenninDalke deleted the feature/tools-netsim-support branch September 23, 2022 02:13


		#if UNITY_EDITOR \|\| DEVELOPMENT_BUILD
		#if UNITY_EDITOR \|\| DEVELOPMENT_BUILD \|\| UNITY_MP_TOOLS_NETSIM_IMPLEMENTATION_ENABLED

		internal static event Action<int, NetworkDriver> TransportInitialized;
		internal static event Action<int> TransportDisposed;

feat: Adding support for netsim to be able to work with Tools adapter #2184

feat: Adding support for netsim to be able to work with Tools adapter #2184

Uh oh!

Conversation

Rosme commented Sep 8, 2022

Testing and Documentation

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Rosme commented Sep 14, 2022

Uh oh!

simon-lemay-unity commented Sep 14, 2022

Uh oh!

Rosme commented Sep 14, 2022

Uh oh!

simon-lemay-unity commented Sep 14, 2022

Uh oh!

becksebenius-unity commented Sep 14, 2022

Uh oh!

Rosme commented Sep 14, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!