Skip to content

Split observer events #20151

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 18 commits into
base: main
Choose a base branch
from

Conversation

tim-blackbird
Copy link
Contributor

@tim-blackbird tim-blackbird commented Jul 15, 2025

Objective

  • Split the two types of observer usages - trigger and trigger_targets - by trait

Solution

Check out the diff of the release notes.

Notes

  • It's not possible to derive BroadcastEvent and EntityEvent together, one must be implemented manually if you want both. This is probably okay as having both implemented is somewhat undesirable.
  • Planned follow-up to this PR is to rename Event to ObserverEvent.

@tim-blackbird tim-blackbird added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jul 15, 2025
@alice-i-cecile
Copy link
Member

What to do with the SceneInstanceReady event which is the only event in Bevy to use both trigger and trigger_targets.

We can replace the untargeted form with a buffered event IMO.

@alice-i-cecile alice-i-cecile added this to the 0.17 milestone Jul 15, 2025
@tim-blackbird tim-blackbird marked this pull request as ready for review July 17, 2025 13:45
@@ -1,4 +1,20 @@
//! Event handling types.
//! Events are things that "happen" and can be processed by app logic.
Copy link
Member

Choose a reason for hiding this comment

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

Really excellent docs!

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.

Very nice. I'm pleased with how simple the diff is. I'm increasingly fond of this as the final form: this should give us 99% of the type-safety benefits without any complexity around mutually exclusive traits or a On variant for broadcast events.

Well, at least after Event -> ObserverEvent is done :)

@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR S-Needs-SME Decision or review from an SME is required and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jul 17, 2025
@alice-i-cecile
Copy link
Member

I spent some time today writing up a focused little design doc for the ultimate split here: https://hackmd.io/@alice-i-cecile/SyuIWJPLxg

I don't particularly care whether we do it in a single PR or across multiple, as long as we converge to our ideal form before we ship 0.17.

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 D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-SME Decision or review from an SME is required X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants