fix(ecs): don't resurrect removed required components on insert_if_new

[satyakwok]

Fixes #24554.

insert_if_new was re-adding a required component after it had been removed, when the component requiring it was already present — so the call wasn't the no-op it should've been.

The required-components transition is cached on one archetype edge shared by Replace and Keep, so the target archetype always included the required component (gating just the value init would leave it present-but-uninitialized). I added a separate Keep edge that leaves out required components only pulled in by already-present components. Components the insert actually adds still bring their required ones, so mixed bundles like insert_if_new((A, B)) (A present, B new) still work. Replace is unchanged.

Added tests for the repro, the mixed-bundle case, "new component still gets its required", and the Replace path.

Benchmarks

insert_simple via criterion --baseline, before vs after. This was on a busy shared machine so the numbers are rough:

• insert_simple/base: ~688µs → ~593µs (criterion called it -15%, but that's just run-to-run noise here — the change shouldn't actually make inserts faster)
• insert_simple/unbatched: ~1.11ms → ~1.13ms ("no change in performance detected")

No regression on the insert hot path — picking the keep/replace edge is a single branch on a cached lookup, and the extra set-building only happens once per edge at cold cache-build, not per insert.

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide, as well as our policy regarding AI usage, and we look forward to reviewing your pull request shortly ✨

[alice-i-cecile]

I was promised benchmarks :) Easy to forget; can you please update your PR description to include some rough numbers?

[satyakwok]

Ah you're right, sorry — those got cut when I trimmed the description down. Added them back.

Short version: insert_simple/unbatched shows no change, and base measured ~15% faster but that's just noise on my machine (the change shouldn't actually speed inserts up). No regression on the hot path.

[chescock]

The change makes sense, but I'd like to understand and document the intended behavior on the edge cases.

Oh, and I guess we should document the behavior on the normal cases, too :). They don't currently say anything about how required components and insert_if_new interact. Maybe add something to the Component docs in the "Required Components" section? Or on insert_if_new?

Do we need a migration guide for this? It's a behavior change to existing APIs.

[chescock]

Why use bool here instead of InsertMode? The custom enum is a little more self-documenting.

[satyakwok]

Yeah, good call — I'll thread InsertMode through instead of the bool, reads better.

[chescock]

This contains will do an O(N) scan. Do we need to worry about a quadratic blowup here?

[satyakwok]

Fair point. This one actually goes away with the shared-constructor fix below — I'll switch required_by_added to a map (required id -> constructor), so the lookup is O(1) instead of the linear scan.

[chescock]

Minor nit, but I don't think we usually include links to resolved issues in comments (here and below).

Suggested change

	// pulled in only by already-present components (issue #24554).
	// pulled in only by already-present components.

[satyakwok]

Agreed, I'll drop the issue-link references from the comments here and below.

[chescock]

I think there is an edge case here if we have a required component with different constructors.

The Bundle will store the constructor used by the first requiree in the bundle. But if we want the semantics of insert_if_new to be the same as if existing components were absent from the bundle, then we might need a constructor from a later requiree.

That is, if I modify your test below to have them share a required component:

#[derive(Component)]
struct Req(usize);

#[derive(Component)]
#[require(Req(1))]
struct A;

#[derive(Component)]
#[require(Req(2))]
struct B;

let mut world = World::new();
let id = world.spawn(A).id();
world.entity_mut(id).remove::<Req>();
world.entity_mut(id).insert_if_new((A, B));

What value should Req have? I would expect the semantics to be "A already exists, so this is the same as insert(B), which requires Req(2)". But I think this implementation says "(A, B) requires Req(1), and B requires Req, so we include Req(1)".

If we want the other semantics, then I think you need to accumulate the constructors in an IndexMap like in BundleInfo::new.

[satyakwok]

Good catch — you're right, the current code takes the first requiree's constructor even when that requiree isn't the one being added. The fix ties into the recursive question below: a per-edge DFS would pick the constructor from the added chain for free; if we keep the current non-DFS approach, I'd accumulate constructors in an IndexMap from the added requirees. Either way insert_if_new((A, B)) with A already present should end up with Req(2).

[chescock]

Hmm, do we also want the new behavior for recursively required components? They currently behave more-or-less the same as insert_if_new, and a difference might be surprising.

That is, suppose you have:

#[derive(Component)]
#[require(B)]
struct A;

#[derive(Component)]
#[require(C)]
struct B;

#[derive(Component)]
struct C;

let mut world = World::new();
let id = world.spawn(B).id();
world.entity_mut(id).remove::<C>();
world.entity_mut(id).insert_if_new(A);

Then insert_if_new(A) will insert C because A recursively requires it through B, even though insert_if_new(B) would not have.

If we want to change that behavior, too, then I think we need to do the full depth-first search here and give up the RequiredComponents::all and BundleInfo::required_component_constructors caches completely. But maybe that's not so bad if we're caching everything on archetype edges anyway?

[satyakwok]

Good question — and I think it has the same root as the constructor case. A per-edge depth-first walk from the explicit components, stopping at components already in the source archetype, would handle both: it only pulls in required components reachable through a newly-added requirer (so insert_if_new(A) with B present wouldn't add C), and it naturally picks the constructor from the added chain.

Since this is built once per archetype edge and cached, the DFS would be cold-path only — the per-insert hot path doesn't change. So I'd lean toward doing the full DFS here and dropping the all / required_component_constructors shortcuts for this path. Want me to go that way, or keep the current behavior and document the recursive case as a known difference?

[chescock]

Want me to go that way, or keep the current behavior and document the recursive case as a known difference?

Either seems reasonable! I wanted to make sure we thought through the edge cases, but I don't have strong opinions on how they should work, and I don't want to cause any unnecessary scope creep. As long as there's an explanation of why you took the approach you did then I'm happy to approve as a community reviewer! The final decision is made by a SME, and they'll do a final review once there are two community approvals.

And if you want more opinions, you're always welcome to hop on the Bevy Discord! The #ecs-dev channel is a good place for building consensus on how ECS features like this ought to work.

[satyakwok]

Thanks! I'll go with the smaller change then — keep the current behavior for the recursive case and document it, rather than dropping the caches for a full DFS. That avoids the scope creep and keeps the per-edge cache.

I'll still fix the shared-constructor case (D), since that one's a genuine wrong value rather than a judgment call: I'll accumulate the constructors from the requirees that are actually being added (so insert_if_new((A, B)) with A present ends up with B's Req). And I'll write up the reasoning and the recursive-edge behavior in the docs.

[satyakwok]

Thanks for the thorough review. Agreed on the docs — I'll write up how required components and insert_if_new interact (normal and edge cases), probably in the Required Components section of the Component docs, plus a migration guide entry since it's a behavior change. I'll settle the edge cases above first so the docs match the final behavior.

[satyakwok]

Pushed the changes:

• keep: bool -> InsertMode through the edge build (A)
• a shared required component now takes the constructor from a requiree that's actually being added, looked up via a map that's also O(1) instead of the linear scan (D + B)
• dropped the resolved-issue links from the comments (C)
• kept the recursive behavior and documented the insert_if_new / required-components interaction in the Component "Required Components" docs, plus a migration guide for the behavior change (E + docs)

Added a regression test for the shared-constructor case. All bevy_ecs tests and doctests pass.