feat(bevy_ecs): Allow attaching/spawning observers as a Bundle via Bundle Effects

[DylanRJohnston]

Objective

• This upstreams one design for the common community pattern of using Bundle Effects to spawn observers inline
• Fixes Observers as part of bundle #14204
• BundleEffectFn can be used to solve Bundles with optional components #2157
• Potentially conflicts with Migrate Observers to use Relationships #17607

Solution

Introduced for SpawnIter and SpawnWith, Bundle Effects are a powerful tool for manipulating the ECS world when a bundle is inserted. This is currently used in the aforementioned children spawning helper methods and has also be extensively used by the community for spawning observers inline. This helps simplify building complex UI by allowing the use of React style components of fn(props) -> impl Bundle. Without it, the relevant Bundle must be spawned so its entity ID can be obtained and then the observer added.

Testing

• Several people including me made heavy use of this pattern during the most recent Bevy Jam 6 and it's a widely spread approach through the community with it being referenced a lot on Discord.
• I'm unclear if the unsafe impl of Bundle is correct.

---

Showcase

Example Usage for a tooltip bundle that can be mixed into other UI nodes

pub fn tooltip(text: impl Into<String>) -> impl Bundle {
    (
        Observer::bundle(tooltip_hover_start(text.into())),
        Observer::bundle(tooltip_hover),
        Observer::bundle(tooltip_hover_end),
        Observer::bundle(tooltip_touch_end),
    )
}

#[derive(Debug, Clone, Copy, Component)]
pub struct Tooltip;

fn tooltip_bundle(text: String, position: Vec2) -> impl Bundle {
    (
        Tooltip,
        Node {
            padding: UiRect::all(Val::Px(8.0)),
            top: Val::Px(position.y),
            left: Val::Px(position.x),
            ..default()
        },
        GlobalZIndex(100),
        Pickable::IGNORE,
        BackgroundColor(Color::BLACK.with_alpha(0.8)),
        BorderRadius::all(Val::Px(8.0)),
        children![(
            Text::new(text),
            TextFont::from_font_size(18.0),
            Pickable::IGNORE
        )],
    )
}

fn tooltip_hover_start(text: String) -> impl Fn(Trigger<Pointer<Over>>, Commands, Res<UiScale>) {
    move |trigger, mut commands, ui_scale| {
        commands.spawn(tooltip_bundle(
            text.clone(),
            trigger.pointer_location.position / **ui_scale,
        ));
    }
}

fn tooltip_hover(
    trigger: Trigger<Pointer<Move>>,
    mut tooltips: Query<&mut Node, With<Tooltip>>,
    ui_scale: Res<UiScale>,
) {
    tooltips.iter_mut().for_each(|mut tooltip| {
        tooltip.top = Val::Px(trigger.pointer_location.position.y / **ui_scale);
        tooltip.left = Val::Px(trigger.pointer_location.position.x / **ui_scale);
    });
}

fn tooltip_hover_end(
    _: Trigger<Pointer<Out>>,
    tooltips: Query<Entity, With<Tooltip>>,
    mut commands: Commands,
) {
    tooltips
        .iter()
        .for_each(|tooltip| commands.entity(tooltip).despawn());
}

fn tooltip_touch_end(
    _: Trigger<Pointer<Click>>,
    tooltips: Query<Entity, With<Tooltip>>,
    mut commands: Commands,
) {
    tooltips
        .iter()
        .for_each(|tooltip| commands.entity(tooltip).despawn());
}

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[github-actions]

It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note.

Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes.

[alice-i-cecile]

Wow, very cool.

Requests:

1. Add this to the observer_overhaul.md release note.
2. Add this to the docs for Observer and the observer module.
3. Mention Observer::bundle in the BundleEffectFn docs.
4. Demo / explain in Observer::bundle why this is useful.

[alice-i-cecile]

@SkiFire13 can I get your eyes on the unsafe bits here?

[SkiFire13]

The unsafe bits looks fine to me, as the safety requirements are trivially satisfied because the bundle includes no components and hence never calls the closures given to it.

However I'd like to see a more flexible version of BundleEffectFn. Currently it is only usable with closures, meaning its type ends up being unnameable and you won't be able to include it in e.g. structs deriving Bundle (although that currently doesn't support effects, but it should be a trivial change in the derive macro)

[alice-i-cecile]

I think this advice needs workshopping. You can do structural changes inside of hooks. The real tradeoff to me IMO is "always needed" vs "optionally needed" behavior. Perf might also be impacted, but I would really really want to see benchmarks before comparing.

[alice-i-cecile]

I don't think we should demo this in a doc test unless we can do it right.

[alice-i-cecile]

This doc test is going to need imports to pass CI :)

