fix world_delete

[PepeElToro41]

Brief Description of your Changes.

Fixes world_delete removing target pair to all entities, which meant making the archetype empty, which prevented iterating the entities again for further cleanup.

Impact of your Changes

Needs to allocate and populate a dictionary for every archetype. And if removes more than one pair. It needs a more expensive archetype_ensure call.

Tests Performed

Additional Comments

[Ukendio]

Overall very happy that you took the time to reproduce the test and make a mockup implementation that handled the different invariants very well. Some slight edge cases that we discussed and also some nitpicks have been requested

[Ukendio]

You can delete the child as soon as you see the flag the first time, this way you do not iterate more types than necessary.

Changing this, renaming the variable from should_delete to something else would reflect the behaviour of the code better.

local deleted_any = false
local remove_count = 0
for _, id in idr_t_types do
	if not ecs_is_pair(id) then
		continue
	end
	local object = entity_index_get_alive(
	    entity_index, ecs_pair_second(id))
	if object ~= entity then
		continue
	end
	local id_record = component_index[id]
	local flags = id_record.flags
	local flags_delete_mask = bit32.btest(flags, ecs_id_delete)
	if flags_delete_mask then
		deleted_any = true
		for i = #entities, 1, -1 do
			local child = entities[i]
			world_delete(world, child)
		end
		break
	else
  		to_remove[id] = id_record
        remove_count += 1
	end
end

if deleted_any then
	continue
end
	   

[Ukendio]

type this as { [i53]: componentrecord}

[Ukendio]

This type cast is most definitely unnecessary.

[Ukendio]

Same as aforementioned. The type cast is unnecessary.

[Ukendio]

record just means row. we want to specify it means component_record in this context.

[Ukendio]

Don't make special cases for remove_count == 1.

For loops are really cheap and have instructions to stop. whereas manual next() calls is NOT optimized in the VM and are very costly.

This will also allow us to have less branches in general

I would much prefer this which I have now benchmarked to be just as fast even with higher fragmentation (5 pairs with the same target). This will also allow us to check for side effects!

local dst_types = table.clone(idr_t_types)
for id, component_record in to_remove do
    table.remove(dst_types, table.find(dst_types, id))
end

local to_u = archetype_ensure(world, dst_types)
for i = #entities, 1, -1 do
    local child = entities[i]
    local r = entity_index_try_get_unsafe(child) :: record
	local to = to_u
	for id, component_record in to_remove do
		local on_remove = component_record.on_remove
		if on_remove then
			-- NOTE(marcus): We could be smarter with this and
			-- assume hooks are deterministic and that they will
			-- move to the same archetype. However users often are not reasonable people.
			on_remove(child, id)
			local src = r.archetype
			if src ~= idr_t_archetype then
				to = archetype_traverse_remove(world, id, src)
			end
		end
	end

	inner_entity_move(child, r, to)
end