feat: addressables by ShadauxCat · Pull Request #1758 · Unity-Technologies/com.unity.netcode.gameobjects

ShadauxCat · 2022-03-01T16:13:36Z

MTT-2576
MTT-2578
MTT-2579
MTT-2580
MTT-2581
MTT-2582
MTT-2585

Changelog

com.unity.netcode.gameobjects

Added: The SDK now supports loading assets from addressables
Added: The SDK now supports modifying the NetworkPrefab list at runtime, so long as it's done before calling StartServer()/StartClient()/StartHost()

Testing and Documentation

Includes integration tests.
Includes documentation for previously-undocumented public API entry points.

# Conflicts: # com.unity.netcode.gameobjects/Runtime/Configuration/NetworkPrefab.cs # com.unity.netcode.gameobjects/Runtime/com.unity.netcode.runtime.asmdef

WhippetsAintDogs · 2022-03-02T04:48:44Z

Hi @ShadauxCat, I have a little question for you. It seems like all network addressables are pulled into memory upon client / server / host start. Wouldn't it be better if they were to be loaded only when we need them (on spawned) ? I've implemented addressables support in our project using custom NetworkPrefabHandlers registered using the GlobalObjectIdHash value as key instead of a direct reference to the addressable prefab's NetworkObject, and we load the addressable in memory only upon spawning on the client side (in the handler's Instantiate method). What are your thoughts on the matter ?

ShadauxCat · 2022-03-03T22:13:37Z

Hi @ShadauxCat, I have a little question for you. It seems like all network addressables are pulled into memory upon client / server / host start. Wouldn't it be better if they were to be loaded only when we need them (on spawned) ? I've implemented addressables support in our project using custom NetworkPrefabHandlers registered using the GlobalObjectIdHash value as key instead of a direct reference to the addressable prefab's NetworkObject, and we load the addressable in memory only upon spawning on the client side (in the handler's Instantiate method). What are your thoughts on the matter ?

