Add `BundleRemover` by ElliottjPierce · Pull Request #18521 · bevyengine/bevy

ElliottjPierce · 2025-03-24T23:23:06Z

Objective

It has long been a todo item in the ecs to create a BundleRemover alongside the inserter, spawner, etc.

This is an uncontroversial first step of #18514.

Solution

Move existing code from complex helper functions to one generalized BundleRemover.

Testing

Existing tests.

chescock

Note that this is likely to have merge conflicts with #17360.

chescock · 2025-05-01T17:02:36Z

+            Option<&mut Table>,
+            &Components,
+            &[ComponentId],
+        ) -> (bool, T),


The old code intentionally used a const generic instead of an ordinary bool to make sure that the branches optimized away. Do we need to preserve that?

Even if we don't, it looks like there are only two functions passed as pre_remove, and one has hard-coded true and the other hard-coded false. It might be simpler to make a separate drop: bool parameter to avoid the tuple and make it clear that it doesn't depend on the values passed to pre_remove.

The old code intentionally used a const generic instead of an ordinary bool to make sure that the branches optimized away. Do we need to preserve that?

It's an impl Fn, and with only two closures types, monomorphization will effectively the same thing. They are both generic, and both bools are constants.

It might be simpler to make a separate drop: bool parameter to avoid the tuple and make it clear that it doesn't depend on the values passed to pre_remove.

I mean, we could add that restriction, but I don't see the point. Letting it depend on it seems more flexible for the future.

chescock · 2025-05-01T17:08:48Z

-                &mut deferred_world,
-                old_archetype,
+        let mut remover = BundleRemover::new::<T>(self.world, self.location.archetype_id, true)?;
+        // SAFETY: The passed location has the sane archetype as the remover, since they came from the same location.


Would it make sense to pass the full EntityLocation to BundleRemover::new instead of just ArchetypeId? That would let you get rid of the safety requirement here.

And do you also need to require that this is the right EntityLocation for the given Entity? Or would that just be a logic bug and not a soundness bug?

Would it make sense to pass the full EntityLocation to BundleRemover::new instead of just ArchetypeId? That would let you get rid of the safety requirement here.

We could but then the remover would be entity specific instead of archetype specific IIUC. So I'd rather leave it as is.

And do you also need to require that this is the right EntityLocation for the given Entity? Or would that just be a logic bug and not a soundness bug?

Only the archetypes need to match for now I think. In principal, we could make remove take just a table row and store the archetype id and row and table id on the remover. But this was just easier IMO. I'm open to changing that though!

chescock · 2025-05-01T17:18:57Z

+pub(crate) struct BundleRemover<'w> {
+    world: UnsafeWorldCell<'w>,
+    bundle_info: ConstNonNull<BundleInfo>,
+    old_and_new_table: Option<(NonNull<Table>, NonNull<Table>)>,


Is it worth trying to make this look more like BundleInserter? That records the current table and archetype pointers along with an enum ArchetypeMoveType that has the new ones.

Although I suppose one major difference is that you don't need to do anything on a remove that doesn't change archetypes, while an insert might be changing the values.

It's been a while since I wrote this, so I don't remember the details. But IIRC, I originally tried to do this with effectively the same structure as BundleInserter and ended up running into issues. I think it was easier to justify safety where accessing the new table mutably could be the same as the old table's mutable pointer. I could take another stab at it, but I don't feel it would be worth it.

Yeah, I agree it's not worth revisiting. I was just surprised that it looked so different! But if you already tried it that way then there must be a good reason for the differences!

chescock · 2025-05-01T17:35:22Z

    }
 }

+// SAFETY: We have exclusive world access so our pointers can't be invalidated externally


This safety comment is on a struct rather than an unsafe block, so I don't think it will get seen. Did you mean to put it somewhere else, or to put it as a doc comment for the struct?

This was just copied from BundleInserter. I don't mind it, but I can understand your point. I wouldn't be opposed to moving it for all Bundle*er maybe in a different pr.

Ah, okay, I didn't realize this was an existing pattern. That makes sense, then!

chescock · 2025-05-01T17:38:45Z

