Conversation
… visible to them
… visible to them. performance (bandwidth). coding style
| if (messageQueueContainer != null) | ||
| { | ||
| if (networkObject != null) | ||
| if (networkObject != null && !networkObject.IsHidden) |
There was a problem hiding this comment.
This prevents the warning from showing
There was a problem hiding this comment.
Add a code comment here maybe. Else I'm afraid someone else won't understand this in the future.
|
|
||
| internal readonly HashSet<ulong> Observers = new HashSet<ulong>(); | ||
|
|
||
| internal bool IsHidden = false; |
There was a problem hiding this comment.
Can we add a code comment explaining what this does. E.g. that this is a client side only value which tracks the visibility/hidden state of the object?
NoelStephensUnity
left a comment
There was a problem hiding this comment.
It all looks real good Jeffrey
| // As long as we have any remaining clients, then notify of the object being destroy. | ||
| if (NetworkManager.ConnectedClientsList.Count > 0) | ||
| { | ||
| List<ulong> targetClientIds = new List<ulong>(); |
There was a problem hiding this comment.
This is now the proper behaviour. It creates a new list of target ClientIds, and fill it with the client ids of machine that don't have the object hidden already.
It allocates memory, though. Maybe someone more versed in C# can find an optimization?
There was a problem hiding this comment.
For non-threaded scenarios like this I like to have a List pool for common types to avoid allocations for utility use of lists. If we're concerned about threading, you could allocate a native array.
com.unity.netcode.gameobjects/Tests/Runtime/HiddenVariableTests.cs
Outdated
Show resolved
Hide resolved
| // As long as we have any remaining clients, then notify of the object being destroy. | ||
| if (NetworkManager.ConnectedClientsList.Count > 0) | ||
| { | ||
| var targetClientIds = new List<ulong>(); |
There was a problem hiding this comment.
So we could half our allocations here by making targetClientIds a member field and clearing it here instead of always allocating a new list. Since this always runs single threaded there's no issue with that.
There was a problem hiding this comment.
Here's a pattern I've used on a few projects before, so that we don't need have a bunch of redundant pooled lists sitting in memory:
public class ReuseableList <T> : List<T>, IDisposable
{
private static List<ReuseableList<T>> pool = new List<ReuseableList<T>>();
public static ReuseableList<T> GetInstance()
{
ReuseableList<T> list;
if (0 < pool.Count)
{
list = pool.Pop();
list.Clear();
}
else
{
list = new ReuseableList<T>(10);
}
return list;
}
public ReuseableList(int capacity) : base(capacity) {}
void IDisposable.Dispose()
{
Clear();
pool.Add(this);
}
}
then usage is
using var targetClientIds = ReuseableList<ulong>.GetInstance();
there's some slight cpu overhead to grabbing / disposing the list though so if this is a hot path it might not be applicable
| private ulong m_NetworkObjectIdCounter; | ||
|
|
||
| // A list of target ClientId, use when sending despawn commands. Kept as a member to reduce memory allocations | ||
| private List<ulong> m_TargetClientIds = new List<ulong>(); |
There was a problem hiding this comment.
Is this a list vs. a set because EnterInternalCommandContext needs an array?
There was a problem hiding this comment.
yeah, it needs an array for now, but arrays are unwieldy. I will change in the future with IMessage. I used a list for storage because that seemed the simpler to convert to an array when needed. The structure I used needed to be able to grow and be cleared, so List seemed to fit the bill. Would you rather have another structure ?
…wned. Prevents exception in IsNetworkVisibleTo()
…nsform * develop: (26 commits) fix: client connected InvokeOnClientConnectedCallback with scene management disabled (#1123) fix: removed `public` class `NetcodeObserver` (MTT-1157) (#1122) feat: add NetworkMessageSent/Received metrics (#1112) feat: snapshot. MTU sizing option for Snapshot. MTT-1087 (#1111) Add metrics for transport bytes sent and received (#1104) fix: Missing end profiling sample (#1118) chore: support standalone mode for netcode runtimetests (#1115) feat: Change MetricNames for a more complex value type (#1109) feat: Track scene event metrics (#1089) style: whitespace fixes (#1117) feat: replace scene registration with scenes in build list (#1080) fix: mtt-857 GitHub issue 915 (#1099) fix: NetworkSceneManager exception when DontDestroyOnLoad NetworkObjects are being synchronized (#1090) feat: NetworkTransform Custom Editor Inspector UI (#1101) refactor: remove TempGlobalObjectIdHashOverride (#1105) fix: MTT-1124 Counters are now reported in sync with other metrics (#1096) refactor: convert using var statements to using var declarations (#1100) chore: updated all of the namespaces to match the tools package change (#1095) refactor!: remove network variable settings, network behaviour cleanup (#1097) fix: mtt-1088 review. Safer handling of out-of-order or old messages (#1091) ... # Conflicts: # com.unity.netcode.gameobjects/Prototyping/NetworkTransform.cs
* wip * fix: issues 915, MTT-857, Clients are receiving data from objects not visible to them * fix: issues 915, MTT-857, Clients are receiving data from objects not visible to them. performance (bandwidth). coding style * fix: compile issue after merge * fix: proper detection of hidden/shown state * style: reverting whitespace changes * fix: adjusting metrics to get the rigth client ids * style: applying code review suggestion Co-authored-by: M. Fatih MAR <[email protected]> * style: coding standards * fix: reducing memory allocations by keeping a List<ulong> as a member Co-authored-by: M. Fatih MAR <[email protected]>
Fix for #915
Adds a test that show two of the three stated problems were already fixed.
Fixes the third one by not warning on despawn of hidden objects.
Reduces bandwidth usage by not sending netvar updates to hidden clients.