refactor: move UTP integration into NGO, delete UTP Adapter package #1823

0xFA11 · 2022-03-23T00:57:49Z

With this PR, we're removing com.unity.netcode.adapter.utp package entirely and moving its contents into the package directly. This makes the package depend on com.unity.transport package directly, also allows us to present UTP as our default, 1st-party supported transport in post-UNET world (2022+).

MTT-2884

Changelog

Added UnityTransport implementation and com.unity.transport package dependency

Testing and Documentation

Moved existing Editor and Runtime tests from UTP Adapter package into the package

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

0xFA11 · 2022-03-23T01:40:21Z

.github/pull_request_template.md


 <!-- Add JIRA link here. Short version (e.g. MTT-123) also works and gets auto-linked. -->

 <!-- Add RFC link here if applicable. -->

 ## Changelog

-### com.unity.netcode.gameobjects


removed section separation since there is no other package under this repo anymore

0xFA11 · 2022-03-23T01:49:12Z

com.unity.netcode.adapter.utp/package.json

@@ -1,11 +0,0 @@
-{
-  "name": "com.unity.netcode.adapter.utp",


I think deleting this package would mean something on UDash/Operate side too.
We should update that catalog thing we maintain for online dashboards people use via Unity Cloud Services site @ashwinimurt

yes there is, I will do a PR there and add you.

com.unity.netcode.gameobjects/package.json

0xFA11 · 2022-03-23T01:52:59Z

.yamato/project.metafile

-      - name: com.unity.netcode.adapter.utp
-        path: com.unity.netcode.adapter.utp


less work on Yamato, faster CI, faster PRs!

0xFA11 · 2022-03-23T12:25:48Z

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

 #if MULTIPLAYER_TOOLS_1_0_0_PRE_7
-                ExtractNetworkMetrics();
+                if (NetworkManager)
+                {
+                    ExtractNetworkMetrics();
+                }
 #endif


this is an additional change, not just a copy-paste from UTP Adapter package.
UnityTransportTests & UnityTransportConnectionTests don't necessarily require a NetworkManager instance to be there, they were working just fine.
however, ExtractNetworkMetrics(); requires one and there won't be a NetworkManager during these tests.
there are tests running with ExtractNetworkMetrics(); and NetworkManager present but this fix basically disables ExtractNetworkMetrics(); call in UnityTransportTests & UnityTransportConnectionTests only which I think is totally fine but just wanted to mention here. @becksebenius-unity @simon-lemay-unity

That looks fine to me. Makes sense in that context. I am not concerned about it.

simon-lemay-unity

Looks good to me! My only concern is not with the change itself, but how it will have to be handled by users. If someone already has the adapter package installed, when they update to this version of NGO, will things break? Assuming things do break, does it break in a way where it's pretty obvious what they need to do to fix the issue?

Rosme · 2022-03-23T14:24:05Z

com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTestHelpers.cs

@@ -109,9 +110,7 @@ public bool OnVerifyCanReceive(ulong senderId, Type messageType)
        public enum InstanceTransport
        {
            SIP,
-#if UTP_ADAPTER


Makes me so happy :D

Rosme · 2022-03-23T14:30:29Z

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

 #if MULTIPLAYER_TOOLS_1_0_0_PRE_7
-                ExtractNetworkMetrics();
+                if (NetworkManager)
+                {
+                    ExtractNetworkMetrics();
+                }
 #endif


That looks fine to me. Makes sense in that context. I am not concerned about it.

wackoisgod · 2022-03-23T17:31:00Z

While I think this is a reasonable change.... is there any concern at the fact that now you will have to release a new package any time UTP needs to be updated? One of the nice things about having the other package was if you needed to update UTP you could and you didn't pull in any changes for NGO as they were separated.

@simon-lemay-unity I agree with this concern because essentially you can install both packages independently of each other which means I could install a newer version of NGO that has UTP, and then install the other package and they will both install as long as they are API compatible and then potentially cause compiler errors. What I would recommend doing is actually adding some editor checks to see which version of NGO you are using and if you are using a version that has this change in it and you have the old package installed then remove it. You can use the package manager API for this... This is similar to what Unity is doing for the IL2CPP Cross compiler package except its uninstalling instead of installing.

I would also work to get the package marked as deprecated in the manifest.json file for an editor release because that will just remove the package if its present.

0xFA11 · 2022-03-23T18:18:41Z

While I think this is a reasonable change.... is there any concern at the fact that now you will have to release a new package any time UTP needs to be updated? One of the nice things about having the other package was if you needed to update UTP you could and you didn't pull in any changes for NGO as they were separated.

yes, this is expected and we're OK with that. if we want to fix UTP <-> NGO integration, that'll happen underneath NGO. also, if we want to upgrade UTP version, that also will happen in a minor NGO version release.

@simon-lemay-unity I agree with this concern because essentially you can install both packages independently of each other which means I could install a newer version of NGO that has UTP, and then install the other package and they will both install as long as they are API compatible and then potentially cause compiler errors. What I would recommend doing is actually adding some editor checks to see which version of NGO you are using and if you are using a version that has this change in it and you have the old package installed then remove it. You can use the package manager API for this... This is similar to what Unity is doing for the IL2CPP Cross compiler package except its uninstalling instead of installing.

as we briefly discussed, it does make sense to implement an editor code to remove UTP Adapter package automatically if present in the project. (for the reference: PackageManager.Client). I'll implement and update the PR shortly.

I would also work to get the package marked as deprecated in the manifest.json file for an editor release because that will just remove the package if its present.

yeah, I think we need to mark UTP Adapter as deprecated and people should not be able to search/discover it. I'll follow-up on that internally too.

thank you @wackoisgod :)

…ally from the project

0xFA11 · 2022-03-23T20:56:52Z

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

@@ -10,3 +10,4 @@
 [assembly: InternalsVisibleTo("TestProject.RuntimeTests")]
 [assembly: InternalsVisibleTo("Unity.Netcode.RuntimeTests")]
 [assembly: InternalsVisibleTo("Unity.Netcode.TestHelpers.Runtime")]
+[assembly: InternalsVisibleTo("Unity.Netcode.Adapter.UTP")]


this needed to be put back to prevent projects with UTP Adapter package installed from breaking.

0xFA11 added 5 commits March 23, 2022 00:57

refactor: move UTP integration into NGO, delete UTP Adapter package

488b77e

minor fixes

f2aff6f

changelog entry

8e7bc56

minor touches

dc809f2

update changelog

e0f96ab

0xFA11 commented Mar 23, 2022

View reviewed changes

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

0xFA11 added 4 commits March 23, 2022 01:29

update CODEOWNERS

fca917f

update CHANGELOG.md

fefd90a

update PR template

47d2a57

update pr template

2b58e02

0xFA11 commented Mar 23, 2022

View reviewed changes

com.unity.netcode.gameobjects/package.json Outdated Show resolved Hide resolved

0xFA11 commented Mar 23, 2022

View reviewed changes

0xFA11 added 2 commits March 23, 2022 11:57

delete [Obsolete] & [FormerlySerializedAs] members

2f3cf4e

fix testproject-tools-integration

f993874

0xFA11 commented Mar 23, 2022

View reviewed changes

0xFA11 marked this pull request as ready for review March 23, 2022 12:26

0xFA11 requested review from lpmaurice and a team as code owners March 23, 2022 12:26

simon-lemay-unity approved these changes Mar 23, 2022

View reviewed changes

Rosme approved these changes Mar 23, 2022

View reviewed changes

put "com.unity.collections": "1.1.0" dependency back

17e5769

implement UTPAdapterChecker to remove UTP Adapter package automatic…

a3bc068

…ally from the project

0xFA11 commented Mar 23, 2022

View reviewed changes

0xFA11 added 2 commits March 23, 2022 20:59

merge branch 'develop' into refactor/default-unity-transport

8c1af87

Merge branch 'develop' into refactor/default-unity-transport

c509073

ashwinimurt approved these changes Mar 23, 2022

View reviewed changes

0xFA11 enabled auto-merge (squash) March 23, 2022 23:18

0xFA11 merged commit 26dbf65 into develop Mar 23, 2022

0xFA11 deleted the refactor/default-unity-transport branch March 23, 2022 23:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move UTP integration into NGO, delete UTP Adapter package #1823

refactor: move UTP integration into NGO, delete UTP Adapter package #1823

0xFA11 commented Mar 23, 2022 •

edited

Loading

0xFA11 Mar 23, 2022

0xFA11 Mar 23, 2022

ashwinimurt Mar 23, 2022

0xFA11 Mar 23, 2022

0xFA11 Mar 23, 2022

Rosme Mar 23, 2022

simon-lemay-unity left a comment

Rosme Mar 23, 2022

Rosme Mar 23, 2022

wackoisgod commented Mar 23, 2022

0xFA11 commented Mar 23, 2022

0xFA11 Mar 23, 2022

		- name: com.unity.netcode.adapter.utp
		path: com.unity.netcode.adapter.utp

refactor: move UTP integration into NGO, delete UTP Adapter package #1823

refactor: move UTP integration into NGO, delete UTP Adapter package #1823

Conversation

0xFA11 commented Mar 23, 2022 • edited Loading

Changelog

Testing and Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simon-lemay-unity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wackoisgod commented Mar 23, 2022

0xFA11 commented Mar 23, 2022

Choose a reason for hiding this comment

0xFA11 commented Mar 23, 2022 •

edited

Loading