Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions com.unity.netcode.gameobjects/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
- Added `NetworkVariableWritePermission` to `NetworkVariableBase` and implemented `Owner` client writable netvars. (#1762)
- `UnityTransport` settings can now be set programmatically. (#1845)
- `FastBufferWriter` and Reader IsInitialized property. (#1859)
- Prefabs can now be added to the network at **runtime** (i.e., from an addressable asset). If `ForceSamePrefabs` is false, this can happen after a connection has been formed. (#1882)
- When `ForceSamePrefabs` is false, a configurable delay (default 1 second, configurable via `NetworkConfig.SpawnTimeout`) has been introduced to gracefully handle race conditions where a spawn call has been received for an object whose prefab is still being loaded. (#1882)

### Changed

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,10 +128,10 @@ public class NetworkConfig
public int LoadSceneTimeOut = 120;

/// <summary>
/// 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.
/// 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.
/// </summary>
[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;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps?

Suggested change
public float SpawnTimeout = 1f;
public float SpawnTimeoutSec = 1f;

Copy link
Copy Markdown
Collaborator Author

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 Time properties have a Sec or Seconds suffix.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay :(

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Collaborator Author

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.

Copy link
Copy Markdown
Collaborator Author

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.


/// <summary>
/// Whether or not to enable network logs.
Expand Down
65 changes: 65 additions & 0 deletions com.unity.netcode.gameobjects/Runtime/Core/ComponentFactory.cs
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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this implementation required or good to have?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, can this live under Tests/ ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The 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.

Loading