bevy_reflect: Function registry #14098

MrGVSV · 2024-07-01T18:55:43Z

Objective

#13152 added support for reflecting functions. Now, we need a way to register those functions such that they may be accessed anywhere within the ECS.

Solution

Added a FunctionRegistry type similar to TypeRegistry.

This allows a function to be registered and retrieved by name.

fn foo() -> i32 {
    123
}

let mut registry = FunctionRegistry::default();
registry.register("my_function", foo);

let function = registry.get_mut("my_function").unwrap();
let value = function.call(ArgList::new()).unwrap().unwrap_owned();
assert_eq!(value.downcast_ref::<i32>(), Some(&123));

Additionally, I added an AppFunctionRegistry resource which wraps a FunctionRegistryArc. Functions can be registered into this resource using App::register_function or by getting a mutable reference to the resource itself.

Limitations

`Send + Sync`

In order to get this registry to work across threads, it needs to be Send + Sync. This means that DynamicFunction needs to be Send + Sync, which means that its internal function also needs to be Send + Sync.

In most cases, this won't be an issue because standard Rust functions (the type most likely to be registered) are always Send + Sync. Additionally, closures tend to be Send + Sync as well, granted they don't capture any !Send or !Sync variables.

This PR adds this Send + Sync requirement, but as mentioned above, it hopefully shouldn't be too big of an issue.

Closures

Unfortunately, closures can't be registered yet. This will likely be explored and added in a followup PR.

Future Work

Besides addressing the limitations listed above, another thing we could look into is improving the lookup of registered functions. One aspect is in the performance of hashing strings. The other is in the developer experience of having to call std::any::type_name_of_val to get the name of their function (assuming they didn't give it a custom name).

Testing

You can run the tests locally with:

cargo test --package bevy_reflect

Changelog

Added FunctionRegistry
Added AppFunctionRegistry (a Resource available from bevy_ecs)
Added FunctionRegistryArc
Added FunctionRegistrationError
Added reflect_functions feature to bevy_ecs and bevy_app
FunctionInfo is no longer Default
DynamicFunction now requires its wrapped function be Send + Sync

Internal Migration Guide

Important

Function reflection was introduced as part of the 0.15 dev cycle. This migration guide was written for developers relying on main during this cycle, and is not a breaking change coming from 0.14.

DynamicFunction (both those created manually and those created with IntoFunction), now require Send + Sync. All standard Rust functions should meet that requirement. Closures, on the other hand, may not if they capture any !Send or !Sync variables from its environment.

crates/bevy_reflect/src/func/registry.rs

NthTensor · 2024-07-05T00:34:23Z

crates/bevy_reflect/src/func/function.rs

 }

 /// Outputs the function signature.
 ///
 /// This takes the format: `DynamicFunction(fn {name}({arg1}: {type1}, {arg2}: {type2}, ...) -> {return_type})`.
 ///
-/// Names for arguments and the function itself are optional and will default to `_` if not provided.
+/// Names for arguments are optional and will default to `_` if not provided.


This doc line seems out of date.

Argument names are still optional since we can't infer these through the type system unfortunately :(

NthTensor · 2024-07-05T00:36:43Z

crates/bevy_reflect/src/func/into_function.rs

            F: FnMut($($Arg),*) -> R + 'env,
            F: for<'a> FnMut($($Arg::Item<'a>),*) -> R + 'env,
+            F: Send + Sync,


Oh, I didn't know you could add multiple bounds on the same thing like this.

I learned this recently too (I think while working on function reflection)! It definitely helps break them up for readability imo

NthTensor

Looks good to me. Code is well documented and clean. Anything remotely controversial (closure names and static refs) will be resolved as future work.

Thanks for working on this! That was quite a quick turn around time as well.

BD103 · 2024-07-05T19:14:19Z

Is there any better alternative to use beyond type_name()? (Maybe TypeId?) The type name is not guaranteed to be unique, so there's a chance multiple functions could collide.

"The returned string must not be considered to be a unique identifier of a type as multiple types may map to the same type name."
- type_name() docs

MrGVSV · 2024-07-05T23:02:16Z

Is there any better alternative to use beyond type_name()? (Maybe TypeId?) The type name is not guaranteed to be unique, so there's a chance multiple functions could collide.

