FromReflect should be able to do conversions

[Person-93]

I found it somewhat surprising that the FromReflect implementation for Option<PathBuf> could not take a String and convert it. After digging around a bit, it seems that there is no attempt to perform any conversion at all, and I could not see any obvious way to make it convert.

I propose that the FromReflect derive macro get a new argument to the attribute helper to tell it to try using the From trait (or a user-provided conversion).

It would look something like this:

#[derive(Reflect)]
#[reflect(from(String, i32))] // list of types to use `From` trait
#[reflect(from(i16(|i| Player::from(i as i32)))] // accept user-provided conversion function
struct Player {
    tag: String,
}

fn main() {
    let mut player = DynamicStruct::default();
    player.insert("tag", 42);
    let player = Player::from_reflect(&player).unwrap();
    assert_eq!(player.tag, "42");
}

impl From<usize> for Player {
    fn from(tag: usize) -> Self {
        Self::from(tag.to_string())
    }
}

impl From<String> for Player {
    fn from(tag: String) -> Self {
        Player { tag }
    }
}

With this in place, the implementations of FromReflect that are provided by bevy itself for types from std and other crates can take advantage of the existing conversions to be more robust.

The implementation for Option<T> can now be updated to accept an instance of T . One possible syntax for that can be #[reflect(from(<T>))] to indicate that T is a generic param of the type. I further propose adding a Null type to the crate as a placeholder for Option::None.

// proposed new struct
pub struct Null;

impl PartialReflect for Null {
    // ... snip ...
}

impl_reflect! {
    #[reflect(from(<T>, Null(|| None)))] // proposed change to `FromReflect` for `Option`
    #[type_path = "core::option"]
    enum Option<T> {
        None,
        Some(T),
    }
}

[MrGVSV]

I like it!

Your first example seems to be a little off though since the conversion actually happens on tag and not player. But I think that is a good example of why we should also allow for #[reflect(from(...))] on fields.

Just a few more comments:

• As I said, this would also be really nice to support on individual fields. Especially since I don't think we should provide any default from behavior for the primitives (at least to start with).
• I think we should require each from contain a single type. That helps remove visual confusion with tuples and leaves room to provide additional options for a type if we ever decide to do that (e.g. validation functions, etc.).
• I don't think we need special syntax for generics. The generated code should have no issue with treating them as any other type
• To keep things focused, I would avoid adding a new helper type like Null. That can be done in a separate PR

[Person-93]

Your first example seems to be a little off though since the conversion actually happens on tag and not player. But I think that is a good example of why we should also allow for #[reflect(from(...))] on fields.

Part of my intent with the example was to allow something like:

#[derive(Reflect)]
struct Team {
    captain: Player,

    #[reflect(default)]
    lieutenant: Option<Player>,

    #[reflect(default)]
    members: Vec<Player>,
}

Then this would use the conversions defined on the Player struct.

As I said, this would also be really nice to support on individual fields. Especially since I don't think we should provide any default from behavior for the primitives (at least to start with).

This is a great idea! Although, I do hope to see some from behavior added at some point. If not then at least a shorthand for adding a standard set of conversions. For something like this example, it'd be nice not to have to define all those conversions multiple times, but the details for that can be fleshed out later.

#[derive(Reflect)]
struct Workspace {
    #[reflect(default)]
    #[reflect(from(PathBuf))]
    #[reflect(from(String))]
    #[reflect(from(&str))]
    #[reflect(from(&Path))]
    save_file: Option<PathBuf>,

    #[reflect(default)]
    #[reflect(from(PathBuf))]
    #[reflect(from(String))]
    #[reflect(from(&str))]
    #[reflect(from(&Path))]
    auto_save_dir: Option<PathBuf>,

    #[reflect(default)]
    #[reflect(from(PathBuf))]
    #[reflect(from(String))]
    #[reflect(from(&str))]
    #[reflect(from(&Path))]
    config_file: Option<PathBuf>,
}

I think we should require each from contain a single type. That helps remove visual confusion with tuples and leaves room to provide additional options for a type if we ever decide to do that (e.g. validation functions, etc.).

Good idea.

To keep things focused, I would avoid adding a new helper type like Null. That can be done in a separate PR

We can put a pin in this for now.

[Person-93]

I think we may also need to update the derived implementations of PartialReflect::try_apply to use the conversions as well.