We had discussed a lot of options like this when we approached this feature. Ultimately we decided for the current release we didn't have the time to properly implement and fully test things like this. It adds a lot of complexity to the spawning system since the spawn - and everything else directed at that object - would have to be delayed until the load is complete, and we're not able to use synchronous loading because, not only is it a potentially very bad idea for performance (i.e., delaying a frame until an asset is downloaded from a remote source when the key doesn't point to a local asset bundle), but that API isn't supported on all versions and platforms that the SDK supports (for example, it doesn't work on WebGL)... so we'd have to refactor huge portions of the SDK to be able to handle asynchronous asset loading without resulting in messages being dropped or reordered in an invalid way.

That's not to say we won't support this in a future version (though I also can't promise that we will), but for this version we don't have the resources to make it work correctly and reliably for all users and thoroughly test all the use cases.

…s if this will work or not...

- Removed code that builds addressables on startup and on change

… failing tests - More tests - Ensure all asset load failures call the OnStartupFailedCallback

… isn't caused by addressables code.

# Conflicts: # com.unity.netcode.gameobjects/CHANGELOG.md # com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs # com.unity.netcode.gameobjects/Runtime/com.unity.netcode.runtime.asmdef # com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTestHelpers.cs # testproject/Assets/Tests/Runtime/testproject.runtimetests.asmdef # testproject/Packages/manifest.json # testproject/ProjectSettings/EditorBuildSettings.asset

NoelStephensUnity

It all looks good!
There are just a few areas that need attention, and as long as we can compile test project without the addressables package to provide us with the ability/option to run our tests against that version of the SDK build I am good with this PR.

- Fixed PlayerAddressable not being respected and added tests for it - Split OnReadyCallback and OnStartupFailedCallback so that the ones provided as method parameters at startup are temporary but those assigned to the properties externally persist between startups.

… exists now.

…ressables

0xFA11 · 2022-03-19T00:29:37Z

+        {
+            "name": "com.unity.addressables",
+            "expression": "",
+            "define": "NETCODE_USE_ADDRESSABLES"


Suggested change

"define": "NETCODE_USE_ADDRESSABLES"

"define": "COM_UNITY_ADDRESSABLES"

0xFA11 · 2022-03-19T00:29:50Z

+        {
+            "name": "com.unity.addressables",
+            "expression": "",
+            "define": "NETCODE_USE_ADDRESSABLES"


Suggested change

"define": "NETCODE_USE_ADDRESSABLES"

"define": "COM_UNITY_ADDRESSABLES"

0xFA11 · 2022-03-21T11:52:35Z

+        }
+
+#if NETCODE_USE_ADDRESSABLES
+        public void AddNetworkPrefab(AssetReferenceGameObject addressableAsset)


AddNetworkPrefab -> AddNetworkAddressable?

also xmldoc on public APIs please?

0xFA11 · 2022-03-21T11:57:01Z

+
+            NetworkConfig.NetworkAddressables.Add(new NetworkAddressable { Addressable = addressableAsset });
+        }
+        public void AddNetworkPrefab(string address)


I think we should not have this method at all because it just wraps the ctor and it's confusing.

My thought process here was that, if the SDK is offering to handle all the loading and management of the asset, that it might be simpler for the user to only keep the string around on their end. Yes, it just wraps the constructor, but it gives the user the ability to not have to worry about implementation details of addressables at all and just mark their things as addressables and work with only strings.

I'm not super attached to this function existing, but just wanted to mention my thought process and see what you think before I get rid of it.

0xFA11 · 2022-03-21T11:58:18Z

+            return null;
+        }
+
+        public GameObject GetGameObjectForAddress(string address)


same, we probably should not have this method (just use the method above directly instead)

0xFA11 · 2022-03-21T14:19:07Z


+            if (State == NetworkManagerState.LoadingAssets)
+            {
+                if (!ResolveNetworkPrefabs())


I remember that we wanted to expose an async API to allow user customization. Something that users could chain their own async functions to set theirselves out before NetworkManager connects. As far as I can tell, this PR lacks that atm. Also, I think we could basically have an async API at this level and chain all async tasks/jobs down the line behind this function call, then just basically do .IsValid() & .IsCompleted here. That'd make the whole flow async without nesting "poll async state and return bool every earlyupdate".

mattwalsh-unity · 2022-03-22T17:36:26Z

+            {
+                if (m_Prefab == null)
+                {
+                    throw new Exception("Not yet loaded.");


Can we make this message more clear? Like "Addressable ____ not yet loaded."

mattwalsh-unity · 2022-03-22T17:45:04Z

+                if (m_AsyncOperationHandle.Status == AsyncOperationStatus.Succeeded)
+                {
+                    var result = m_AsyncOperationHandle.Result;
+                    NetworkPrefab.VerifyValidPrefab(result);


It seems odd to me to have a "verify" function that doesn't return a value. I see that it throws an exception which would bubble up to the caller of ResolveAsync. I also see there's another exception thrown on line 84. Is this what we want? Seems a little hard to reason about.

What would you suggest naming the function? CheckValidPrefab maybe?

CheckIfPrefabIsValid?

mattwalsh-unity · 2022-03-22T17:46:27Z

+                    throw new Exception($"Could not load addressable object: {Addressable.AssetGUID}");
+                }
+
+                return false;


So AsyncOperationStatus can be Failed, Succeeded of None. Is None a legit state we can fall into? Is None a 'false-y' condition but Failed is an exception-throwing condition? Or can line 82 be an 'else' for all non-'Succeeded' cases?

None = still loading. We just have to keep waiting when it's None.

(Hence the return false)

mattwalsh-unity · 2022-03-22T18:07:01Z

+        /// </summary>
+        public AssetReferenceGameObject OverridingTargetAddressable;
+
+        public bool ResolveAsync()


It looks like this function gets called by way of ResolveNetworkPrefabs()
both during StartHost/StartServer (once, obviously), then repeatedly in OnNetworkEarlyUpdate.

I spent a considerable amount of time tracing all this out; at a minimum can you explain in comments the behavior of this function? (e.g., this throws a "game over" exception if ____, returns true if ___, and returns false if ____ and should be called until it returns true)?

(xml doc public facing API)

mattwalsh-unity · 2022-03-22T18:08:54Z

 #endif

+        private bool ResolveNetworkPrefabs()
+        {


This function is pretty large - 300+ lines of code. Can it be broken into smaller pieces?

The vast majority of this code is moved code, not code I wrote... I'm always hesitant to try to squeeze these kinds of refactors of other code into unrelated changes just because I moved it from one place to another. This all used to be part of Initialize and had to be delayed until assets are loaded.

My initial reaction was similar to Matt's until I realized the majority of that came from the Initialize method. How I finally "convinced myself" that this is "ok" for now was that there would be no real functional point in trying to break it up into several functions inside of NetworkManager since this is all NetworkPrefab initialization related code that really should live somewhere outside of NetworkManager but still invoked by NetworkManager.
Now that it is all in its own function it will be easier to create a small task after this PR to migrate the code into a "NetworkPrefab" relative location...perhaps even beak it up into smaller chunks during or after that PR.
(like 1-2 minor PRs)

mattwalsh-unity · 2022-03-22T19:29:36Z


 namespace Unity.Netcode
 {
+    public enum NetworkManagerState


Feels like a natural thing to be in its own PR

Had this discussion with Fatih. Could have been its own PR if I'd predicted the need for it and done it first... But addressables can't function without it, and it also has a dependency on Addressables unless I remove the LoadingAssets state and reimplement all the places the state gets changed. Splitting it at this point, to me, adds both time risk (unable to get this PR approved until that one's done and merged in) and code risk (the implementation of state on develop would look slightly different and that adds risk of partial bits of that ending up left in by accident after a merge)

mattwalsh-unity · 2022-03-22T19:30:43Z

        internal void InvokeOnClientConnectedCallback(ulong clientId) => OnClientConnectedCallback?.Invoke(clientId);

+        /// <summary>
+        /// This callback is executed when the SDK is ready for use. Before this is executed, the SDK


This too I think belongs in its own PR / something worth a quick team sync on

Same reponse as your other comment.

mattwalsh-unity · 2022-03-22T19:32:39Z

+            AddNetworkPrefab(new AssetReferenceGameObject(address));
+        }
+
+        public GameObject GetGameObjectForAddressable(AssetReferenceGameObject addressableAsset)


What would the use case for this public API be? Do we need to expose this?

Just trying to make it a little easier for users to use the API. I find the addressables API a little clunky and difficult to understand so I thought having something like this might make it easier - since we're doing all the rest of the addressables API under the hood, just felt to me like it was worthwhile to let the user just not think about the addressables API at all if they don't want to. Not opposed to removing it if you think that's superfluous.

mattwalsh-unity · 2022-03-22T19:34:24Z

+            State = NetworkManagerState.LoadingAssets;

-            return false;
+            return ResolveNetworkPrefabs();


Again, per my earlier comment, embedding the transport start calls in ResolveNetworkPrefabs makes it harder to reason about / maintain this.

Have a new proposal that will help decouple these, but will probably be a separate PR that builds on this one.

mattwalsh-unity · 2022-03-22T19:35:26Z

                NetworkLog.LogInfo(nameof(ShutdownInternal));
            }

+#if NETCODE_USE_ADDRESSABLES


Prefer having a "ShutdownAddressables()" method vs. embedding this here

wackoisgod · 2022-03-23T17:55:34Z

Hi @ShadauxCat, I have a little question for you. It seems like all network addressables are pulled into memory upon client / server / host start. Wouldn't it be better if they were to be loaded only when we need them (on spawned) ? I've implemented addressables support in our project using custom NetworkPrefabHandlers registered using the GlobalObjectIdHash value as key instead of a direct reference to the addressable prefab's NetworkObject, and we load the addressable in memory only upon spawning on the client side (in the handler's Instantiate method). What are your thoughts on the matter ?

We had discussed a lot of options like this when we approached this feature. Ultimately we decided for the current release we didn't have the time to properly implement and fully test things like this. It adds a lot of complexity to the spawning system since the spawn - and everything else directed at that object - would have to be delayed until the load is complete, and we're not able to use synchronous loading because, not only is it a potentially very bad idea for performance (i.e., delaying a frame until an asset is downloaded from a remote source when the key doesn't point to a local asset bundle), but that API isn't supported on all versions and platforms that the SDK supports (for example, it doesn't work on WebGL)... so we'd have to refactor huge portions of the SDK to be able to handle asynchronous asset loading without resulting in messages being dropped or reordered in an invalid way.

That's not to say we won't support this in a future version (though I also can't promise that we will), but for this version we don't have the resources to make it work correctly and reliably for all users and thoroughly test all the use cases.

So I am curious... as someone who is invested in Unity's solution here, what was or is the point of just throwing in a solution and not actually doing the work to find the right solution? To me it just seems like a complete miss when it comes to wanting to provide customers any sort of value when you know it's not a great solution or even a tested solution.

As for the solution recommended by @WhippetsAintDogs is there any reason why that solution doesn't make sense? Clearly the dev has gotten something working with whats released today.

Also the purposed solution is pretty much useless for mobile devs because we don't want everything in memory on a low memory device and essentially with the current code it doesn't look like we have many options to really customize our loading strategy without just doing it ourselves much like how @WhippetsAintDogs did.

WhippetsAintDogs · 2022-03-23T18:16:44Z

As for the solution recommended by @WhippetsAintDogs is there any reason why that solution doesn't make sense? Clearly the dev has gotten something working with whats released today.

Hi @wackoisgod ! For our use-case, we're loading addressables on a per "sectors" / "levels" basis. So when something is spawned at runtime inside a given sector, it is already loaded on both the clients and the server upon the given sector entry (and unloaded upon its exit) using Addressables.LoadAssetAsync. So when the PrefabHandler resolve the spawning on the client-side, using operationHandle.WaitForCompletion() to make the instantiation synchronous (in order to comply with the handler's Instantiate method signature) has virtually no cost since the asset is already pulled in memory. However our gain is not having direct references to these prefabs in the NetworkManager that would be pulled on the game start, instead they are pulled per sector.

So in a sense, our solution works for that kind of use-case but not use-cases where you're really only pulling something in memory upon instantiation / spawning - the cost of making the addressable flow synchronous would be heavy (it's a while (true) on the main thread under the hood 😅).

ShadauxCat · 2022-03-23T18:22:12Z

@wackoisgod

So I am curious... as someone who is invested in Unity's solution here, what was or is the point of just throwing in a solution and not actually doing the work to find the right solution? To me it just seems like a complete miss when it comes to wanting to provide customers any sort of value when you know it's not a great solution or even a tested solution.

Also the purposed solution is pretty much useless for mobile devs because we don't want everything in memory on a low memory device and essentially with the current code it doesn't look like we have many options to really customize our loading strategy without just doing it ourselves much like how @WhippetsAintDogs did.

Have to let @lpmaurice speak to this, because it wasn't my call here. I agree with you and tried to push for it but that's not the decision that was made. I don't want to speak for Louis here so I'll hope he can come and chime in to give more detail on the reasoning for you.

As for the solution recommended by @WhippetsAintDogs is there any reason why that solution doesn't make sense? Clearly the dev has gotten something working with whats released today.

Their solution would only work if the asset is already loaded or can be loaded synchronously, which sounds like it works for their use case, but doesn't really work well for a general solution. Addressables may be stored in a remote location and may have to be downloaded before they can be loaded, which is not something we can allow to be done synchronously, and more importantly, some platforms (ex: WebGL) don't support the synchronous API, so if we were to build a solution based around that, we'd be building a solution that falls apart if addressables take too long to load and makes the SDK completely unavailable on platforms where the synchronous API isn't supported. To support load-and-unload-on-demand, we have to be able to do it asynchronously, and that bubbles down to all the possible things that could happen while we're waiting on the load to finish that will fail because they expect an object to be spawned that isn't.

As it is, we know that the solution doesn't support all use cases for addressables. We will have a more robust solution in a future release, but this solution at least offers the ability to support simple use cases such as patch downloads, DLC content, etc that can be obtained and loaded before the SDK starts.

wackoisgod · 2022-03-24T16:09:14Z

Have to let @lpmaurice speak to this, because it wasn't my call here. I agree with you and tried to push for it but that's not the decision that was made. I don't want to speak for Louis here so I'll hope he can come and chime in to give more detail on the reasoning for you.

Indeed we can hope so :)

As it is, we know that the solution doesn't support all use cases for addressables. We will have a more robust solution in a future release, but this solution at least offers the ability to support simple use cases such as patch downloads, DLC content, etc that can be obtained and loaded before the SDK starts.

I mean I would argue this doesn't even support the most common platform that bundles are used on which is mobile and its used in such a way that speaks to exactly to what you mentioned which is the unloading and loading of things on the fly async. I mean more often then not developers are using bundles as a means to track and manage at a fine grain level whats loaded on a device and if I really don't have that control because the SDK is not flexible enough to allow for it it just seems like I would not want to use it it.

I have said this in a past life but I do think the SDK should not care about what you are loading other than saying hey load this thing and should be an ID of some sort that is really up to the game to figure out what it needs to do with that. The real issue here is the SDK is so coupled to the the element of the prefab etc... that you can't have a representation of the network object separated from a prefab that would allow the simulation to continue to replicate for data and things load when they load for the visuals and sync up on that underlying network data.

To me it just seems this solution is checking some boxes (not your fault @ShadauxCat) but not really solving a much larger issue which is the fact the SDK is so coupled to prefabs and that essentially it forces game developers to adhear to whatever asset loading and data model it prescribes which just doesn't seem like its job.

ShadauxCat · 2022-04-19T19:49:31Z

We've rethought our approach to addressables. This PR is being closed. The new approach is in #1882.

TwoTenPvP and others added 3 commits February 24, 2022 16:48

feat: Added addressables UI

d905edc

Addressables Runtime Support

c32951a

Merge branch 'addressables-ui' into feat/addressables

e9c18b7

# Conflicts: # com.unity.netcode.gameobjects/Runtime/Configuration/NetworkPrefab.cs # com.unity.netcode.gameobjects/Runtime/com.unity.netcode.runtime.asmdef

ShadauxCat requested a review from 0xFA11 as a code owner March 1, 2022 16:13

ShadauxCat requested a review from NoelStephensUnity March 1, 2022 19:49

ShadauxCat added 3 commits March 1, 2022 13:52

Standards + changelog

7896c34

Addressables as a dependency

024351b

Fixed missing preprocessor guards, removed hard dependency.

8f747e4

NoelStephensUnity reviewed Mar 3, 2022

View reviewed changes

Comment thread com.unity.netcode.gameobjects/Runtime/Configuration/NetworkPrefab.cs Outdated

NoelStephensUnity reviewed Mar 3, 2022

View reviewed changes

Comment thread com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs

NoelStephensUnity reviewed Mar 3, 2022

View reviewed changes

Comment thread com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs

ShadauxCat and others added 14 commits March 4, 2022 16:47

Maybe fixes stand-alone test running.

a8395ef

An attempt at ensuring assets are properly built for CI use. Who know…

e0fd09e

…s if this will work or not...

standards

2d053a7

- Removed unnecessary #if block

667795a

- Removed code that builds addressables on startup and on change

- Improved resource management for addressables, should fix currently…

c19a1a1

… failing tests - More tests - Ensure all asset load failures call the OnStartupFailedCallback

standards

e29579a

Nullreference fix when no config is present during shutdown.

38520fe

This fixes an unrelated test but I need to make sure the test failure…

c73a59d

… isn't caused by addressables code.

Not sure how this managed to not get in the last commit.

4dd8152

Standards

cd98669

Fix transition to Ready state when scene management is enabled.

ef3a801

Merge branch 'develop' into feat/addressables

174dbbc

Fix compile error when addressables isn't present.

4ba92af