"The returned string must not be considered to be a unique identifier of a type as multiple types may map to the same type name."

type_name() docs

Great question! I probably should have mentioned the reasoning for using type_name here.

Ideally, yes, we'd use a TypeId. Unfortunately, there's a few issues with this:

1. DynamicFunction can be created manually and do not need to be tied to any one function. This is because it essentially holds a wrapper function ((ArgList, FunctionInfo) -> FunctionResult) that reifies all the reflected arguments. A user can choose to do their logic their, not needing a dedicated function, or they can choose to call multiple functions. This means that we can't guarantee there's always exactly one TypeId to represent the function.
2. We can't pull the TypeId off the closure that calls to the actual function either, since each instance of that closure would be unique. This is an issue because it means that foo.into_function() won't have the same TypeId as another call to foo.into_function()
3. We don't want to have the registry itself extract the TypeId from the given function since it accepts DynamicFunction, which results in the same problems as above (note that we really should keep the ability to register a DynamicFunction directly since we're very limited by Rust in how many functions we can realistically have implement IntoFunction).

Edit: While I was typing this I ended up doing a couple quick tests regarding that second point and I'm completely wrong lol. It turns out it does result in the same TypeId every time!

So it is possible to instead store these by their TypeId. The only issue right now is that it has to be done in the constructor of DynamicFunction since we have to have access to the actual type of the closure, F, and it only works if F has a 'static lifetime.

So this should theoretically be possible with #14141 since it adds the requirement that DynamicFunction is 'static. Maybe we wait until that one is merged?

BD103 · 2024-07-05T23:08:03Z

So this should theoretically be possible with #14141 since it adds the requirement that DynamicFunction is 'static. Maybe we wait until that one is merged?

I would prefer that, since TypeId is guaranteed unique and not intended specifically for debugging.

NthTensor · 2024-07-06T00:46:05Z

I think it makes sense to hold off on this until after the DynamicClosure split.

crates/bevy_reflect/src/func/registry.rs

