-
Notifications
You must be signed in to change notification settings - Fork 461
feat: Addressables Redux #1882
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Addressables Redux #1882
Changes from all commits
5a26513
aeaed97
4614416
4d9ee6f
4d9de53
7c8e43a
59455cd
d4f86f3
7dea1a9
6ab9310
3dfac95
4d57292
b1ebda5
0e3784e
1507eed
2d44191
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| using System; | ||
| using System.Collections.Generic; | ||
|
|
||
| namespace Unity.Netcode | ||
| { | ||
| /// <summary> | ||
| /// This class is used to support testable code by allowing any supported component used by NetworkManager to be replaced | ||
| /// with a mock component or a test version that overloads certain methods to change or record their behavior. | ||
| /// Components currently supported by ComponentFactory: | ||
| /// - IDeferredMessageManager | ||
| /// </summary> | ||
| internal static class ComponentFactory | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this implementation required or good to have?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, can this live under
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.) |
||
| { | ||
| internal delegate object CreateObjectDelegate(NetworkManager networkManager); | ||
|
|
||
| private static Dictionary<Type, CreateObjectDelegate> s_Delegates = new Dictionary<Type, CreateObjectDelegate>(); | ||
|
|
||
| /// <summary> | ||
| /// Instantiates an instance of a given interface | ||
| /// </summary> | ||
| /// <param name="networkManager">The network manager</param> | ||
| /// <typeparam name="T">The interface to instantiate it with</typeparam> | ||
| /// <returns></returns> | ||
| public static T Create<T>(NetworkManager networkManager) | ||
| { | ||
| return (T)s_Delegates[typeof(T)](networkManager); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Overrides the default creation logic for a given interface type | ||
| /// </summary> | ||
| /// <param name="creator">The factory delegate to create the instance</param> | ||
| /// <typeparam name="T">The interface type to override</typeparam> | ||
| public static void Register<T>(CreateObjectDelegate creator) | ||
| { | ||
| s_Delegates[typeof(T)] = creator; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Reverts the creation logic for a given interface type to the default logic | ||
| /// </summary> | ||
| /// <typeparam name="T">The interface type to revert</typeparam> | ||
| public static void Deregister<T>() | ||
| { | ||
| s_Delegates.Remove(typeof(T)); | ||
| SetDefaults(); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Initializes the default creation logic for all supported component types | ||
| /// </summary> | ||
| public static void SetDefaults() | ||
| { | ||
| SetDefault<IDeferredMessageManager>(networkManager => new DeferredMessageManager(networkManager)); | ||
| } | ||
|
|
||
| private static void SetDefault<T>(CreateObjectDelegate creator) | ||
| { | ||
| if (!s_Delegates.ContainsKey(typeof(T))) | ||
| { | ||
| s_Delegates[typeof(T)] = creator; | ||
| } | ||
| } | ||
| } | ||
| } | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Timeproperties have aSecorSecondssuffix.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll update the changelog to mention SpawnTimeout though.