Flecs.NET icon indicating copy to clipboard operation
Flecs.NET copied to clipboard

[v4] SetInternal component size regression?

Open Danon5 opened this issue 1 year ago • 1 comments

Hello. I have not delved deep into the source code for this project, however, I noticed what appears to be a regression in the 4.0.0 release. Prior to 4.0.0, reference type components were (what I assume to be) correctly sized as sizeof(IntPtr) when calling SetInternal in Entity.cs.

        // Old (working)
        private ref Entity SetInternal<T>(ulong id, ref T component)
        {
            Ecs.Assert(Type<T>.GetSize() != 0,
                "Zero-sized types can't be used as components. Use .Add() to add them as tags instead.");

            bool isRef = RuntimeHelpers.IsReferenceOrContainsReferences<T>();
            int size = isRef ? sizeof(IntPtr) : sizeof(T); // Here

            fixed (void* data = &component)
            {
                IntPtr ptr = default;
                ecs_set_id(World, Id, id, (IntPtr)size, isRef ? Managed.AllocGcHandle(&ptr, ref component) : data);
                return ref this;
            }
        }

After 4.0.0, they are incorrectly sized as (IntPtr)sizeof(T). This seems to cause issues with struct components that contain reference types inside them. In my case, I was beginning a defer, then calling entity.Set(someValue) where someValue was a struct containing a class reference and a bool. It resulted in an internal exception from flecs due to a type size mismatch.

            // Current
            if (RuntimeHelpers.IsReferenceOrContainsReferences<T>())
            {
                Managed.AllocGcHandle(component, out GCHandle handle);
                ecs_set_id(World, Id, id, (IntPtr)sizeof(T), &handle);
            }
            else
            {
                ecs_set_id(World, Id, id, (IntPtr)sizeof(T), component);
            }

In my project, changing the current code to this fixed any issues I was having:

            // Current (revised)
            if (RuntimeHelpers.IsReferenceOrContainsReferences<T>())
            {
                Managed.AllocGcHandle(component, out GCHandle handle);
                ecs_set_id(World, Id, id, (IntPtr)sizeof(IntPtr), &handle); // Simply changed sizeof(T) to sizeof(IntPtr)
            }
            else
            {
                ecs_set_id(World, Id, id, (IntPtr)sizeof(T), component);
            }

Danon5 avatar Aug 02 '24 08:08 Danon5

Yep you're right, it should be sizeof(IntPtr) or sizeof(GCHandle). This will be fixed in the next NuGet release.

BeanCheeseBurrito avatar Aug 03 '24 02:08 BeanCheeseBurrito