feat: Addressables Redux#1882
Conversation
- Disable deferring spawn messages when `ForceSamePrefabs` is true because there's no point
| [Tooltip("The amount of time a message should be buffered for without being consumed. If it is not consumed within this time, it will be dropped")] | ||
| public float MessageBufferTimeout = 20f; | ||
| [Tooltip("The amount of time a message should be buffered if the asset or object needed to process it doesn't exist yet. If the asset is not added/object is not spawned within this time, it will be dropped")] | ||
| public float SpawnTimeout = 1f; |
There was a problem hiding this comment.
perhaps?
| public float SpawnTimeout = 1f; | |
| public float SpawnTimeoutSec = 1f; |
There was a problem hiding this comment.
I started to name it this, actually, but we have other time values in NetworkConfig that don't specify units. I'm more in favor of consistency here unless we change them all to specify their units - though that'd still be inconsistent with Unity as a whole, since none of the Time properties have a Sec or Seconds suffix.
There was a problem hiding this comment.
should we add changelog entries like "removed MessageBufferTimeout from NetworkConfig" and rephrase other added changelog entry to mention SpawnTimeout somehow there too?
There was a problem hiding this comment.
I don't think it's really worth mentioning the removal of MessageBufferTimeout given that the message buffers were removed before we ever released 1.0-pre, so that config was already dead and had no function. I removed it because it was dead code.
There was a problem hiding this comment.
I'll update the changelog to mention SpawnTimeout though.
| /// Components currently supported by ComponentFactory: | ||
| /// - IDeferredMessageManager | ||
| /// </summary> | ||
| internal static class ComponentFactory |
There was a problem hiding this comment.
is this implementation required or good to have?
There was a problem hiding this comment.
This specific implementation isn't required but the tests need to have some ability to replace the DeferredMessageManager with a test-specific version. It's either a factory method that can be replaced like this, or NetworkManager constructs the default one and the test then assigns a new one... which works in this case, but may not work in other cases. This is just one possible implementation of Dependency Injection, but Dependency Injection as a pattern is really important for good testable code.
There was a problem hiding this comment.
hmm, can this live under Tests/ ?
There was a problem hiding this comment.
No, it's still needed in the runtime, otherwise NetworkManager doesn't know how to create the DeferredMessageManager. :) The idea is that this provides default construction logic (which is what's used at runtime) but allows it to be overridden (which is used for tests - though this pattern could also be used to let users provide their own implementations of specific components, but that's a discussion we'd need to have about if we want to support that, so I left it internal for now.)
| public class DeferredMessageTestNetworkVariableComponent : NetworkBehaviour | ||
| { | ||
| public NetworkVariable<int> TestNetworkVariable = new NetworkVariable<int>(); | ||
| } | ||
|
|
||
| public class DeferredMessageTestRpcAndNetworkVariableComponent : NetworkBehaviour | ||
| { | ||
| public bool ClientRpcCalled; | ||
|
|
||
| [ClientRpc] | ||
| public void SendTestClientRpc() | ||
| { | ||
| ClientRpcCalled = true; | ||
| } | ||
|
|
||
| public NetworkVariable<int> TestNetworkVariable = new NetworkVariable<int>(); | ||
| } |
There was a problem hiding this comment.
what was the rationale behind separating these two? could we have one comp with NetVar & RPC in it?
There was a problem hiding this comment.
Test isolation. If a NetworkVariable is present, NetworkVariableDelta messages will get sent. Makes it hard for a test to reliably verify that the message that was captured was the RPC message.
Co-authored-by: Fatih Mar <mfatihmar@gmail.com>
NoelStephensUnity
left a comment
There was a problem hiding this comment.
Other than it could use more comments in various places.
The over-all PR looks good, passes in stand-alone, and fixes other edge case scenarios with the deferred messages addition! (yay)
LGTM!
|
Seems like there are 2 things going on here, the new |
|
So on the per-NetworkObject queueing of messages...say you have a horse and rider game objects, and they interact in script code, and just the horse comes in late. If you have object inter-dependencies, but only queue one of them, what happens? I guess I'm intimating, can we safely assume that we can have per-object queues and unwind them independently and expect things to work consistently? |
This PR isn't functional without |
I would say if that's something that can happen, it's a problem user code has to handle (i.e., the rider needs to verify the horse exists before it does any gameplay logic on it). There's no way we can know that the rider depends on the horse, and I don't think we should try to predict and solve every gameplay problem for our users. We just need to document that this can happen and make sure our users are aware of that. |
|
|
||
| triggers[key].TriggerData.Add(new TriggerData | ||
| { | ||
| Reader = new FastBufferReader(reader.GetUnsafePtr(), Allocator.Persistent, reader.Length), |
There was a problem hiding this comment.
I am not sure about this, but doesn't this allocate each time you are "deferring" a message?
There was a problem hiding this comment.
Not in the way you're thinking. FastBufferReader is a struct so it's allocated on the stack. It contains only an unsafe pointer so its size is only one word (so copying it to pass it around is as fast as passing a long around). Its underlying data is allocated using Unity's native allocators (same ones used for NativeList and NativeArray) - so yes, there's an allocation, but it's a low-level malloc and doesn't create garbage.
It's unavoidable though because the existing reader was allocated with Allocator.TempJob and those have a maximum lifetime of 4 frames. Since the deferred messages can last up to 60 frames (or more if NetworkConfig.SpawnTimeout is increased) it has to be moved to Allocator.Persistent. The whole buffer copy isn't ideal... But this is here to handle an edge-case scenario so it's not like it'll be happening constantly.
…t/addrssables_redux # Conflicts: # com.unity.netcode.gameobjects/CHANGELOG.md
- Reverted unintentional file change - Addressed more review feedback
|
you could also re-save (re-serialize) these scene files to cause Unity replace
|
I need to understand a few things a little better.
| var sourcePrefabGlobalObjectIdHash = (uint)0; | ||
| var targetPrefabGlobalObjectIdHash = (uint)0; |
There was a problem hiding this comment.
I can't quite follow objectidhash changes here and below. I'll try to grab you some time to discuss and understand better.
|
Whoops. Did not mean to push the close button. |
As opposed to the previous addressables PR, this one focuses on unblocking users from being able to make their own addressables implementations. This PR adds two things:
NetworkConfig.ForceSamePrefabsis false)In this implementation, it's up to the user to load their own addressables in whatever way they choose, and tell the SDK about them after the addressable has been loaded into a GameObject with a NetworkObject component. When this is done dynamically at runtime, it's also up to the user (using whatever methods are appropriate for their game - deterministic loading when changing levels, RPCs, etc) to ensure both server and client have the required assets before the server sends a spawn message to the client. We will not make any attempt to trigger the client to load something just because the server did, as that may not be desirable in all cases.
Changelog
Testing and Documentation