# Objective As mentioned in [this](#13152 (comment)) comment, creating a function registry (see #14098) is a bit difficult due to the requirements of `DynamicFunction`. Internally, a `DynamicFunction` contains a `Box<dyn FnMut>` (the function that reifies reflected arguments and calls the actual function), which requires `&mut self` in order to be called. This means that users would require a mutable reference to the function registry for it to be useful— which isn't great. And they can't clone the `DynamicFunction` either because cloning an `FnMut` isn't really feasible (wrapping it in an `Arc` would allow it to be cloned but we wouldn't be able to call the clone since we need a mutable reference to the `FnMut`, which we can't get with multiple `Arc`s still alive, requiring us to also slap in a `Mutex`, which adds additional overhead). And we don't want to just replace the `dyn FnMut` with `dyn Fn` as that would prevent reflecting closures that mutate their environment. Instead, we need to introduce a new type to split the requirements of `DynamicFunction`. ## Solution Introduce new types for representing closures. Specifically, this PR introduces `DynamicClosure` and `DynamicClosureMut`. Similar to how `IntoFunction` exists for `DynamicFunction`, two new traits were introduced: `IntoClosure` and `IntoClosureMut`. Now `DynamicFunction` stores a `dyn Fn` with a `'static` lifetime. `DynamicClosure` also uses a `dyn Fn` but has a lifetime, `'env`, tied to its environment. `DynamicClosureMut` is most like the old `DynamicFunction`, keeping the `dyn FnMut` and also typing its lifetime, `'env`, to the environment Here are some comparison tables: | | `DynamicFunction` | `DynamicClosure` | `DynamicClosureMut` | | - | ----------------- | ---------------- | ------------------- | | Callable with `&self` | ✅ | ✅ | ❌ | | Callable with `&mut self` | ✅ | ✅ | ✅ | | Allows for non-`'static` lifetimes | ❌ | ✅ | ✅ | | | `IntoFunction` | `IntoClosure` | `IntoClosureMut` | | - | -------------- | ------------- | ---------------- | | Convert `fn` functions | ✅ | ✅ | ✅ | | Convert `fn` methods | ✅ | ✅ | ✅ | | Convert anonymous functions | ✅ | ✅ | ✅ | | Convert closures that capture immutable references | ❌ | ✅ | ✅ | | Convert closures that capture mutable references | ❌ | ❌ | ✅ | | Convert closures that capture owned values | ❌[^1] | ✅ | ✅ | [^1]: Due to limitations in Rust, `IntoFunction` can't be implemented for just functions (unless we forced users to manually coerce them to function pointers first). So closures that meet the trait requirements _can technically_ be converted into a `DynamicFunction` as well. To both future-proof and reduce confusion, though, we'll just pretend like this isn't a thing. ```rust let mut list: Vec<i32> = vec![1, 2, 3]; // `replace` is a closure that captures a mutable reference to `list` let mut replace = |index: usize, value: i32| -> i32 { let old_value = list[index]; list[index] = value; old_value }; // Convert the closure into a dynamic closure using `IntoClosureMut::into_closure_mut` let mut func: DynamicClosureMut = replace.into_closure_mut(); // Dynamically call the closure: let args = ArgList::default().push_owned(1_usize).push_owned(-2_i32); let value = func.call_once(args).unwrap().unwrap_owned(); // Check the result: assert_eq!(value.take::<i32>().unwrap(), 2); assert_eq!(list, vec![1, -2, 3]); ``` ### `ReflectFn`/`ReflectFnMut` To make extending the function reflection system easier (the blanket impls for `IntoFunction`, `IntoClosure`, and `IntoClosureMut` are all incredibly short), this PR generalizes callables with two new traits: `ReflectFn` and `ReflectFnMut`. These traits mimic `Fn` and `FnMut` but allow for being called via reflection. In fact, their blanket implementations are identical save for `ReflectFn` being implemented over `Fn` types and `ReflectFnMut` being implemented over `FnMut` types. And just as `Fn` is a subtrait of `FnMut`, `ReflectFn` is a subtrait of `ReflectFnMut`. So anywhere that expects a `ReflectFnMut` can also be given a `ReflectFn`. To reiterate, these traits aren't 100% necessary. They were added in purely for extensibility. If we decide to split things up differently or add new traits/types in the future, then those changes should be much simpler to implement. ### `TypedFunction` Because of the split into `ReflectFn` and `ReflectFnMut`, we needed a new way to access the function type information. This PR moves that concept over into `TypedFunction`. Much like `Typed`, this provides a way to access a function's `FunctionInfo`. By splitting this trait out, it helps to ensure the other traits are focused on a single responsibility. ### Internal Macros The original function PR (#13152) implemented `IntoFunction` using a macro which was passed into an `all_tuples!` macro invocation. Because we needed the same functionality for these new traits, this PR has copy+pasted that code for `ReflectFn`, `ReflectFnMut`, and `TypedFunction`— albeit with some differences between them. Originally, I was going to try and macro-ify the impls and where clauses such that we wouldn't have to straight up duplicate a lot of this logic. However, aside from being more complex in general, autocomplete just does not play nice with such heavily nested macros (tried in both RustRover and VSCode). And both of those problems told me that it just wasn't worth it: we need to ensure the crate is easily maintainable, even at the cost of duplicating code. So instead, I made sure to simplify the macro code by removing all fully-qualified syntax and cutting the where clauses down to the bare essentials, which helps to clean up a lot of the visual noise. I also tried my best to document the macro logic in certain areas (I may even add a bit more) to help with maintainability for future devs. ### Documentation Documentation for this module was a bit difficult for me. So many of these traits and types are very interconnected. And each trait/type has subtle differences that make documenting it in a single place, like at the module level, difficult to do cleanly. Describing the valid signatures is also challenging to do well. Hopefully what I have here is okay. I think I did an okay job, but let me know if there any thoughts on ways to improve it. We can also move such a task to a followup PR for more focused discussion. ## Testing You can test locally by running: ``` cargo test --package bevy_reflect ``` --- ## Changelog - Added `DynamicClosure` struct - Added `DynamicClosureMut` struct - Added `IntoClosure` trait - Added `IntoClosureMut` trait - Added `ReflectFn` trait - Added `ReflectFnMut` trait - Added `TypedFunction` trait - `IntoFunction` now only works for standard Rust functions - `IntoFunction` no longer takes a lifetime parameter - `DynamicFunction::call` now only requires `&self` - Removed `DynamicFunction::call_once` - Changed the `IntoReturn::into_return` signature to include a where clause ## Internal Migration Guide > [!important] > Function reflection was introduced as part of the 0.15 dev cycle. This migration guide was written for developers relying on `main` during this cycle, and is not a breaking change coming from 0.14. ### `IntoClosure` `IntoFunction` now only works for standard Rust functions. Calling `IntoFunction::into_function` on a closure that captures references to its environment (either mutable or immutable), will no longer compile. Instead, you will need to use either `IntoClosure::into_closure` to create a `DynamicClosure` or `IntoClosureMut::into_closure_mut` to create a `DynamicClosureMut`, depending on your needs: ```rust let punct = String::from("!"); let print = |value: String| { println!("{value}{punct}"); }; // BEFORE let func: DynamicFunction = print.into_function(); // AFTER let func: DynamicClosure = print.into_closure(); ``` ### `IntoFunction` lifetime Additionally, `IntoFunction` no longer takes a lifetime parameter as it always expects a `'static` lifetime. Usages will need to remove any lifetime parameters: ```rust // BEFORE fn execute<'env, F: IntoFunction<'env, Marker>, Marker>(f: F) {/* ... */} // AFTER fn execute<F: IntoFunction<Marker>, Marker>(f: F) {/* ... */} ``` ### `IntoReturn` `IntoReturn::into_return` now has a where clause. Any manual implementors will need to add this where clause to their implementation.

