InstancingSpreadComponent copies all matrices every frame when SpreadBuilder is connected

[tonfilm]

This node became pretty intransparent in terms of memory management, we just had many GC2 collections because this node can now use a SpreadBuilder as input.

The problem seems to be this code, which basically only avoids a copy for MutableArray, Spread, and ImmutableArray (almost never in use). So this should rather change into an adaptive implementation which will get red when an unsupported collection is connected or at least trigger a warning or have an output pin that informs the user that the data is copied and show the user a list of collections that work without a copy:

    public static bool TryGetArray<T>(this IEnumerable<T> sequence, out T[] array)
    {
        if (sequence is T[] _array)
        {
            array = _array;
            return true;
        }
        else if (sequence is Spread<T> spread)
        {
            array = spread.GetInternalArray();
            return true;
        }
        else if (sequence is ImmutableArray<T> immutableArray)
        {
            array = Unsafe.As<ImmutableArray<T>, T[]>(ref immutableArray);
            return true;
        }
        else
        {
            array = default;
            return false;
        }
    }

The idea of supporting more collections is definitely good, but then also the name should change and just be called InstancingComponent or so. And as is very likely that it deals with big memory chunks the memory behavior should be transparent.

[Elias]

The method you mentioned is in turn called by the StoreSequence node, and as its description text says, it tries to fetch the upstream array (the code you posted), but if it can’t it will copy the input sequence into its own internal array. But because the downstream node wants an array, you can get very unlucky here if your count changes all the time because then the array needs to be resized all the time. Was that what happended in your case? Otherwise if the count stayed constant you shouldn’t have been confronted with memory allocations, just with a mem-copy.

So if we wanted to get rid of the copying / potential re-allocation issue alltogether, the instancing component itself should accept a more memory friendly type, like say ArraySegment<T>. That would allow for an efficient data transfer from upstream without any copying. Would that be doable on the Stride end of things?

[tonfilm]

Ah got it, yes the count changes often.

I think that in any case it’s better to use the buffer version of the instancing and copy directly from the collection to the GPU, if possible. The GraphicsData node tries to do that, it basically only needs start address and length.

The array and spread versions of the instancing are just wrappers that make it easier to work with these two types directly, wrapping tham again, like it’s done in the latest change, results an extra CPU copy most of the time and allocation if the count changes.