[alice-i-cecile]

I don't like this name: it leaks too many implementation details and implies that the observer component is added to the same entity. Something like Observe::watch_this (uh, please bikeshed) would much more clearly communicate the intent in this code snippet.

[greym0uth]

Would this have any overlap with an On(|trigger: Trigger<...>| {}) BundleEffect like the on in the BSN prototype (https://github.com/bevyengine/bevy_editor_prototypes/blob/5dcfd5e19177f236379147f855383bfc75f761f1/crates/bevy_proto_bsn/src/bsn_helpers.rs#L41-L77)? I, personally, feel like this is a little weird to have implemented on the Observer struct and maybe would make more sense as it's own thing.

[chescock]

Is Send + Sync + 'static necessary here? They aren't required by the BundleEffect trait itself.

Suggested change

	E: FnOnce(&mut EntityWorldMut) + Send + Sync + 'static,
	E: FnOnce(&mut EntityWorldMut),

[chescock]

The DynamicBundle impl uses the general BundleEffect while this uses the more specific FnOnce bound. They should probably be the same.

Suggested change

	F: FnOnce(&mut EntityWorldMut) + Send + Sync + 'static,
	F: BundleEffect + Send + Sync + 'static,

Note that SpawnRelatedBundle and SpawnOneRelated have type Effect = Self, which means this will make Patch(Children::spawn_one(things)) compile. I think that's harmless, though?

[cart]

I see the appeal and value of this PR, but it sadly violates some "key principles" that I would like to retain in the Bundle API:

1. Things that are not components on entities should not appear to be components on entities. Put another way: bundles should always be thought of as collections of components added to the current entity. The Observer being added in the example above is in the "component position". Someone without intimate knowledge of the inner working of this would very likely assume that they are adding an Observer component to the entity in the Bundle. This is not what is happening. By enabling this sugar, we are making the system significantly harder to reason about. Notably, the Children::spawn() API (which motivated the bundle effect API) returns a bundle that inserts the Children component on the entity, so it does not violate that principle.
2. BundleEffects should be written by people who are more likely to enforce (1): In Improved Spawn APIs and Bundle Effects #17521 I intentionally did not add sugar to make it easier for users to define BundleEffects for custom bundles because it would be easy (and desirable) for the community to violate this principle. By adding Patch, we're encouraging the community violate this principle (which they will want to do, as evidenced by this PR).

Again, I understand why people want this. But this is attempting to turn Bundle into an "arbitrary templating abstraction", which blurs the conceptual model (specifically, how hierarchies are formed) in a way that I think will be detrimental to peoples' understanding of the system. I'm even shooting for BSN, which is Bevy's planned "arbitrary templating abstraction", to do its best to keep the "perceived data model" intact.

For this specific case, we already have plans for how to make observers better:

1. Make inline-component-driven observers viable by implying observer targets via relationships:

commands.spawn((
    Node::default(),
    Text::from("Not clicked"),
    // This adds the Observers component to the current entity and spawns the given Observers
    // as related entities
    observers![|trigger: On<Pointer<Click>>, mut text: Query<&mut Text>| {
         **text.get_mut(trigger.target).unwrap() = "Clicked!".to_string();
    }]
))

This builds on Bevy's existing data model without introducing ambiguities or confusion.

2. Use BSN to further improve this experience:

bsn! {
    Node,
    Text("Not clicked"),
    Observers [|trigger: On<Pointer<Click>>, mut text: Query<&mut Text>| {
         **text.get_mut(trigger.target).unwrap() = "Clicked!".to_string();
    }]
}

And if for some reason we decide that we want inline "arbitrary effect templates" (such as "spawn an observer and relate it to the current entity), I think we should use a specific syntax in BSN to ensure the integrity of the conceptual model:

bsn! {
    Node,
    Text("Not clicked"),
    @On(|trigger: Trigger<Pointer<Click>>, mut text: Query<&mut Text>| {
      **text.get_mut(trigger.target).unwrap() = "Clicked!".to_string();
    })
}

[cart]

Once BSN lands part of me wants to remove BundleEffect entirely in favor of BSN templates (in the interest of preserving the principles of the system / preventing the community from accidentally violating them). However being able to define hierarchies without BSN templates is nice, so I'm not currently planning of advocating for that.

[cart]

Closing this based on my comment here: #19613 (comment)