Add non-overwriting variants of component/resource insertion functions

[GarettCooper]

As detailed in #2054, this PR adds try_insert, try_insert_bundle, and try_insert_resource functions. These functions act in the same manner as the original functions but instead of overwriting an existing component/resource, silently skip those which already exist.

This is my first bevy contribution and my first contribution to a large open-source rust project, so I appreciate any and all forms of feedback. I'm not completely confident in my decision to make the collision with an existing compoent/resource fail silently, but I couldn't decide how a return value could fit neatly into the command pattern. Additionally, I'm not sure about the usage of a boolean parameter to control the flow of insert_resource_with_id. I'd appreciate f some more experienced members of the team could enlighten me as to how this could be done better/idiomatically.

I thought about the names quite a bit but didn't arrive at any that I was happy enough with to use instead of the try prefix suggested in the original issue by @alice-i-cecile. One alternative I considered was unifying_insert to borrow some language from set theory, but that doesn't really apply to inserting single components or resources. The other was ensure_inserted to communicate that the function doesn't care about the value of the component, just its prescence, but I'm not sure if the meaning of that one is immediately obvious to people other than me.

[alice-i-cecile]

Welcome, and thanks for the contribution! Feel free to ask any questions you may have here or on Discord in #dev-general or #ecs.

@cart could you approve the CI for this?

[alice-i-cecile]

Looks good to me! As a final note for other reviewers, I think fixing the "silently failure" behavior is out of scope for this PR, and should be solved for all commands at once per #2004.

[cart]

This implementation looks good. Three things I want to cover before merging:

1. We should run benchmarks to see how this affects perf. Currently that means using my fork here, but we should really move the relevant benches into this repo to make the process easier on people. For now, I can run the benchmarks and post them here.
2. The try_insert_bundle naming makes this seem like an all-or-nothing operation. The name is nice and short, but we might want to consider more verbose options, especially if we anticipate future insert impls (such as fallible all-or-nothing inserts).
3. We should answer the question: "is this a pattern we want to support/encourage"?

First, the behavior in this pr can already be emulated by just changing insertion order (for cases where bundles are inserted at the same point in time):

// FooBundle: A B
// BarBundle: A C

// Version 1 (oops I don't want Bar::A to overwrite Foo::A!)
world.spawn()
  .insert_bundle(FooBundle::default())  // entity = Foo::A, Foo::B
  .insert_bundle(BarBundle::default())  // entity = Bar::A, Foo::B, Bar::C

// Version 2 (lets use try_insert_bundle)
world.spawn()
  .insert_bundle(FooBundle::default())  // entity = Foo::A, Foo::B
  .try_insert_bundle(BarBundle::default())  // entity = Foo::A, Foo::B, Bar::C

// Version 3 (lets invert order ... note that the outcome is identical to Version 2)
world.spawn()
  .insert_bundle(BarBundle::default())  // entity = Bar::A, Bar::C
  .insert_bundle(FooBundle::default())  // entity = Foo::A, Foo::B, Bar::C

Additionally, this pr enables a few specific situations to play out as desired, but it doesn't enable every situation to play out as desired. If we have XBundle { A, B, C } and YBundle { A, B, C} and we want the final result to be XBundle::A, YBundle::B, XBundle::C, there is no order of insert_bundle / try_insert_bundle calls that will accomplish this outcome.

A more complete solution would be a "bundle builder api", which we have discussed previously, although not in the context of the problem this pr aims to solve. We could do something like this to handle the case above:

world.spawn().insert_bundle(
  XBundle::default()
    .with_bundle(
        YBundle::default()
           .without::<A>()
           .without::<C>()
    )
)
// results in XBundle::A, YBundle::B, XBundle::C

This impl also has the added benefit of not requiring archetype changes (at the cost of making bundle construction heavier).

Alternatively, we could just encourage developers to do this instead, which cuts down on implementation complexity:

let y = YBundle::default();
world.spawn()
  .insert_bundle(XBundle::default())
  .insert(y.b)
)

Additionally, Bundles often set specific component values to work together to produce a specific outcome. In some contexts, they should be considered "transactional". Ex: a CameraBundle might set a projection value with near/far planes and a transform offset that matches those near/far plans. try_insert_bundle makes it easier for people to invalidate these assumptions (and hard to figure out why things aren't working).

[alice-i-cecile]

The try_insert_bundle naming makes this seem like an all-or-nothing operation. The name is nice and short, but we might want to consider more verbose options, especially if we anticipate future insert impls (such as fallible all-or-nothing inserts).

Hmm that's a good choice. Perhaps insert_non_overwriting? I like the idea of keeping the prefix the same so then autocomplete works nicely.

Alternatively, we could just encourage developers to do this instead, which cuts down on implementation complexity:

This is a bit more onerous, and won't work well if a crate exports only the bundle, rather than all of them components. That said, I would argue that exposing a bundle but not the components in your public API is a recipe for headaches in general.

A more complete solution would be a "bundle builder api", which we have discussed previously

Agreed. IMO I think it's better to close this and wrap this into a bundle builder RFC where we consider this whole area more holistically. I'm not keen on slowly exploding this API to allow for all of the desired possible behaviors)

@cart if you're on board with that I'll close this + the issue out and roll it into #792.

(@GarettCooper this was still incredibly valuable to do; your impl really helped us explore this space more thoughtfully and think through exactly what our requirements are.)

[cart]

This is a bit more onerous, and won't work well if a crate exports only the bundle, rather than all of them components. That said, I would argue that exposing a bundle but not the components in your public API is a recipe for headaches in general.

If they haven't exported the components or the fields in the Bundle then imo it is a "private api" and you shouldn't be trying to mess with Bundle content at all, otherwise you risk breaking something. This isn't a case I think we need to care about for "partial bundles".

@cart if you're on board with that I'll close this + the issue out and roll it into #792.

I'll just close it out now.

(@GarettCooper this was still incredibly valuable to do; your impl really helped us explore this space more thoughtfully and think through exactly what our requirements are.)

Yeah thanks for putting this together!