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

Add trivial Event trait #4685

Closed
wants to merge 2 commits into from

Conversation

alice-i-cecile
Copy link
Member

Objective

  • The trait bounds on e.g. EventWriter<T> are currently confusing to users: why does it matter if the underlying type is a resource?
  • This is actually misleading: the type T is never stored as a resource, instead, Events<T> is inserted as a resource.
  • However, because of the blanket implementation of Resource, any Events<T> will be Send + Sync + 'static if and only if T is.
  • Down the line, if we want to make either Event or Resource non-trivial, this coupling will be confusing and error-prone.

Solution

  • Add a trivial Event: Send + Sync + 'static trait, and use this in place of Resource in the assorted event types.
  • Create a blanket impl for Event so users do not need to manually implement or derive this trait (yet).

Changelog

  • Events<T>, EventWriter<T>, EventReader<T> and so on now require that the underlying type is Event, rather than Resource. Both of these are trivial supertraits of Send + Sync + 'static with universal blanket implementations: this change is currently purely cosmetic.

Context

This was first discussed in the context of the more controversial #2073.

You can see the confusion that this sort of conflation generates in #4680 (comment), where the changes made to the Resource trait echo through to our event types in surprising ways.

Down the line, there's an argument to be made for forcing a derive macro on both Resource and Event, and supporting more configuration, but that's a conversation for another day.

@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 labels May 7, 2022
@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label May 7, 2022
@alice-i-cecile alice-i-cecile removed the S-Needs-Triage This issue needs to be labelled label May 7, 2022
@mockersf
Copy link
Member

mockersf commented May 7, 2022

I updated #3863 (which as far as I know shouldn't have anything controversial) with the doc and type renamed from this PR. If you prefer to go with this one, you should also change in bevy_app:

pub fn add_event<T>(&mut self) -> &mut Self
where
T: Resource,

@alice-i-cecile
Copy link
Member Author

Closing in favor of #3863.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants