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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Lift the 16-field limit from the SystemParam derive #6867

Closed
wants to merge 8 commits into from
Closed

[Merged by Bors] - Lift the 16-field limit from the SystemParam derive #6867

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d5d0a2a
relax 16-field limitation on `SystemParam` derive
JoJoJet Dec 6, 2022
768b8e1
add a test case
JoJoJet Dec 6, 2022
19da0e0
use a tuple impl for `get_param`
JoJoJet Dec 7, 2022
2c8518c
use field names for locals
JoJoJet Dec 7, 2022
c8d64f0
Merge remote-tracking branch 'upstream/main' into system-param-limit
JoJoJet Dec 12, 2022
17568f8
remove `field_indices`
JoJoJet Dec 14, 2022
5aabe45
Merge remote-tracking branch 'upstream/main' into system-param-limit
JoJoJet Dec 21, 2022
513afbf
cargo fmt
JoJoJet Dec 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 21 additions & 5 deletions crates/bevy_ecs/macros/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use proc_macro2::Span;
use quote::{format_ident, quote};
use syn::{
parse::{Parse, ParseStream},
parse_macro_input,
parse_macro_input, parse_quote,
punctuated::Punctuated,
spanned::Spanned,
token::Comma,
Expand Down Expand Up @@ -359,8 +359,8 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
)
})
.collect::<Vec<(&Field, SystemParamFieldAttributes)>>();
let mut field_locals = Vec::new();
let mut fields = Vec::new();
let mut field_indices = Vec::new();
let mut field_types = Vec::new();
let mut ignored_fields = Vec::new();
let mut ignored_field_types = Vec::new();
Expand All @@ -369,6 +369,7 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
ignored_fields.push(field.ident.as_ref().unwrap());
ignored_field_types.push(&field.ty);
} else {
field_locals.push(format_ident!("f{i}"));
let i = Index::from(i);
fields.push(
field
Expand All @@ -378,7 +379,6 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
.unwrap_or_else(|| quote! { #i }),
);
field_types.push(&field.ty);
field_indices.push(i);
}
}

Expand Down Expand Up @@ -424,6 +424,19 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
_ => unreachable!(),
}));

let mut tuple_types: Vec<_> = field_types.iter().map(|x| quote! { #x }).collect();
let mut tuple_patterns: Vec<_> = field_locals.iter().map(|x| quote! { #x }).collect();

// If the number of fields exceeds the 16-parameter limit,
// fold the fields into tuples of tuples until we are below the limit.
const LIMIT: usize = 16;
while tuple_types.len() > LIMIT {
let end = Vec::from_iter(tuple_types.drain(..LIMIT));
tuple_types.push(parse_quote!( (#(#end,)*) ));
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit difficult to follow this but does this make a tuple of tuples? Wouldn't that just raise the limit to 256 instead of removing it?

Copy link
Member

Choose a reason for hiding this comment

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

Oh wait I think I see what this is doing. My god this is going to have a lot of codgen for anything more than 32 entries, but you probably already should be expecting that with a ParamSet that big.


let end = Vec::from_iter(tuple_patterns.drain(..LIMIT));
tuple_patterns.push(parse_quote!( (#(#end,)*) ));
}
// Create a where clause for the `ReadOnlySystemParam` impl.
// Ensure that each field implements `ReadOnlySystemParam`.
let mut read_only_generics = generics.clone();
Expand All @@ -448,7 +461,7 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {

#[doc(hidden)]
type State<'w, 's, #punctuated_generic_idents> = FetchState<
(#(<#field_types as #path::system::SystemParam>::State,)*),
(#(<#tuple_types as #path::system::SystemParam>::State,)*),
#punctuated_generic_idents
>;

Expand Down Expand Up @@ -484,8 +497,11 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
world: &'w #path::world::World,
change_tick: u32,
) -> Self::Item<'w, 's> {
let (#(#tuple_patterns,)*) = <
<(#(#tuple_types,)*) as #path::system::SystemParam>::State as #path::system::SystemParamState
>::get_param(&mut state.state, system_meta, world, change_tick);
#struct_name {
#(#fields: <<#field_types as #path::system::SystemParam>::State as #path::system::SystemParamState>::get_param(&mut state.state.#field_indices, system_meta, world, change_tick),)*
JoJoJet marked this conversation as resolved.
Show resolved Hide resolved
#(#fields: #field_locals,)*
#(#ignored_fields: <#ignored_field_types>::default(),)*
}
}
Expand Down
29 changes: 29 additions & 0 deletions crates/bevy_ecs/src/system/system_param.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1661,6 +1661,35 @@ mod tests {
_local: Local<'s, T>,
}

#[derive(Resource)]
pub struct R<const I: usize>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why you added this over re-using UnitParam?

Copy link
Member Author

Choose a reason for hiding this comment

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

I want each field to be a distinct type, to make sure the derive macro doesn't mess up the order of the fields.


#[derive(SystemParam)]
pub struct LongParam<'w> {
_r0: Res<'w, R<0>>,
_r1: Res<'w, R<1>>,
_r2: Res<'w, R<2>>,
_r3: Res<'w, R<3>>,
_r4: Res<'w, R<4>>,
_r5: Res<'w, R<5>>,
_r6: Res<'w, R<6>>,
_r7: Res<'w, R<7>>,
_r8: Res<'w, R<8>>,
_r9: Res<'w, R<9>>,
_r10: Res<'w, R<10>>,
_r11: Res<'w, R<11>>,
_r12: Res<'w, R<12>>,
_r13: Res<'w, R<13>>,
_r14: Res<'w, R<14>>,
_r15: Res<'w, R<15>>,
_r16: Res<'w, R<16>>,
}

#[allow(dead_code)]
fn long_system(_param: LongParam) {
crate::system::assert_is_system(long_system);
}

#[derive(SystemParam)]
pub struct UnitParam;

Expand Down