Skip to content

Deprecate insert_or_spawn function family #18147

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

Conversation

ElliottjPierce
Copy link
Contributor

@ElliottjPierce ElliottjPierce commented Mar 3, 2025

Objective

Based on #18054, this PR builds on #18035 to deprecate:

  • Commands::insert_or_spawn_batch
  • Entities::alloc_at_without_replacement
  • Entities::alloc_at
  • World::insert_or_spawn_batch
  • World::insert_or_spawn_batch_with_caller

Testing

Just deprecation, so no new tests. Note that as of writing #18035 is still under testing and review.

Open Questions

  • Should entity::AllocAtWithoutReplacement be deprecated? It is internal and only used in Entities::alloc_at_without_replacement. EDIT: Now deprecated.

Migration Guide

The following functions have been deprecated:

  • Commands::insert_or_spawn_batch
  • World::insert_or_spawn_batch
  • World::insert_or_spawn_batch_with_caller

These functions, when used incorrectly, can cause major performance problems and are generally viewed as anti-patterns and foot guns. These are planned to be removed altogether in 0.17.

Instead of these functions consider doing one of the following:

Option A) Instead of despawing entities and re-spawning them at a particular id, insert the new Disabled component without despawning the entity, and use try_insert_batch or insert_batch and remove Disabled instead of re-spawning it.

Option B) Instead of giving special meaning to an entity id, simply use spawn_batch and ensure entity references are valid when despawning.

@alice-i-cecile
Copy link
Member

Should entity::AllocAtWithoutReplacement be deprecated? It is internal and only used in Entities::alloc_at_without_replacement. Currently not deprecated.

Mark this as deprecated so it's easier to find and remove later please :)

@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 S-Blocked This cannot move forward until something else changes labels Mar 4, 2025
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Can you link the issue number in the deprecation notice please? I'd like to funnel affected users to it, and it's a much more compelling explanation than a bare assertion that this is slow.

@ElliottjPierce
Copy link
Contributor Author

Can you link the issue number in the deprecation notice please? I'd like to funnel affected users to it, and it's a much more compelling explanation than a bare assertion that this is slow.

Great idea! I've added the issue number. If you like, I'd be happy to change it to a full link too.

Copy link
Contributor

@Carter0 Carter0 left a comment

Choose a reason for hiding this comment

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

LGTM

@alice-i-cecile
Copy link
Member

Merging the prerequisite PR, then this.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Blocked This cannot move forward until something else changes labels Mar 6, 2025
@alice-i-cecile alice-i-cecile enabled auto-merge March 6, 2025 16:45
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 6, 2025
Merged via the queue into bevyengine:main with commit ed7b366 Mar 6, 2025
30 checks passed
github-merge-queue bot pushed a commit that referenced this pull request May 5, 2025
# Objective

Based on and closes #18054, this PR builds on #18035 and #18147 to
remove:

- `Commands::insert_or_spawn_batch`
- `Entities::alloc_at_without_replacement`
- `Entities::alloc_at`
- `entity::AllocAtWithoutReplacement`
- `World::insert_or_spawn_batch`
- `World::insert_or_spawn_batch_with_caller`

## Testing

Just removing unused, deprecated code, so no new tests. Note that as of
writing, #18035 is still under testing and review.

## Future Work

Per
[this](#18054 (comment))
comment on #18054, there may be additional performance improvements
possible to the entity allocator now that `alloc_at` no longer is
supported. At a glance, I don't see anything obvious to improve, but it
may be worth further investigation in the future.

---------

Co-authored-by: JaySpruce <[email protected]>
andrewzhurov pushed a commit to andrewzhurov/bevy that referenced this pull request May 17, 2025
# Objective

Based on and closes bevyengine#18054, this PR builds on bevyengine#18035 and bevyengine#18147 to
remove:

- `Commands::insert_or_spawn_batch`
- `Entities::alloc_at_without_replacement`
- `Entities::alloc_at`
- `entity::AllocAtWithoutReplacement`
- `World::insert_or_spawn_batch`
- `World::insert_or_spawn_batch_with_caller`

## Testing

Just removing unused, deprecated code, so no new tests. Note that as of
writing, bevyengine#18035 is still under testing and review.

## Future Work

Per
[this](bevyengine#18054 (comment))
comment on bevyengine#18054, there may be additional performance improvements
possible to the entity allocator now that `alloc_at` no longer is
supported. At a glance, I don't see anything obvious to improve, but it
may be worth further investigation in the future.

---------

Co-authored-by: JaySpruce <[email protected]>
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-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 S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants