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.

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

Flexible ECS System Params #798

Merged
merged 12 commits into from
Nov 8, 2020
2 changes: 1 addition & 1 deletion crates/bevy_app/src/app_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::{
plugin::Plugin,
stage, startup_stage, PluginGroup, PluginGroupBuilder,
};
use bevy_ecs::{FromResources, IntoQuerySystem, Resources, System, World};
use bevy_ecs::{FromResources, IntoSystem, Resources, System, World};

/// Configure [App]s using the builder pattern
pub struct AppBuilder {
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_asset/src/assets.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use crate::{
update_asset_storage_system, Asset, AssetLoader, AssetServer, Handle, HandleId, RefChange,
};
use bevy_app::{prelude::Events, AppBuilder};
use bevy_ecs::{FromResources, IntoQuerySystem, ResMut};
use bevy_ecs::{FromResources, IntoSystem, ResMut};
use bevy_type_registry::RegisterType;
use bevy_utils::HashMap;
use crossbeam_channel::Sender;
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_asset/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ pub mod prelude {
}

use bevy_app::{prelude::Plugin, AppBuilder};
use bevy_ecs::IntoQuerySystem;
use bevy_ecs::IntoSystem;
use bevy_type_registry::RegisterType;

/// Adds support for Assets to an App. Assets are typed collections with change tracking, which are added as App Resources.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::{Diagnostic, DiagnosticId, Diagnostics};
use bevy_app::prelude::*;
use bevy_core::Time;
use bevy_ecs::{IntoQuerySystem, Res, ResMut};
use bevy_ecs::{IntoSystem, Res, ResMut};

/// Adds "frame time" diagnostic to an App, specifically "frame time", "fps" and "frame count"
#[derive(Default)]
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_diagnostic/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ impl Plugin for DiagnosticsPlugin {
app.init_resource::<Diagnostics>();
#[cfg(feature = "profiler")]
{
use bevy_ecs::IntoQuerySystem;
use bevy_ecs::IntoSystem;
app.add_resource::<Box<dyn bevy_ecs::Profiler>>(Box::new(
system_profiler::SystemProfiler::default(),
))
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_diagnostic/src/print_diagnostics_plugin.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use super::{Diagnostic, DiagnosticId, Diagnostics};
use bevy_app::prelude::*;
use bevy_core::{Time, Timer};
use bevy_ecs::{IntoQuerySystem, Res, ResMut};
use bevy_ecs::{IntoSystem, Res, ResMut};
use std::time::Duration;

/// An App Plugin that prints diagnostics to the console
Expand Down
94 changes: 93 additions & 1 deletion crates/bevy_ecs/hecs/macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@ use proc_macro::TokenStream;
use proc_macro2::{Span, TokenStream as TokenStream2};
use proc_macro_crate::crate_name;
use quote::quote;
use syn::{parse_macro_input, DeriveInput, Error, Ident, Index, Lifetime, Path, Result};
use syn::{
parse::ParseStream, parse_macro_input, Data, DataStruct, DeriveInput, Error, Field, Fields,
Ident, Index, Lifetime, Path, Result,
};

/// Implement `Bundle` for a monomorphic struct
///
Expand Down Expand Up @@ -332,3 +335,92 @@ pub fn impl_query_set(_input: TokenStream) -> TokenStream {

tokens
}

#[derive(Default)]
struct SystemParamFieldAttributes {
pub ignore: bool,
}

static SYSTEM_PARAM_ATTRIBUTE_NAME: &str = "system_param";

#[proc_macro_derive(SystemParam, attributes(system_param))]
pub fn derive_system_param(input: TokenStream) -> TokenStream {
let ast = parse_macro_input!(input as DeriveInput);
let fields = match &ast.data {
Data::Struct(DataStruct {
fields: Fields::Named(fields),
..
}) => &fields.named,
_ => panic!("expected a struct with named fields"),
Copy link
Contributor

@mvlabat mvlabat Nov 8, 2020

Choose a reason for hiding this comment

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

I just wanted to share how errors are designed in Legion derives:
https://github.com/amethyst/legion/blob/8fe4588d934abad8faffb54beebf415b4880773d/codegen/src/lib.rs#L173

I find it a pretty nice way to organize and make them more informative, especially in the case when you can point at a specific invalid argument and get such a compile-time error:
https://github.com/amethyst/legion/blob/8fe4588d934abad8faffb54beebf415b4880773d/tests/failures/value_argument.stderr

Probably you've already considered working on something like this in future. I don't want to suggest spending time on this right now, delaying this particular feature, but I hope this can serve as some inspiration for future improvements.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think thats one of the major selling points of their "proc macro" systems. I think derives like this don't benefit as much from that type of error handling because the logic is much more straightforward (ex: if these fields implement a trait, use it, otherwise fail). We'll already get decently informative errors for those cases. But yeah im definitely open to making improvements where we can.

};

let path_str = if crate_name("bevy").is_ok() {
"bevy::ecs"
} else {
"bevy_ecs"
};
let path: Path = syn::parse(path_str.parse::<TokenStream>().unwrap()).unwrap();

let field_attributes = fields
.iter()
.map(|field| {
(
field,
field
.attrs
.iter()
.find(|a| *a.path.get_ident().as_ref().unwrap() == SYSTEM_PARAM_ATTRIBUTE_NAME)
.map_or_else(SystemParamFieldAttributes::default, |a| {
syn::custom_keyword!(ignore);
let mut attributes = SystemParamFieldAttributes::default();
a.parse_args_with(|input: ParseStream| {
if input.parse::<Option<ignore>>()?.is_some() {
attributes.ignore = true;
}
Ok(())
})
.expect("invalid 'render_resources' attribute format");

attributes
}),
)
})
.collect::<Vec<(&Field, SystemParamFieldAttributes)>>();
let mut fields = Vec::new();
let mut field_types = Vec::new();
let mut ignored_fields = Vec::new();
let mut ignored_field_types = Vec::new();
for (field, attrs) in field_attributes.iter() {
if attrs.ignore {
ignored_fields.push(field.ident.as_ref().unwrap());
ignored_field_types.push(&field.ty);
} else {
fields.push(field.ident.as_ref().unwrap());
field_types.push(&field.ty);
}
}

let generics = ast.generics;
let (impl_generics, ty_generics, _where_clause) = generics.split_for_impl();

let struct_name = &ast.ident;

TokenStream::from(quote! {
impl #impl_generics #path::SystemParam for #struct_name#ty_generics {
fn init(system_state: &mut #path::SystemState, world: &#path::World, resources: &mut #path::Resources) {
#(<#field_types>::init(system_state, world, resources);)*
}

unsafe fn get_param(
system_state: &mut #path::SystemState,
world: &#path::World,
resources: &#path::Resources,
) -> Option<Self> {
Some(#struct_name {
#(#fields: <#field_types>::get_param(system_state, world, resources)?,)*
#(#ignored_fields: <#ignored_field_types>::default(),)*
})
}
}
})
}
2 changes: 1 addition & 1 deletion crates/bevy_ecs/hecs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,4 +98,4 @@ pub use lazy_static;
pub use query::Fetch;

#[cfg(feature = "macros")]
pub use bevy_hecs_macros::{impl_query_set, Bundle};
pub use bevy_hecs_macros::{impl_query_set, Bundle, SystemParam};
2 changes: 1 addition & 1 deletion crates/bevy_ecs/hecs/src/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ impl_or_query!(Q1, Q2, Q3, Q4, Q5, Q6, Q7, Q8, Q9, Q10);
/// .collect::<Vec<_>>();
/// assert_eq!(components, &[(false, 457)]);
/// ```
pub struct Or<T>(PhantomData<T>);
pub struct Or<T>(pub T);
//pub struct Or<Q1, Q2, Q3>(PhantomData<(Q1, Q2, Q3)>);

#[doc(hidden)]
Expand Down
11 changes: 11 additions & 0 deletions crates/bevy_ecs/hecs/src/world.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,7 @@ impl World {
/// assert!(entities.contains(&(a, 123, true)));
/// assert!(entities.contains(&(b, 456, false)));
/// ```
#[inline]
pub fn query<Q: Query>(&self) -> QueryIter<'_, Q>
where
Q::Fetch: ReadOnlyFetch,
Expand Down Expand Up @@ -281,13 +282,15 @@ impl World {
/// assert!(entities.contains(&(a, 123, true)));
/// assert!(entities.contains(&(b, 456, false)));
/// ```
#[inline]
pub fn query_mut<Q: Query>(&mut self) -> QueryIter<'_, Q> {
// SAFE: unique mutable access
unsafe { self.query_unchecked() }
}

/// Like `query`, but instead of returning a single iterator it returns a "batched iterator",
/// where each batch is `batch_size`. This is generally used for parallel iteration.
#[inline]
pub fn query_batched<Q: Query>(&self, batch_size: usize) -> BatchedIter<'_, Q>
where
Q::Fetch: ReadOnlyFetch,
Expand All @@ -298,6 +301,7 @@ impl World {

/// Like `query`, but instead of returning a single iterator it returns a "batched iterator",
/// where each batch is `batch_size`. This is generally used for parallel iteration.
#[inline]
pub fn query_batched_mut<Q: Query>(&mut self, batch_size: usize) -> BatchedIter<'_, Q> {
// SAFE: unique mutable access
unsafe { self.query_batched_unchecked(batch_size) }
Expand All @@ -316,6 +320,7 @@ impl World {
/// # Safety
/// This does not check for mutable query correctness. To be safe, make sure mutable queries
/// have unique access to the components they query.
#[inline]
pub unsafe fn query_unchecked<Q: Query>(&self) -> QueryIter<'_, Q> {
QueryIter::new(&self.archetypes)
}
Expand Down Expand Up @@ -347,6 +352,7 @@ impl World {
/// let (number, flag) = world.query_one::<(&i32, &bool)>(a).unwrap();
/// assert_eq!(*number, 123);
/// ```
#[inline]
pub fn query_one<Q: Query>(
&self,
entity: Entity,
Expand All @@ -372,6 +378,7 @@ impl World {
/// if *flag { *number *= 2; }
/// assert_eq!(*number, 246);
/// ```
#[inline]
pub fn query_one_mut<Q: Query>(
&mut self,
entity: Entity,
Expand All @@ -387,6 +394,7 @@ impl World {
/// # Safety
/// This does not check for mutable query correctness. To be safe, make sure mutable queries
/// have unique access to the components they query.
#[inline]
pub unsafe fn query_one_unchecked<Q: Query>(
&self,
entity: Entity,
Expand All @@ -399,6 +407,7 @@ impl World {
}

/// Borrow the `T` component of `entity`
#[inline]
pub fn get<T: Component>(&self, entity: Entity) -> Result<&'_ T, ComponentError> {
unsafe {
let loc = self.entities.get(entity)?;
Expand All @@ -414,6 +423,7 @@ impl World {
}

/// Mutably borrow the `T` component of `entity`
#[inline]
pub fn get_mut<T: Component>(&mut self, entity: Entity) -> Result<Mut<'_, T>, ComponentError> {
// SAFE: uniquely borrows world
unsafe { self.get_mut_unchecked(entity) }
Expand All @@ -434,6 +444,7 @@ impl World {
/// # Safety
/// This does not check for mutable access correctness. To be safe, make sure this is the only
/// thing accessing this entity's T component.
#[inline]
pub unsafe fn get_mut_unchecked<T: Component>(
&self,
entity: Entity,
Expand Down
6 changes: 2 additions & 4 deletions crates/bevy_ecs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,8 @@ pub use world::*;

pub mod prelude {
pub use crate::{
resource::{ChangedRes, FromResources, Local, OrRes, Res, ResMut, Resource, Resources},
system::{
Commands, IntoForEachSystem, IntoQuerySystem, IntoThreadLocalSystem, Query, System,
},
resource::{ChangedRes, FromResources, Local, Res, ResMut, Resource, Resources},
system::{Commands, IntoSystem, IntoThreadLocalSystem, Query, System},
world::WorldBuilderSource,
Added, Bundle, Changed, Component, Entity, Mut, Mutated, Or, QuerySet, Ref, RefMut, With,
Without, World,
Expand Down
Loading