Skip to content
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.

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

Chained EntityCommands create useless temporary archetypes #5074

Open
alice-i-cecile opened this issue Jun 22, 2022 · 4 comments
Open

Chained EntityCommands create useless temporary archetypes #5074

alice-i-cecile opened this issue Jun 22, 2022 · 4 comments
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior C-Performance A change motivated by improving speed, memory usage or compile times C-Usability A targeted quality-of-life change that makes Bevy easier to use

Comments

@alice-i-cecile
Copy link
Member

What problem does this solve or what need does it fill?

When calling EntityCommands::insert (or remove, or the bundle equivalents) repeatedly within a single system, all intermediate archetypes are constructed.

Suppose we have an entity with the component A, and call .insert(B) and .insert(C). Despite only caring about enities that have the component sets (archetypes) {A} and {A, B, C}, the archetype {A, B} is also constructed.

This has both immediate performance costs, and reduces general program performance as these empty archetypes continue to exist.

What solution would you like?

Automatically batch all component-modifying methods on EntityCommands into a single modify_bundle EntityCommand for each entity before processing them.

What alternative(s) have you considered?

Users can acheive this effect by manually grouping these calls, but this is limited to pure insertion or pure removal EntityCommands, non-obvious and can lead to less clear code.

We could do even smarter batching strategies, e.g generating spawn_batch calls, but that is a) much harder and b) less immediately important.

Additional context

This is essential to ensuring that #1481 is both usable and maintains correctness at all times, otherwise these temporary archetypes are constructed and can fail the assertions generated.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jun 22, 2022
@Nilirad
Copy link
Contributor

Nilirad commented Jul 5, 2022

This will become a bug once #5121 gets merged. Should it be tagged as such?

@alice-i-cecile alice-i-cecile added the C-Bug An unexpected or incorrect behavior label Jul 5, 2022
@alice-i-cecile
Copy link
Member Author

It may even block #5121, as it's a serious hinderance to the usability.

@BoxyUwU
Copy link
Member

BoxyUwU commented Oct 7, 2022

I don't think that this should block #5121/Archetype invariants, removing "useless" archetype moves should be an optimization not fundamental to the usability of archetype invariants. It seems reasonable that archetype invariants cannot be checked after every command since then .insert(A).insert(B) may crash during the "intermediate" step.

An alternative solution might be to add stuff to the Command trait as to whether invariants have to hold before running the command?

@alice-i-cecile
Copy link
Member Author

An alternative solution might be to add stuff to the Command trait as to whether invariants have to hold before running the command?

This seems like a sensible solution; I think we could do this to unblock the work there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior C-Performance A change motivated by improving speed, memory usage or compile times C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

No branches or pull requests

3 participants