Skip to content
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

Require #[derive(Event)] on all Events #7086

Merged
merged 19 commits into from
Jun 6, 2023

Conversation

CatThingy
Copy link
Contributor

Objective

Be consistent with Resources and Components and have Event types be more self-documenting.
Although not susceptible to accidentally using a function instead of a value due to Events only being initialized by their type, much of the same reasoning for removing the blanket impl on Resource also applies here.

  • Not immediately obvious if a type is intended to be an event
  • Prevent invisible conflicts if the same third-party or primitive types are used as events
  • Allows for further extensions (e.g. opt-in warning for missed events)

Solution

Remove the blanket impl for the Event trait. Add a derive macro for it.


Changelog

  • Event is no longer implemented for all applicable types. Add the #[derive(Event)] macro for events.

Migration Guide

  • Add the #[derive(Event)] macro for events. Third-party types used as events should be wrapped in a newtype.

@james7132 james7132 added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR labels Jan 4, 2023
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

Tentatively approving this on the same grounds as #[derive(Resource)]. This should provide solid ground for compile time optimization in the future for events (i.e. garbage collection policy).

Marking this as controversial as this definitely adds friction when integrating with third party crates.

@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 4, 2023
@CatThingy CatThingy force-pushed the manual-impl-event branch 2 times, most recently from 4a6c1af to 32ead94 Compare January 12, 2023 21:30
@james7132 james7132 requested a review from cart January 16, 2023 20:53
@CatThingy CatThingy force-pushed the manual-impl-event branch 3 times, most recently from c8a6d2a to e6f192d Compare February 6, 2023 17:01
@nicopap nicopap added this to the 0.11 milestone Mar 7, 2023
@CatThingy
Copy link
Contributor Author

Closing as this does not improve the developer experience in isolation.

@CatThingy CatThingy closed this Mar 7, 2023
@james7132 james7132 removed this from the 0.11 milestone Mar 7, 2023
@nicopap
Copy link
Contributor

nicopap commented Mar 7, 2023

I don't agree. Admittedly, it was only once, but this feature would have saved me an hour of debugging when I mistakenly used the wrong type in an EventReader.

@JoJoJet
Copy link
Member

JoJoJet commented Mar 9, 2023

@CatThingy can you consider re-opening this? Most people seem to think this PR is a good idea.

@CatThingy CatThingy reopened this Mar 10, 2023
@JoJoJet JoJoJet added this to the 0.11 milestone Mar 10, 2023
@targrub
Copy link
Contributor

targrub commented Mar 10, 2023

Doesn't this need documentation changes? Migration instructions?

@alice-i-cecile
Copy link
Member

Doesn't this need documentation changes? Migration instructions?

Documentation changes I actually don't think are needed: the blog post will contain a blurb and the examples will all demonstrate the pattern.

Migration guide is done: it's just adding a derive and newtyping when needed. People are used to this with components and resources already.

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

Looks good to me. Consistent with our other ECS apis / traits and paves the way for static event configuration (which we almost certainly want).

@alice-i-cecile
Copy link
Member

@CatThingy can you get this passing CI? Once that's done we'll finally merge this in :)

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 6, 2023
Merged via the queue into bevyengine:main with commit 89cbc78 Jun 6, 2023
@Selene-Amanita Selene-Amanita added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 9, 2023
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-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-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it 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.