Skip to content

ObserverTrigger: Add wrapper type around ComponentId #19755

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.

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

oscar-benderstone
Copy link

@oscar-benderstone oscar-benderstone commented Jun 20, 2025

Objective

Add a newtype to store events, as mentioned in this HackMD.

Solution

  • From the Discord, we agreed on creating a EventKey struct that wraps around ComponentId.
  • I created EventKey and refactored all previous instances of ComponentId in observer related functions.

Testing

  • I tried three examples, whose behavior matches that of main:

    • examples/observer.rs
    • examples/observer_propagation.rs
    • examples/ui/core_widgets.rs
  • There is an issue with examples/observer_propagation.rs, but this is also occurs in main: Warning: missing IDs in observer_propagation #19753.

Copy link
Contributor

Welcome, new contributor!

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

@oscar-benderstone oscar-benderstone changed the title Observer trigger refactor ObserverTrigger: Add wrapper type around ComponentId Jun 20, 2025
@oscar-benderstone oscar-benderstone force-pushed the observer_trigger_refactor branch from 092309b to ea87a3f Compare June 20, 2025 18:31
@oscar-benderstone oscar-benderstone force-pushed the observer_trigger_refactor branch from 9e73ec7 to abc49da Compare June 20, 2025 18:33
@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 C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 20, 2025
@alice-i-cecile alice-i-cecile moved this to Observer overhaul in Alice's Work Planning Jun 20, 2025
@oscar-benderstone oscar-benderstone force-pushed the observer_trigger_refactor branch from 668c8bd to 84eb8df Compare June 20, 2025 19:51
@oscar-benderstone oscar-benderstone marked this pull request as draft June 20, 2025 20:26
@oscar-benderstone oscar-benderstone marked this pull request as ready for review June 20, 2025 21:29
@oscar-benderstone oscar-benderstone force-pushed the observer_trigger_refactor branch from 9b079d4 to 0addb40 Compare June 20, 2025 21:30
@oscar-benderstone oscar-benderstone marked this pull request as draft June 21, 2025 00:40
@oscar-benderstone oscar-benderstone force-pushed the observer_trigger_refactor branch from 1f1040e to fcecaa0 Compare June 21, 2025 00:56
@oscar-benderstone oscar-benderstone marked this pull request as ready for review June 21, 2025 01:35
@alice-i-cecile alice-i-cecile added this to the 0.17 milestone Jun 21, 2025
@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jun 21, 2025
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.

@alice-i-cecile
Copy link
Member

There's an existing observers migration guide; we should add the changes here to that :)

fn register_component_id(world: &mut World) -> ComponentId {
world.register_component::<EventWrapperComponent<Self>>()
/// and should always correspond to the implementation of [`event_type`](Event::event_type).
fn register_event_type(world: &mut World) -> EventKey {
Copy link
Member

Choose a reason for hiding this comment

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

I think that these names would be clearer and more consistent as register_event_key / event_key.

Comment on lines +428 to +437
/// # Warning
///
/// This struct should only be instantiated internally to this crate.
///
/// [`EventKey`]s are created in:
/// - `crate::observer::trigger_dynamic_ref_with_caller`
/// - `crate::observer::trigger_dynamic_ref_with_caller`
/// - `crate::observer::observer_multiple_events`
/// - `crate::observer::observer_dynamic_trigger`
/// - `crate::observer::runner::hook_on_add`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// # Warning
///
/// This struct should only be instantiated internally to this crate.
///
/// [`EventKey`]s are created in:
/// - `crate::observer::trigger_dynamic_ref_with_caller`
/// - `crate::observer::trigger_dynamic_ref_with_caller`
/// - `crate::observer::observer_multiple_events`
/// - `crate::observer::observer_dynamic_trigger`
/// - `crate::observer::runner::hook_on_add`
/// A unique identifier for an [`Event`], used by [observers].
///
/// You can look up the key for your event by calling the [`Event::event_key`] method.
///
/// [observers]: crate::observer

We don't need to give a warning about non-public construction: Rust makes that impossible with correct usage of the visibility system. But we should give some docs about what this type is for and how to get it.


impl EventKey {
/// Returns id of the underlying [`Event`].
/// Used internally in:
Copy link
Member

Choose a reason for hiding this comment

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

This list will be really hard to maintain. Better to leave this off, and allow users to use their IDEs go-to-uses instead.

pub struct EventKey(pub(crate) ComponentId);

impl EventKey {
/// Returns id of the underlying [`Event`].
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Returns id of the underlying [`Event`].
/// Returns the internal [`ComponentId`].

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Some small suggestions for names and docs that we should tackle, but this is looking much better. The clarity of this code is much improved!

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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-Waiting-on-Author The author needs to make changes or address concerns before this can be merged
Projects
Status: Observer overhaul
Development

Successfully merging this pull request may close these issues.

2 participants