The dynamic example is scary and needs a revamp

[nicopap]

The example in question:

https://github.com/bevyengine/bevy/blob/259fb6896e07bed89df2637aace547f295fc7e82/examples/ecs/dynamic.rs

Does far more than presenting how to use the dynamic query API. Notably it:

1. Creates a custom components not tied to a rust type
2. It uses the dynamic query API
3. It uses std::alloc and NonNull to copy values from a Vec and point a OwningPtr to it.
4. It parses a mini syntax for querying.

To illustrate a dynamic querying API, I think (2) and (4) are sufficient. (1) should have a separate example. (3) could illustrate separately the bevy_ptr crate as well.

This might seem harmless, but in reality, it causes real problems: it confuses users. Here is an example: #11424

Other problems with the example

bevy/examples/ecs/dynamic.rs

Lines 123 to 128 in 259fb68

	let ptr = unsafe {
	let data = std::alloc::alloc_zeroed(info.layout()).cast::<u64>();
	data.copy_from(values.as_mut_ptr(), values.len());
	let non_null = NonNull::new_unchecked(data.cast());
	OwningPtr::new(non_null)
	};

There is direct calls to std::alloc functions, a lot of manual pointer fiddling (cast, copy_from, NonNull::new), Even a memory leak! (the data ptr is allocated without ever being freed) I think this kind of code has nothing to do in a bevy example, not only it isn't representative of general bevy usage, but it might scare off potential users. Especially given that supposedly people chose a rust game engine because they don't have to remember to free their dynamic arrays!

Proposed changes

In actuality, the currently exposed API do still require OwningPtr (the bevy_ptr type) at least for insertion but I think the best example would show how to convert between the Ptr returned by dynamic query iteration into a &dyn Reflect. This has already been a question (@coreh) and I suspect it's going to be super useful to a lot of users.

The way you go from the PtrMut to a &mut dyn Reflect is as follow:

1. Get the ReflectFromPtr for the component id. Here I cache it when I build the query state, so that I don't have to fetch it while iterating the query
   https://github.com/nicopap/bevy_mod_dynamic_query/blob/149b5d4a5a3611373b99b83bc9075cbd7d03b01e/src/builder/named.rs#L28-L32

2. Get the Ptr or PtrMut returned by iterating a query in the Dynamic queries and builder API #9774 API and call ReflectFromPtr::from_ptr on the individual fetch items, then you have a &dyn Reflect:
   https://github.com/nicopap/bevy_mod_dynamic_query/blob/149b5d4a5a3611373b99b83bc9075cbd7d03b01e/src/fetches.rs#L167-L169
   Here is a version of point (1) where I go from T: Component to ReflectFromPtr: https://github.com/nicopap/bevy_mod_dynamic_query/blob/149b5d4a5a3611373b99b83bc9075cbd7d03b01e/src/builder/methods.rs#L23-L29

Knowing how to convert into a &dyn Reflect a PtrMut is immediately more useful than knowing how to copy from a Vec<u64> into a custom component. It also has more applications, especially for someone who is interested into using the dynamic query API.

[james-j-obrien]

Your right that the memory allocation is scary, and it does seem to create a leak as the OwningPtr doesn't free the buffer it copies from. This is unfortunate and I wish we had better APIs for inserting dynamic components, I'll look into creating a fix for this. EDIT: Created a PR with a fix.

I do agree that having all of the dynamic ECS constructs in one example is a little overwhelming and makes it hard to parse for new users who probably only need 1 or 2 parts of it, so we should probably split it up or at the very least document it more thoroughly showing alternatives for simpler use cases. I do think there will be some people who want to do everything that the example is doing and it is non-trivial to figure out how to use bevy's dynamic APIs so I think all parts of it should still exist somewhere.

The main reason I made the example the way it is was to prove it was possible to have the components, queries and entities "fully" defined dynamically, but it's non-ergonomic and I think it also points to some areas where we could improve the bevy_ecs API itself. Ideally there should be a better way to insert owned values into dynamic components without either recursive calls to OwningPtr::make or multiple unsafe casts.

I also agree that we should show how to convert from Ptr to a dyn Reflect, it doesn't necessarily have to be associated with dynamic queries since there are other scenarios you would have to perform that conversion but it's as good a place as any.

[nicopap]

Yeah! In any case, I think you did great work, and seriously, even a non-pedagogically-optimal example shouldn't be a blocker to merge such an important feature as the one you implemented.

I want to say how much I'm glad you implemented dynamic queries. I personally need them a lot, and the rest of the community will be delighted, as direct users, and as users of the future functionalities built on top of dynamic queries.

Thank you again! I think it makes sense that the rest of the community (me included) cleans up behind. I wanted to direct attention to things that can be improved so that new users could use more effectively your work.