Support more kinds of system params in buildable systems.#14050
Support more kinds of system params in buildable systems.#14050alice-i-cecile merged 3 commits intobevyengine:mainfrom
Conversation
…llow better composition of builders.
bfaef59 to
20577c8
Compare
|
It would be nice if this API could support some of the other |
|
In case it's useful to see some of the things I'm using this API to do, currently this will only support one #[no_mangle]
pub unsafe extern "cdecl" fn system_new(app: *mut App, request: SystemData) {
let data = std::slice::from_raw_parts(request.queries, request.count)
.iter()
.map(|&ptr| *ptr)
.collect::<Vec<_>>();
let system = SystemBuilder::<()>::new((*app).world_mut())
.builder::<Query<FilteredEntityMut>>(|query| {
for component in &data {
let id = ComponentId::new(component.id);
let kind = &component.kind;
match kind {
ComponentKind::Reference => {
query.ref_id(id);
}
ComponentKind::Mutable => {
query.mut_id(id);
}
ComponentKind::With => {
query.with_id(id);
}
ComponentKind::Without => {
query.without_id(id);
}
}
}
})
.build(move |query: Query<FilteredEntityMut>| {
let mut count = 0usize;
let components: Vec<*mut u8> = query
.iter()
.flat_map(|entity| {
count += 1;
data.iter()
.filter_map(|component| match component.kind {
ComponentKind::Reference | ComponentKind::Mutable => Some(
entity
.get_by_id(ComponentId::new(component.id))
.unwrap()
.as_ptr(),
),
_ => None,
})
.collect::<Vec<_>>()
})
.collect();
let components_ptr = components.as_ptr();
std::mem::forget(components);
(request.callback)(QueryResult {
components: components_ptr,
count,
});
});
(*app).add_systems(Update, system);
} |
Both the existing API and the one in this PR handle arbitrary
Maybe you want a buildable I don't think this PR would make that easier or harder to implement.
This PR implements |
| } | ||
|
|
||
| #[test] | ||
| fn param_set_vec_builder() { |
There was a problem hiding this comment.
Question: what's the point of being able to extract a vec of queries? This seems...generally unhelpful.
There was a problem hiding this comment.
I plan to use it for scripting. The idea is to load a script at runtime that does any number of queries, and then build a script runner system with a Vec<Query<FilteredEntityMut>> based on those queries (or ParamSet<Vec<Query<FilteredEntityMut>>> if they might conflict). The script runner system would bind the queries in the script by indexing the Vec.
(Buildable Vec and ParamSet<Vec> are the features I actually wanted here, and it would have been straightforward to implement BuildableSystemParam for them. It just felt wrong to implement buildable ParamSet<Vec> without buildable ParmSet<(tuple,)>, and I didn't see a clean way to build tuples with a builder type.)
I'm happy to split it out to a follow-up PR if that makes this easier to review!
There was a problem hiding this comment.
Yes please, I'd much prefer reviewing it as a separate PR :)
| } | ||
| } | ||
|
|
||
| // SAFETY: `init_state` does no access. The interesting safety checks are on the `SystemParamBuilder`. |
There was a problem hiding this comment.
This safety comment isn't clear enough IMO. Make sure to tie back to the safety requirements of SystemParam explicitly.
alice-i-cecile
left a comment
There was a problem hiding this comment.
Neutral on the API changes. As is, mildly useful, as it allows for the construction of more complex system param like ParamSet. Generally well-made: good docs and tests.
I genuinely don't understand the value of the Vec SystemParam, and would like to cut it unless there's strong motivation. If it's borderline, please split it out into its own PR.
I quite like all three follow-ups, but I'd like them to each be their own PRs.
P.S. SystemState::build_system is particularly interesting 🤔 Definitely feels like the sort of tool that will get used for shenanigans eventually.
Yeah I think it wouldn't make sense for this PR, I think it's something that would be needed. From the rust docs for 0.14 I was going off Query and Local both being the only things that implement BuildableSystemParam. I think it would be nice to have Res/ResMut also implement this trait. |
james-j-obrien
left a comment
There was a problem hiding this comment.
Implementation looks good, although I don't love how it looks aesthetically the utility for composition of paramsets is there.
I think using ParamBuilder in this way will be a little confusing for new users, there's nothing in this snippet that tells me what that identifier is there for so without knowing how the feature works it reads strangely:
let system = (
ParamBuilder,
LocalBuilder(10),
QueryParamBuilder::new(|builder| { builder.with::<B>(); }),
)
.build_state(&mut world)
.build_system(system);Maybe including "default" in there somewhere could help.
Although I see in follow up PRs you can also express it more explicitly like this:
ParamBuilder::local::<u64>()
So I suppose it's not a huge deal.
Yeah, I don't think I quite got the names right, but I couldn't think of better ones. I'm happy to rename things if you have a suggestion! |
…vyengine#14962) # Objective Make the documentation for `SystemParamBuilder` nicer by combining the tuple implementations into a single line of documentation. ## Solution Use `#[doc(fake_variadic)]` for `SystemParamBuilder` tuple impls.  (This got missed originally because bevyengine#14050 and bevyengine#14703 were open at the same time.)
Objective
Support more kinds of system params in buildable systems, such as a
ParamSetorVeccontaining buildable params or tuples of buildable params.Solution
Replace the
BuildableSystemParamtrait withSystemParamBuilderto make it easier to compose builders. Provide implementations for existing buildable params, plus tuples,ParamSet, andVec.Examples
Migration Guide
The API for
SystemBuilderhas changed. Instead of constructing a builder with a world and then adding params, you first create a tuple of param builders and then supply the world.Possible Future Work
Here are a few possible follow-up changes. I coded them up to prove that this API can support them, but they aren't necessary for this PR.