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_reflect: Allow parameters to be passed to type data #13723

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented Jun 6, 2024

Objective

Addresses comments regarding #7317 (note that this doesn't replace #7317, there are still some great improvements there besides this syntactical problem).

There currently exist some "special" type data registrations that can be registered like other type data (e.g. #[reflect(Hash)]) or can use a "special" syntax to allow specifying custom implementations (e.g. #[reflect(Hash(custom_hash_fn))]). And there may be more to follow (#13432).

What's interesting is that most of these special cased registrations don't actually come with any type data type. Instead, they simply modify methods on Reflect (e.g. Reflect::reflect_hash).

#7317 sought to distinguish between these "special" registrations by making them lowercase and use a more conventional attribute style: #[reflect(hash = "custom_hash_fn")].

However, while this did help distinguish these registrations and make them a bit prettier, they now require the user to actually know which traits are "special" and which are not (as pointed out here).

Ideally, users shouldn't have to know which traits are "special" until they need to. For most users, they should just know that they need to register their trait in order for certain things to work. And the special-casing may be easier to follow if we open up the configuration abilities to all type data.

Solution

This PR introduces CreateTypeData which replaces FromType. This was done for two reasons.

Firstly, FromType isn't very descriptive as to what it should be used for. We are creating type data from a type, but it's not immediately clear this is even for type data. Renaming to CreateTypeData should hopefully make this much clearer.

Secondly, in order to support type data with parameters like the custom_hash_fn in reflect(Hash(custom_hash_fn)), an additional Input type parameter had to be added. This makes the new signature CreateTypeData<T, Input = ()>.

We can now create type data that accepts input!

trait Combine {
  fn combine(a: f32, b: f32) -> f32;
}

#[derive(Clone)]
struct ReflectCombine {
  multiplier: f32,
  additional: f32,
  combine: fn(f32, f32) -> f32,
}

impl ReflectCombine {
  pub fn combine(&self, a: f32, b: f32) -> f32 {
    let combined = (self.combine)(a, b);
    let multiplied = self.multiplier * combined;
    multiplied + self.additional
  }
}

impl<T: Combine + Reflect> CreateTypeData<T, (f32, f32)> for ReflectCombine {
  fn create_type_data(input: (f32, f32)) -> Self {
    Self {
      multiplier: input.0,
      additional: input.1,
      combine: T::combine,
    }
  }
}

And then register them with the special function-like syntax:

#[derive(Reflect)]
#[reflect(Combine(2.0, 4.0))]
struct Foo;

The above code will compile into the following registration:

registration.insert(<ReflectCombine as CreateTypeData<Self, _>>::create_type_data((2.0, 4.0)))

Notice how the macro automatically generates the tuple for us, so we don't have to add an additional layer of parentheses.

Multiple Input Types

You might be wondering why we're using a type parameter instead of an associated type to specify the input type.

An associated type would limit us to a single implementation. This means that if we want to support the type data with optional parameters (e.g. support both Hash and Hash(custom_hash_fn)), then all type data must take in Option<Self::Input>, regardless of whether or not a None case is supported.

This is important because the macro has to be pass in something, whether that be () or None.

By using a type parameter we open the door to type data with required input:

// `ReflectMyTrait` must be registered with input
impl<T> CreateTypeData<T, u32> for ReflectMyTrait {
  fn create_type_data(input: u32) -> Self {
    Self {
      value: input,
    }
  }
}

// And we can support all different input types
impl<T> CreateTypeData<T, i32> for ReflectMyTrait {
  fn create_type_data(input: i32) -> Self {
    Self {
      value: input.abs() as u32,
    }
  }
}

However, this may be something we don't necessarily care about since users could also get away with this using custom input enums. And the required-input case could be deferred until runtime (i.e. maybe a panic in the None case).

Adding ReflectPartialEq and ReflectHash

I had originally considered adding ReflectPartialEq and ReflectHash type data to further decrease the differences between the "special" registrations and the regular ones. However, I chose not to do that to (1) reduce the complexity of this PR and (2) we may end up removing these entirely due to #8695.

What else is this good for?

Another question you might have is what else this is good for beyond just making things a bit more consistent.

I'm not sure exactly how the community will use it, but I can see it being used for things like feature gating certain functionality:

#[derive(Reflect)]
#[cfg_attr(feature = "debug", reflect(MyTrait(true)))]
#[cfg_attr(not(feature = "debug"), reflect(MyTrait(false)))]
struct Foo;

Or to emulate specialization via reflection:

impl<T> DoSomething for T {
  fn do_something(&self) {
    println!("Doing the same old stuff.");
  }
}

#[derive(Reflect)]
#[reflect(ReflectDoSomething(|_| {
  println!("Doing something special!");
}))]
struct Foo;

Note that all of the above could always be done with manual registration. However, due to them requiring input, some cases could only be done with manual registration.

This PR mainly opens the door to doing more of this interesting stuff with type data via the macro registration. It not only unifies "special" and regular registrations, but also manual and automatic registrations.

Testing

The tests for this feature are split into doctests (for the docs on CreateTypeData) and in the compile-fail tests.

These will both be verified automatically by CI.


Changelog

  • Replaced FromType<T> with CreateTypeData<T, Input = ()>
  • Type data may now opt-in to accepting input during creation using the #[reflect(MyTrait(...))] syntax
  • Added TypeRegistry::register_type_data_with method

Migration Guide

FromType<T> has been replaced by CreateTypeData<T, Input = ()>. Implementors of FromType<T> will need to update their implementation:

// BEFORE
impl<T> FromType<T> for ReflectMyTrait {
  fn from_type() -> Self {
    // ...
  }
}

// AFTER
impl<T> CreateTypeData<T> for ReflectMyTrait {
  fn create_type_data(input: ()) -> Self {
    // ...
  }
}

Additionally, any calls made to FromType::from_type will need to be updated as well:

// BEFORE
<ReflectMyTrait as FromType<Foo>>::from_type()

// AFTER
<ReflectMyTrait as CreateTypeData<Foo>>::create_type_data(())

@MrGVSV MrGVSV added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels Jun 6, 2024
@MrGVSV MrGVSV added this to the 0.15 milestone Jun 10, 2024
@janhohenheim janhohenheim added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Jul 17, 2024
///
/// This is used by the `#[derive(Reflect)]` macro to generate an implementation
/// of [`TypeData`] to pass to [`TypeRegistration::insert`].
pub trait FromType<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it's desirable to keep FromType and have a generic implementation like

impl <T, D> CreateTypeData<T, ()> for D where D: FromType<T> + TypeData {
    fn create_type_data(_input: ()) -> Self {
        D::from_type()
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh and mark it as deprecated? That's probably better than flat out removing it 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, this is an important trait after all, maybe deprecating it for now is better.

@alice-i-cecile alice-i-cecile removed this from the 0.15 milestone Oct 8, 2024
@pablo-lua pablo-lua added this to the 0.16 milestone Oct 19, 2024
@MrGVSV MrGVSV mentioned this pull request Oct 24, 2024
@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged
Projects
Status: Respond (With Priority)
Status: In Progress
Development

Successfully merging this pull request may close these issues.

6 participants