fix: mtt-857 GitHub issue 915

[jeffreyrainy]

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.

[jeffreyrainy]

This prevents the warning from showing

[LukeStampfli]

Add a code comment here maybe. Else I'm afraid someone else won't understand this in the future.

[LukeStampfli]

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]

It all looks real good Jeffrey

[jeffreyrainy]

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?

[becksebenius-unity]

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.

[LukeStampfli]

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.

[becksebenius-unity]

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

[LukeStampfli]

Ship it!

[mattwalsh-unity]

Is this a list vs. a set because EnterInternalCommandContext needs an array?

[jeffreyrainy]

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 ?