Skip to content

Remove insert_or_spawn function family #18148

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

Jump to bottom
Merged
merged 10 commits into from
May 5, 2025
Merged

Remove insert_or_spawn function family #18148

merged 10 commits into from
May 5, 2025

Conversation

ElliottjPierce
Copy link
Contributor

@ElliottjPierce ElliottjPierce commented Mar 3, 2025 •
edited
Loading

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 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.

@alice-i-cecile alice-i-cecile added this to the 0.17 milestone Mar 4, 2025
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change S-Blocked This cannot move forward until something else changes C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Mar 4, 2025
@ElliottjPierce ElliottjPierce mentioned this pull request Mar 18, 2025
Closed
@ElliottjPierce ElliottjPierce mentioned this pull request Apr 1, 2025
Open
@ElliottjPierce ElliottjPierce mentioned this pull request Apr 4, 2025
Open
9 tasks
@ElliottjPierce ElliottjPierce added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Blocked This cannot move forward until something else changes labels Apr 18, 2025
@ElliottjPierce
Copy link
Contributor Author

removed blocked since #18147 is merged.

Henauxg
Henauxg approved these changes Apr 25, 2025
View reviewed changes
Copy link
Contributor

@Henauxg Henauxg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There was a comment from Cart in #18054 saying

If we plan on removing support for this, we should also look into some of the "invalid checks" in Entities. Iirc we added additional checking to support this scenario, and we might be able to simplify / optimize some things. Worth digging up the PRs that introduced those changes.

Did someone have a look, if not we could maybe just track this somewhere ?

@ElliottjPierce
Copy link
Contributor Author

Did someone have a look, if not we could maybe just track this somewhere ?

Yup! I've rewritten most of the Entities allocator 9 times now lol. I'm picturing building from #18670 (which this is blocking). If we don't do v9, I'll take another look at what we end up with.

Also marking as ready for final since it has two approvals. (I think that's how that's supposed to work.)

@ElliottjPierce ElliottjPierce 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-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 25, 2025
@NthTensor NthTensor added M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Apr 27, 2025
@github-actions GitHub Actions
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

@NthTensor
Copy link
Contributor

Please commit your migration guide as a file following the instructions above.

@ElliottjPierce
Copy link
Contributor Author

Please commit your migration guide as a file following the instructions above.

Oops. Thanks for pointing that out. Migration guide has been migrated lol.

@NthTensor NthTensor 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-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Apr 28, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 5, 2025
Merged via the queue into bevyengine:main with commit bfc76c5 May 5, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atlv24 atlv24 atlv24 approved these changes

@Henauxg Henauxg Henauxg approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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
Milestone
0.17
Development

Successfully merging this pull request may close these issues.

Improve Performance of Entities::alloc_at
6 participants
@ElliottjPierce @NthTensor @atlv24 @Henauxg @alice-i-cecile @JaySpruce