Skip to content

Conversation

@joseph-gio
Copy link
Member

@joseph-gio joseph-gio commented Apr 18, 2023

Objective

Label traits such as ScheduleLabel currently have a major footgun: the trait is implemented for Box<dyn ScheduleLabel>, but the implementation does not function as one would expect since Box<T> is considered to be a distinct type from T. This is because the behavior of the ScheduleLabel trait is specified mainly through blanket implementations, which prevents Box<dyn ScheduleLabel> from being properly special-cased.

Solution

Replace the blanket-implemented behavior with a series of methods defined on ScheduleLabel. This allows us to fully special-case Box<dyn ScheduleLabel> .


Changelog

Fixed a bug where boxed label types (such as Box<dyn ScheduleLabel>) behaved incorrectly when compared with concretely-typed labels.

Migration Guide

The ScheduleLabel trait has been refactored to no longer depend on the traits std::any::Any, bevy_utils::DynEq, and bevy_utils::DynHash. Any manual implementations will need to implement new trait methods in their stead.

impl ScheduleLabel for MyType {
    // Before:
    fn dyn_clone(&self) -> Box<dyn ScheduleLabel> { ... }

    // After:
    fn dyn_clone(&self) -> Box<dyn ScheduleLabel> { ... }

    fn as_dyn_eq(&self) -> &dyn DynEq {
        self
    }

    // No, `mut state: &mut` is not a typo.
    fn dyn_hash(&self, mut state: &mut dyn Hasher) {
        self.hash(&mut state);
        // Hashing the TypeId isn't strictly necessary, but it prevents collisions.
        TypeId::of::<Self>().hash(&mut state);
    }
}

@joseph-gio joseph-gio added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Apr 18, 2023
@alice-i-cecile alice-i-cecile self-requested a review April 18, 2023 15:14
@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Apr 18, 2023
@cart cart added this pull request to the merge queue Apr 19, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 19, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 19, 2023
Merged via the queue into bevyengine:main with commit fe852fd Apr 19, 2023
@joseph-gio joseph-gio deleted the boxed-label-fix branch April 19, 2023 03:07
@Selene-Amanita Selene-Amanita added the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 10, 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-Bug An unexpected or incorrect behavior C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants