Skip to content

fix world_delete #293

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Ukendio merged 2 commits into from
Jan 2, 2026
Merged

fix world_delete #293

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f2dada4
fix world_delete clearing archetype entities before proper cleanup
PepeElToro41 Jan 2, 2026
e3dc5c7
make changes
PepeElToro41 Jan 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 43 additions & 10 deletions src/jecs.luau
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3497,10 +3497,14 @@ local function world_new(DEBUG: boolean?)
end
if idr_t then
local archetype_ids = idr_t.records
local to_remove = {}
Copy link
Owner

@Ukendio Ukendio Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type this as { [i53]: componentrecord}


for archetype_id in archetype_ids do
local idr_t_archetype = archetypes[archetype_id]
local idr_t_types = idr_t_archetype.types
local entities = idr_t_archetype.entities
local should_delete = false
local remove_count = 0

for _, id in idr_t_types do
if not ECS_IS_PAIR(id) then
Expand All @@ -3515,27 +3519,56 @@ local function world_new(DEBUG: boolean?)
local flags = id_record.flags
local flags_delete_mask = bit32.btest(flags, ECS_ID_DELETE)
if flags_delete_mask then
for i = #entities, 1, -1 do
local child = entities[i]
world_delete(world, child)
end
should_delete = true
break
else
local on_remove = id_record.on_remove
to_remove[id] = id_record
remove_count += 1
end
end

local to = archetype_traverse_remove(world, id, idr_t_archetype)
for i = #entities, 1, -1 do
if should_delete then
for i = #entities, 1, -1 do
local child = entities[i]
world_delete(world, child)
end
else
if remove_count == 1 then
local id, id_record = next(to_remove)
local to = archetype_traverse_remove(world, id :: i53, idr_t_archetype)
local on_remove = (id_record :: componentrecord).on_remove
for i = #entities, 1, -1 do
Copy link
Owner

@Ukendio Ukendio Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

local child = entities[i]
if on_remove then
on_remove(child, id)
on_remove(child, id :: i53)
Copy link
Owner

@Ukendio Ukendio Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type cast is most definitely unnecessary.

end

local r = entity_index_try_get_unsafe(child) :: record
inner_entity_move(child, r, to)
end
end
end
elseif remove_count > 1 then
local dst_types = table.clone(idr_t_types)
for id, record in to_remove do
table.remove(dst_types, table.find(dst_types, id))

local on_remove = record.on_remove
Copy link
Owner

@Ukendio Ukendio Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if on_remove then
for _, child in entities do
on_remove(child, id :: i53)
Copy link
Owner

@Ukendio Ukendio Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as aforementioned. The type cast is unnecessary.

end
end
end

local to = archetype_ensure(world, dst_types)
for i = #entities, 1, -1 do
local child = entities[i]
local r = entity_index_try_get_unsafe(child) :: record
inner_entity_move(child, r, to)
end
end
end
Copy link
Owner

@Ukendio Ukendio Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


table.clear(to_remove)
archetype_destroy(world, idr_t_archetype)
end
end
Expand Down