MrGVSV · 2024-07-24T20:01:09Z

Okay so since function reflection is now behind a feature, I had to add corresponding features to bevy_app and bevy_ecs. Hopefully, that's okay!

MrGVSV · 2024-08-04T18:50:09Z

Okay so I've updated the PR to use Option 3: require names during registration.

I mentioned it on Discord, but the only way to get the 2/3 mix I mentioned here is to parse the result of std::any::type_name to determine if the given function is named or not.

While that should work well and is unlikely to suddenly break, it's not guaranteed it will never break. std::any::type_name mentions in its docs that it shouldn't really be used for stuff like this. Now, I know Bevy is one to bend the rules a bit so I will try to put together a followup PR that makes use of that approach. But for now, we'll try to make this current PR as non-controversial as possible.

MrGVSV · 2024-08-04T18:53:02Z

crates/bevy_app/src/app.rs

+    /// # Panics
+    ///
+    /// Panics if a function has already been registered with the given name.


Should we panic here or just return the Result?

If we return the Result, then users will be forced to unwrap it themselves, but it gives them more options on handling the error (not that they can't just get the AppFunctionRegistry directly).

Long term, I would prefer is this returned a result. But since none of the other methods on App do currently, I vote we panic for consistency. We can take a more general look at error handling during app setup later.

NthTensor

I've been following the discussion and think this API you landed on is great. Clean code, great docs, looks excellent to me.

mweatherley

I think this is a good move.

# Objective ### TL;DR #14098 added the `FunctionRegistry` but had some last minute complications due to anonymous functions. It ended up going with a "required name" approach to ensure anonymous functions would always have a name. However, this approach isn't ideal for named functions since, by definition, they will always have a name. Therefore, this PR aims to modify function reflection such that we can make function registration easier for named functions, while still allowing anonymous functions to be registered as well. ### Context Function registration (#14098) ran into a little problem: anonymous functions. Anonymous functions, including function pointers, have very non-unique type names. For example, the anonymous function `|a: i32, b: i32| a + b` has the type name of `fn(i32, i32) -> i32`. This obviously means we'd conflict with another function like `|a: i32, b: i32| a - b`. The solution that #14098 landed on was to always require a name during function registration. The downside with this is that named functions (e.g. `fn add(a: i32, b: i32) -> i32 { a + b }`) had to redundantly provide a name. Additionally, manually constructed `DynamicFunction`s also ran into this ergonomics issue. I don't entirely know how the function registry will be used, but I have a strong suspicion that most of its registrations will either be named functions or manually constructed `DynamicFunction`s, with anonymous functions only being used here and there for quick prototyping or adding small functionality. Why then should the API prioritize the anonymous function use case by always requiring a name during registration? #### Telling Functions Apart Rust doesn't provide a lot of out-of-the-box tools for reflecting functions. One of the biggest hurdles in attempting to solve the problem outlined above would be to somehow tell the different kinds of functions apart. Let's briefly recap on the categories of functions in Rust: | Category | Example | | ------------------ | ----------------------------------------- | | Named function | `fn add(a: i32, b: i32) -> i32 { a + b }` | | Closure | `\|a: i32\| a + captured_variable` | | Anonymous function | `\|a: i32, b: i32\| a + b` | | Function pointer | `fn(i32, i32) -> i32` | My first thought was to try and differentiate these categories based on their size. However, we can see that this doesn't quite work: | Category | `size_of` | | ------------------ | --------- | | Named function | 0 | | Closure | 0+ | | Anonymous function | 0 | | Function pointer | 8 | Not only does this not tell anonymous functions from named ones, but it struggles with pretty much all of them. My second then was to differentiate based on type name: | Category | `type_name` | | ------------------ | ----------------------- | | Named function | `foo::bar::baz` | | Closure | `foo::bar::{{closure}}` | | Anonymous function | `fn() -> String` | | Function pointer | `fn() -> String` | This is much better. While it can't distinguish between function pointers and anonymous functions, this doesn't matter too much since we only care about whether we can _name_ the function. So why didn't we implement this in #14098? #### Relying on `type_name` While this solution was known about while working on #14098, it was left out from that PR due to it being potentially controversial. The [docs](https://doc.rust-lang.org/stable/std/any/fn.type_name.html) for `std::any::type_name` state: > The returned string must not be considered to be a unique identifier of a type as multiple types may map to the same type name. Similarly, there is no guarantee that all parts of a type will appear in the returned string: for example, lifetime specifiers are currently not included. In addition, the output may change between versions of the compiler. So that's it then? We can't use `type_name`? Well, this statement isn't so much a rule as it is a guideline. And Bevy is no stranger to bending the rules to make things work or to improve ergonomics. Remember that before `TypePath`, Bevy's scene system was entirely dependent on `type_name`. Not to mention that `type_name` is being used as a key into both the `TypeRegistry` and the `FunctionRegistry`. Bevy's practices aside, can we reliably use `type_name` for this? My answer would be "yes". Anonymous functions are anonymous. They have no name. There's nothing Rust could do to give them a name apart from generating a random string of characters. But remember that this is a diagnostic tool, it doesn't make sense to obfuscate the type by randomizing the output. So changing it to be anything other than what it is now is very unlikely. The only changes that I could potentially see happening are: 1. Closures replace `{{closure}}` with the name of their variable 2. Lifetimes are included in the output I don't think the first is likely to happen, but if it does then it actually works out in our favor: closures are now named! The second point is probably the likeliest. However, adding lifetimes doesn't mean we can't still rely on `type_name` to determine whether or not a function is named. So we should be okay in this case as well. ## Solution Parse the `type_name` of the function in the `TypedFunction` impl to determine if the function is named or anonymous. This once again makes `FunctionInfo::name` optional. For manual constructions of `DynamicFunction`, `FunctionInfo::named` or ``FunctionInfo::anonymous` can be used. The `FunctionRegistry` API has also been reworked to account for this change. `FunctionRegistry::register` no longer takes a name and instead takes it from the supplied function, returning a `FunctionRegistrationError::MissingName` error if the name is `None`. This also doubles as a replacement for the old `FunctionRegistry::register_dynamic` method, which has been removed. To handle anonymous functions, a `FunctionRegistry::register_with_name` method has been added. This works in the same way `FunctionRegistry::register` used to work before this PR. The overwriting methods have been updated in a similar manner, with modifications to `FunctionRegistry::overwrite_registration`, the removal of `FunctionRegistry::overwrite_registration_dynamic`, and the addition of `FunctionRegistry::overwrite_registration_with_name`. This PR also updates the methods on `App` in a similar way: `App::register_function` no longer requires a name argument and `App::register_function_with_name` has been added to handle anonymous functions (and eventually closures). ## Testing You can run the tests locally by running: ``` cargo test --package bevy_reflect --features functions ``` --- ## Internal Migration Guide > [!important] > Function reflection was introduced as part of the 0.15 dev cycle. This migration guide was written for developers relying on `main` during this cycle, and is not a breaking change coming from 0.14. > [!note] > This list is not exhaustive. It only contains some of the most important changes. `FunctionRegistry::register` no longer requires a name string for named functions. Anonymous functions, however, need to be registered using `FunctionRegistry::register_with_name`. ```rust // BEFORE registry .register(std::any::type_name_of_val(&foo), foo)? .register("bar", || println!("Hello world!")); // AFTER registry .register(foo)? .register_with_name("bar", || println!("Hello world!")); ``` `FunctionInfo::name` is now optional. Anonymous functions and closures will now have their name set to `None` by default. Additionally, `FunctionInfo::new` has been renamed to `FunctionInfo::named`.

…onRegistry` (#14704) # Objective #14098 added the `FunctionRegistry` for registering functions such that they can be retrieved by name and used dynamically. One thing we chose to leave out in that initial PR is support for closures. Why support closures? Mainly, we don't want to prohibit users from injecting environmental data into their registered functions. This allows these functions to not leak their internals to the public API. For example, let's say we're writing a library crate that allows users to register callbacks for certain actions. We want to perform some actions before invoking the user's callback so we can't just call it directly. We need a closure for this: ```rust registry.register("my_lib::onclick", move |event: ClickEvent| { // ...other work... user_onclick.call(event); // <-- Captured variable }); ``` We could have made our callback take a reference to the user's callback. This would remove the need for the closure, but it would change our desired API to place the burden of fetching the correct callback on the caller. ## Solution Modify the `FunctionRegistry` to store registered functions as `DynamicClosure<'static>` instead of `DynamicFunction` (now using `IntoClosure` instead of `IntoFunction`). Due to limitations in Rust and how function reflection works, `DynamicClosure<'static>` is functionally equivalent to `DynamicFunction`. And a normal function is considered a subset of closures (it's a closure that doesn't capture anything), so there shouldn't be any difference in usage: all functions that satisfy `IntoFunction` should satisfy `IntoClosure`. This means that the registration API introduced in #14098 should require little-to-no changes on anyone following `main`. ### Closures vs Functions One consideration here is whether we should keep closures and functions separate. This PR unifies them into `DynamicClosure<'static>`, but we can consider splitting them up. The reasons we might want to do so are: - Simplifies mental model and terminology (users don't have to understand that functions turn into closures) - If Rust ever improves its function model, we may be able to add additional guarantees to `DynamicFunction` that make it useful to separate the two - Adding support for generic functions may be less confusing for users since closures in Rust technically can't be generic The reasons behind this PR's unification approach are: - Reduces the number of methods needed on `FunctionRegistry` - Reduces the number of lookups a user may have to perform (i.e. "`get_function` or else `get_closure`") - Establishes `DynamicClosure<'static>` as the de facto dynamic callable (similar to how most APIs in Rust code tend to prefer `impl Fn() -> String` over `fn() -> String`) I'd love to hear feedback on this matter, and whether we should continue with this PR's approach or switch to a split model. ## Testing You can test locally by running: ``` cargo test --package bevy_reflect ``` --- ## Showcase Closures can now be registered into the `FunctionRegistry`: ```rust let punct = String::from("!!!"); registry.register_with_name("my_crate::punctuate", move |text: String| { format!("{}{}", text, punct) }); ```

MrGVSV added C-Feature A new feature, making something new possible A-Reflection Runtime information about types D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 1, 2024

MrGVSV force-pushed the mrgvsv/reflect/function-registry branch from fbc660d to 92a383a Compare July 1, 2024 18:58

NthTensor self-requested a review July 1, 2024 19:02

alice-i-cecile added this to the 0.15 milestone Jul 2, 2024

MrGVSV commented Jul 4, 2024

View reviewed changes

crates/bevy_reflect/src/func/registry.rs Outdated Show resolved Hide resolved

MrGVSV mentioned this pull request Jul 4, 2024

bevy_reflect: Add DynamicClosure and DynamicClosureMut #14141

Merged

NthTensor reviewed Jul 5, 2024

View reviewed changes

NthTensor approved these changes Jul 5, 2024

View reviewed changes

MrGVSV added S-Blocked This cannot move forward until something else changes and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 5, 2024

mweatherley reviewed Jul 12, 2024

View reviewed changes

crates/bevy_reflect/src/func/registry.rs Outdated Show resolved Hide resolved

MrGVSV added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Blocked This cannot move forward until something else changes labels Jul 17, 2024

MrGVSV force-pushed the mrgvsv/reflect/function-registry branch from 92a383a to fbfb72d Compare July 24, 2024 17:58

MrGVSV added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jul 24, 2024

MrGVSV force-pushed the mrgvsv/reflect/function-registry branch 2 times, most recently from f1b9e2e to efb4bd5 Compare July 24, 2024 20:00

MrGVSV force-pushed the mrgvsv/reflect/function-registry branch 2 times, most recently from 5a6b2b9 to ab81521 Compare July 24, 2024 20:37

MrGVSV added 7 commits August 4, 2024 11:41

Fix docs on App

231d817

Require functions to be Send + Sync

03dc89a

Add FunctionRegistrationError

6d61b57

Fix doc

61c55cf

Update docs to include valid inputs for registration

83f6169

Remove FunctionRegistry::get_mut

8ecd6d6

Require a name for FunctionRegistry::register

9b7d13a

MrGVSV force-pushed the mrgvsv/reflect/function-registry branch from 315787a to 66c7eff Compare August 4, 2024 18:42

MrGVSV commented Aug 4, 2024

View reviewed changes

Add doc examples

6ce018a

MrGVSV force-pushed the mrgvsv/reflect/function-registry branch from 66c7eff to 6ce018a Compare August 4, 2024 18:56

MrGVSV added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Needs-Design This issue requires design work to think about how it would best be accomplished labels Aug 4, 2024

MrGVSV marked this pull request as ready for review August 4, 2024 18:57

NthTensor approved these changes Aug 4, 2024

View reviewed changes

NthTensor 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 Aug 4, 2024

mweatherley approved these changes Aug 5, 2024

View reviewed changes

alice-i-cecile added C-Needs-Release-Note Work that should be called out in the blog due to impact and removed C-Needs-Release-Note Work that should be called out in the blog due to impact labels Aug 6, 2024

alice-i-cecile approved these changes Aug 6, 2024

View reviewed changes

alice-i-cecile added this pull request to the merge queue Aug 6, 2024

Merged via the queue into bevyengine:main with commit df61117 Aug 6, 2024
33 checks passed

MrGVSV deleted the mrgvsv/reflect/function-registry branch August 6, 2024 18:58

MrGVSV mentioned this pull request Aug 6, 2024

bevy_reflect: Anonymous function parsing #14641

Merged

MrGVSV mentioned this pull request Aug 11, 2024

bevy_reflect: Store functions as DynamicClosure<'static> in FunctionRegistry #14704

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bevy_reflect: Function registry #14098

bevy_reflect: Function registry #14098

MrGVSV commented Jul 1, 2024 •

edited

Loading

NthTensor Jul 5, 2024

MrGVSV Jul 5, 2024

NthTensor Jul 5, 2024

MrGVSV Jul 5, 2024

NthTensor left a comment

BD103 commented Jul 5, 2024

MrGVSV commented Jul 5, 2024

BD103 commented Jul 5, 2024

NthTensor commented Jul 6, 2024

MrGVSV commented Jul 24, 2024

MrGVSV commented Aug 4, 2024

MrGVSV Aug 4, 2024

NthTensor Aug 4, 2024

NthTensor left a comment

mweatherley left a comment

bevy_reflect: Function registry #14098

bevy_reflect: Function registry #14098

Conversation

MrGVSV commented Jul 1, 2024 • edited Loading

Objective

Solution

Limitations

Send + Sync

Closures

Future Work

Testing

Changelog

Internal Migration Guide

NthTensor Jul 5, 2024

Choose a reason for hiding this comment

MrGVSV Jul 5, 2024

Choose a reason for hiding this comment

NthTensor Jul 5, 2024

Choose a reason for hiding this comment

MrGVSV Jul 5, 2024

Choose a reason for hiding this comment

NthTensor left a comment

Choose a reason for hiding this comment

BD103 commented Jul 5, 2024

MrGVSV commented Jul 5, 2024

BD103 commented Jul 5, 2024

NthTensor commented Jul 6, 2024

MrGVSV commented Jul 24, 2024

MrGVSV commented Aug 4, 2024

MrGVSV Aug 4, 2024

Choose a reason for hiding this comment

NthTensor Aug 4, 2024

Choose a reason for hiding this comment

NthTensor left a comment

Choose a reason for hiding this comment

mweatherley left a comment

Choose a reason for hiding this comment

MrGVSV commented Jul 1, 2024 •

edited

Loading

`Send + Sync`