+        require_all: bool,
+    ) -> Option<Self> {
+        let bundle_info = world.bundles.get_unchecked(bundle_id);
+        // SAFETY: Ensured by caller and that intersections are never `None`.


I don't understand this safety comment. What does "and that intersections are never None" refer to?

The safety requirements for remove_bundle_from_archetype are "archetype_id must exist and components in bundle_info must exist". The caller is ensuring that archetype_id exists/is valid. Does the fact that you have a BundleId prove that the components all exist?

Yeah, my comment was poorly written. And yeah, you can't make a bundle without having component ids, so we should be good there.

ElliottjPierce · 2025-05-02T19:11:51Z

I don't know if it's policy or just a pattern I've noticed, but since there's two approvals (thanks!), I'll mark ready for final. Let me know if there's some step I'm missing.

chescock · 2025-05-02T19:57:26Z

I don't know if it's policy or just a pattern I've noticed, but since there's two approvals (thanks!), I'll mark ready for final. Let me know if there's some step I'm missing.

Yup, that's the policy! See https://bevyengine.org/learn/contribute/helping-out/reviewing-pull-requests/#how-pull-requests-are-merged:

Relatively uncontroversial PRs can be merged following approval from at least two community members (including Maintainers) with appropriate expertise.

(Sorry, I probably should have changed the tag when I approved!)

ElliottjPierce · 2025-05-02T20:39:41Z

You're good! I thought I read that somewhere...

# Objective It has long been a todo item in the ecs to create a `BundleRemover` alongside the inserter, spawner, etc. This is an uncontroversial first step of bevyengine#18514. ## Solution Move existing code from complex helper functions to one generalized `BundleRemover`. ## Testing Existing tests.

ElliottjPierce added 5 commits March 24, 2025 18:37

create the remover

ebb7318

use remover

c08527f

Fixed warnings

8e306cf

removed change tick

487cc5d

remove the todos

fcb2033

alice-i-cecile approved these changes Mar 24, 2025

View reviewed changes

alice-i-cecile added this to the 0.17 milestone Mar 24, 2025

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-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Unsafe Touches with unsafe code in some way labels Mar 24, 2025

ElliottjPierce added 2 commits March 24, 2025 21:24

fix doc

0d4f1c7

don't re-index the table

ad78601

ElliottjPierce mentioned this pull request Mar 28, 2025

Making Required Components force existence as a follow-up of #18514 #18566

Open

ElliottjPierce and others added 2 commits April 6, 2025 17:06

Merge branch 'main' into add-bundle-remover

7b79990

fixed bad main merge

c0f6024

chescock reviewed May 1, 2025

View reviewed changes

ElliottjPierce added 3 commits May 2, 2025 13:21

make remove infalible

0fe2b5c

make new unsafe

b207eaf

fix safety comment

0893fdf

chescock approved these changes May 2, 2025

View reviewed changes

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 May 2, 2025

ElliottjPierce mentioned this pull request May 2, 2025

Simplify usage of EntityWorldMut::move_entity_from_remove by making it a proper method #17360

Closed

urben1680 mentioned this pull request May 4, 2025

Track spawn Tick of entities, offer methods, query data SpawnDetails and query filter Spawned #19047

Merged

alice-i-cecile added the X-Uncontroversial This work is generally agreed upon label May 5, 2025

alice-i-cecile added this pull request to the merge queue May 5, 2025

Merged via the queue into bevyengine:main with commit f654350 May 6, 2025

urben1680 mentioned this pull request May 9, 2025

Moving components instead of duplicating should lift the Clone/Reflect requirement #18079

Closed

JaySpruce mentioned this pull request May 11, 2025

bevy_ecs::world::entity_ref::EntityWorldMut::take(), a safe function, has an undocumented unsafe block #17345

Closed

Conversation

ElliottjPierce commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Testing

Uh oh!

chescock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElliottjPierce commented May 2, 2025

Uh oh!

chescock commented May 2, 2025

Uh oh!

ElliottjPierce commented May 2, 2025

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ElliottjPierce commented Mar 24, 2025 •

edited

Loading