Support closures as ComponentHooks

[james7132]

Objective

Component hooks right now only take raw function pointers instead of allowing those registering it to store some state with them.

Solution

Store an Arc<dyn Fn(...)> instead of a function pointer and make the registration functions generic over the function type being passed in.

Right now this only takes read only closures (Fn not FnMut), though strictly speaking we could support them as we have mutable access when triggering the hooks, accessing the closure mutably through an Arc would be troublesome if the ComponentInfo was cloned. I tried to keep the traits on ComponentHooks and ComponentInfo intact as much as I could.

This shouldn't change the performance characteristics of the crate when not using a closure since &dyn Fn(...) directly includes the function pointer instead of being a double indirection.

This PR also makes the type alias for ComponentHook private since it's now not part of the public interface of the crate.

[james-j-obrien]

Looks good, the only reason I didn't do this in the first pass was because I wasn't sure about the potential for overhead, if this is the same for non-closure functions then it seems like a good change.

[james7132]

Looks good, the only reason I didn't do this in the first pass was because I wasn't sure about the potential for overhead, if this is the same for non-closure functions then it seems like a good change.

Thinking about this a bit more, the Arc may throw a wrench in that due to the extra metadata it has. It may be better to do this with a Box and drop the Clone impl.

@SludgePhD filed #9812 which introduced the Clone impl, primarily for inspecting the ComponentInfo without a borrow on the World. The Clone impl could be removed and replaced with an equivalent extraction of the metadata (i.e. reading the information into a hypothetical ComponentSummary, and stored elsewhere). It'd also allow us to use FnMut closures instead, which may be very useful for storing state inside of a component hook.

[MiniaczQ]

Had to ask for some language explanations on discord 😅
Looks good!

[james7132]

On further inspection, having FnMut hooks may be unsound due to UnsafeWorldCell::world_mut's safety invariants as DeferredWorld would only be sound to pass into the hook if the hook doesn't use the DeferredWorld in any way. I'll just switch it to using Box instead of Arc.

[JoJoJet]

Why not leave it as a function pointer for simplicity and minimal overhead? If you really need to store state you can always use a resource, though I feel like if you're doing anything complicated with a hook you should probably use an observer instead

[james7132]

Why not leave it as a function pointer for simplicity and minimal overhead? If you really need to store state you can always use a resource, though I feel like if you're doing anything complicated with a hook you should probably use an observer instead

That's fair. Though I think this comes across as something most typical Rust users expect when working with callbacks. I've definitely found it rare to come across an API that takes a function as an argument and not have it take a closure of some kind.

Perhaps we do need some sort of benchmarking to verify that this doesn't cost us anything extra when using normal function pointers.

[JoJoJet]

Perhaps we do need some sort of benchmarking to verify that this doesn't cost us anything extra when using normal function pointers.

I mean, it does cost us extra, since the runtime needs to go through the vtable to find the address, rather than just directly using the function pointer. I don't think it will be a significant slowdown, but for something so low-level, why have more overhead when we can have less?

[alice-i-cecile]

@james7132 what do you think should be done with this, and how can I help? I'd like to get this into 0.14 to try and ship a nicer API in the first iteration of hooks.