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

bevy_ecs::event::EventId should manually implement Eq, PartialEq, Ord, PartialOrd, and Hash #8528

Closed
Themayu opened this issue May 2, 2023 · 1 comment · Fixed by #8529
Closed
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior D-Trivial Nice and easy! A great choice to get started with Bevy

Comments

@Themayu
Copy link
Contributor

Themayu commented May 2, 2023

Currently, bevy_ecs::event::EventId makes use of derive macros for its implementations of a set of traits:

#[derive(Eq, PartialEq, Ord, PartialOrd, Hash)]
pub struct EventId<E: Event> {
    pub id: usize,
    _marker: PhantomData<E>,
}

This creates an issue when attempting to actually use these implementations (especially that of Hash), as it requires them to be implemented on the events themselves; despite the generic only being used in a marker type.

It would be better to manually implement these traits to ignore the marker, freeing them up to be used in more places than they are currently:

error[E0599]: the method `insert` exists for struct `Local<'_, HashSet<EventId<MouseButtonInput>>>`, but its trait bounds were not satisfied
   --> src\interaction.rs:327:54
    |
327 |  if !old_events.contains(&id) && processed_events.insert(id) {
    |                                                   ^^^^^^
    |
   ::: D:\Programs\Rust\cargo\registry\src\github.com-1ecc6299db9ec823\bevy_ecs-0.10.0\src\event.rs:20:1
    |
20  | pub struct EventId<E: Event> {
    | ---------------------------- doesn't satisfy `bevy::ecs::event::EventId<MouseButtonInput>: Hash`
    |
    = note: the following trait bounds were not satisfied:
            `bevy::ecs::event::EventId<MouseButtonInput>: Hash`

In the above error message, I am unable to insert an EventId object into a HashSet as MouseButtonInput does not implement Hash. As MouseButtonInput is only being used in a marker, I don't believe it should be relied upon in the Hash implementation; however the derive macro is not smart enough to know this. For this reason, I believe the traits should be implemented manually.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior D-Trivial Nice and easy! A great choice to get started with Bevy A-ECS Entities, components, systems, and events labels May 2, 2023
@alice-i-cecile
Copy link
Member

Agreed, good analysis. I'd be happy to review a PR to fix this.

konsti219 added a commit to konsti219/bevy that referenced this issue May 2, 2023
# Objective
Fixes bevyengine#8528

## Solution
Manually implement `PartialEq`, `Eq`, `PartialOrd`, `Ord`, and `Hash` for `bevy_ecs::event::EventId`.
These new implementations do not rely on the `Event` implementing the same traits allowing `EventId` to be used in more cases.
konsti219 added a commit to konsti219/bevy that referenced this issue May 3, 2023
# Objective
Fixes bevyengine#8528

## Solution
Manually implement `PartialEq`, `Eq`, `PartialOrd`, `Ord`, and `Hash` for `bevy_ecs::event::EventId`.
These new implementations do not rely on the `Event` implementing the same traits allowing `EventId` to be used in more cases.
konsti219 added a commit to konsti219/bevy that referenced this issue May 3, 2023
# Objective
Fixes bevyengine#8528

## Solution
Manually implement `PartialEq`, `Eq`, `PartialOrd`, `Ord`, and `Hash` for `bevy_ecs::event::EventId`.
These new implementations do not rely on the `Event` implementing the same traits allowing `EventId` to be used in more cases.
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-Bug An unexpected or incorrect behavior D-Trivial Nice and easy! A great choice to get started with Bevy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants