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: Add Function trait #15205

Merged
merged 7 commits into from
Sep 22, 2024

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented Sep 14, 2024

Objective

While #13152 added function reflection, it didn't really make functions reflectable. Instead, it made it so that they can be called with reflected arguments and return reflected data. But functions themselves cannot be reflected.

In other words, we can't go from DynamicFunction to dyn PartialReflect.

Solution

Allow DynamicFunction to actually be reflected.

This PR adds the Function reflection subtrait (and corresponding ReflectRef, ReflectKind, etc.). With this new trait, we're able to implement PartialReflect on DynamicFunction.

Implementors

Function is currently only implemented for DynamicFunction<'static>. This is because we can't implement it generically over all functions—even those that implement IntoFunction.

What about DynamicFunctionMut? Well, this PR does not implement Function for DynamicFunctionMut.

The reasons for this are a little complicated, but it boils down to mutability. DynamicFunctionMut requires &mut self to be invoked since it wraps a FnMut. However, we can't really model this well with Function. And if we make DynamicFunctionMut wrap its internal FnMut in a Mutex to allow for &self invocations, then we run into either concurrency issues or recursion issues (or, in the worst case, both).

So for the time-being, we won't implement Function for DynamicFunctionMut. It will be better to evaluate it on its own. And we may even consider the possibility of removing it altogether if it adds too much complexity to the crate.

Dynamic vs Concrete

One of the issues with DynamicFunction is the fact that it's both a dynamic representation (like DynamicStruct or DynamicList) and the only way to represent a function.

Because of this, it's in a weird middle ground where we can't easily implement full-on Reflect. That would require Typed, but what static TypeInfo could it provide? Just that it's a DynamicFunction? None of the other dynamic types implement Typed.

However, by not implementing Reflect, we lose the ability to downcast back to our DynamicStruct. Our only option is to call Function::clone_dynamic, which clones the data rather than by simply downcasting. This works in favor of the PartialReflect::try_apply implementation since it would have to clone anyways, but is definitely not ideal. This is also the reason I had to add Debug as a supertrait on Function.

For now, this PR chooses not to implement Reflect for DynamicFunction. We may want to explore this in a followup PR (or even this one if people feel strongly that it's strictly required).

The same is true for FromReflect. We may decide to add an implementation there as well, but it's likely out-of-scope of this PR.

Testing

You can test locally by running:

cargo test --package bevy_reflect --all-features

Showcase

You can now pass around a DynamicFunction as a dyn PartialReflect! This also means you can use it as a field on a reflected type without having to ignore it (though you do need to opt out of FromReflect).

#[derive(Reflect)]
#[reflect(from_reflect = false)]
struct ClickEvent {
    callback: DynamicFunction<'static>,
}

let event: Box<dyn Struct> = Box::new(ClickEvent {
    callback: (|| println!("Clicked!")).into_function(),
});

// We can access our `DynamicFunction` as a `dyn PartialReflect`
let callback: &dyn PartialReflect = event.field("callback").unwrap();

// And access function-related methods via the new `Function` trait
let ReflectRef::Function(callback) = callback.reflect_ref() else {
    unreachable!()
};

// Including calling the function
callback.reflect_call(ArgList::new()).unwrap(); // Prints: Clicked!

@MrGVSV MrGVSV added C-Feature A new feature, making something new possible A-Reflection Runtime information about types D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 14, 2024
@MrGVSV MrGVSV added this to the 0.15 milestone Sep 14, 2024
Currently only implemented for DynamicFunction
Needed to get try_apply to work on DynamicFunction
@MrGVSV MrGVSV force-pushed the mrgvsv/reflect/function-trait branch from 8186212 to 15a5835 Compare September 15, 2024 22:43
Copy link
Contributor

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

Very cool, makes sense, and my mind is already jumping with possibilities... proxied DynamicStructs with callable fields that can change at runtime? My sense is that, in this respect, DynamicFunctionMut is going to be desirable, even if it requires some gotchas around reentrancy, etc.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 22, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 22, 2024
Merged via the queue into bevyengine:main with commit 59c0521 Sep 22, 2024
27 checks passed
@MrGVSV MrGVSV deleted the mrgvsv/reflect/function-trait branch September 22, 2024 23:39
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-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants