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

[Merged by Bors] - Obviate the need for RunSystem, and remove it #3817

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion crates/bevy_ecs/macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
#[doc(hidden)]
#fetch_struct_visibility struct #fetch_struct_name<TSystemParamState, #punctuated_generic_idents> {
state: TSystemParamState,
marker: std::marker::PhantomData<(#punctuated_generic_idents)>
marker: std::marker::PhantomData<fn()->(#punctuated_generic_idents)>
}

unsafe impl<TSystemParamState: #path::system::SystemParamState, #punctuated_generics> #path::system::SystemParamState for #fetch_struct_name<TSystemParamState, #punctuated_generic_idents> {
Expand Down
79 changes: 1 addition & 78 deletions crates/bevy_ecs/src/system/function_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::{
query::{Access, FilteredAccessSet},
system::{
check_system_change_tick, ReadOnlySystemParamFetch, System, SystemParam, SystemParamFetch,
SystemParamItem, SystemParamState,
SystemParamState,
},
world::{World, WorldId},
};
Expand Down Expand Up @@ -46,11 +46,6 @@ impl SystemMeta {
pub fn set_non_send(&mut self) {
self.is_send = false;
}

#[inline]
pub(crate) fn check_change_tick(&mut self, change_tick: u32) {
check_system_change_tick(&mut self.last_change_tick, change_tick, self.name.as_ref());
}
}

// TODO: Actually use this in FunctionSystem. We should probably only do this once Systems are constructed using a World reference
Expand Down Expand Up @@ -194,10 +189,6 @@ impl<Param: SystemParam> SystemState<Param> {
self.world_id == world.id()
}

pub(crate) fn new_archetype(&mut self, archetype: &Archetype) {
self.param_state.new_archetype(archetype, &mut self.meta);
}

fn validate_world_and_update_archetypes(&mut self, world: &World) {
assert!(self.matches_world(world), "Encountered a mismatched World. A SystemState cannot be used with Worlds other than the one it was created with.");
let archetypes = world.archetypes();
Expand Down Expand Up @@ -236,74 +227,6 @@ impl<Param: SystemParam> SystemState<Param> {
}
}

/// A trait for defining systems with a [`SystemParam`] associated type.
///
/// This facilitates the creation of systems that are generic over some trait
/// and that use that trait's associated types as `SystemParam`s.
pub trait RunSystem: Send + Sync + 'static {
/// The `SystemParam` type passed to the system when it runs.
type Param: SystemParam;

/// Runs the system.
fn run(param: SystemParamItem<Self::Param>);

/// Creates a concrete instance of the system for the specified `World`.
fn system(world: &mut World) -> ParamSystem<Self::Param> {
ParamSystem {
run: Self::run,
state: SystemState::new(world),
}
}
}

pub struct ParamSystem<P: SystemParam> {
state: SystemState<P>,
run: fn(SystemParamItem<P>),
}

impl<P: SystemParam + 'static> System for ParamSystem<P> {
type In = ();

type Out = ();

fn name(&self) -> Cow<'static, str> {
self.state.meta().name.clone()
}

fn new_archetype(&mut self, archetype: &Archetype) {
self.state.new_archetype(archetype);
}

fn component_access(&self) -> &Access<ComponentId> {
self.state.meta().component_access_set.combined_access()
}

fn archetype_component_access(&self) -> &Access<ArchetypeComponentId> {
&self.state.meta().archetype_component_access
}

fn is_send(&self) -> bool {
self.state.meta().is_send()
}

unsafe fn run_unsafe(&mut self, _input: Self::In, world: &World) -> Self::Out {
let param = self.state.get_unchecked_manual(world);
(self.run)(param);
}

fn apply_buffers(&mut self, world: &mut World) {
self.state.apply(world);
}

fn initialize(&mut self, _world: &mut World) {
// already initialized by nature of the SystemState being constructed
}

fn check_change_tick(&mut self, change_tick: u32) {
self.state.meta.check_change_tick(change_tick);
}
}

/// Conversion trait to turn something into a [`System`].
///
/// Use this to get a system from a function. Also note that every system implements this trait as
Expand Down
125 changes: 125 additions & 0 deletions crates/bevy_ecs/src/system/system_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1254,6 +1254,131 @@ pub mod lifetimeless {
pub type SCommands = crate::system::Commands<'static, 'static>;
}

/// A helper for using system parameters in generic contexts
///
/// This type is a [`SystemParam`] adapter which always has
/// `Self::Fetch::Item == Self` (ignoring lifetimes for brevity),
/// no matter the argument [`SystemParam`] (`P`) (other than
/// that `P` must be `'static`)
Copy link
Member Author

Choose a reason for hiding this comment

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

This restriction is unfortunate, but pretty fundamental as far as I can tell.
This is because we need be able to refer to P in SystemParamState, which reasonably needs to be 'static

///
/// This makes it useful for having arbitrary [`SystemParam`] type arguments
/// to function systems, or for generic types using the [`derive@SystemParam`]
/// derive:
///
/// ```
/// # use bevy_ecs::prelude::*;
/// use bevy_ecs::system::{SystemParam, StaticSystemParam};
/// #[derive(SystemParam)]
/// struct GenericParam<'w,'s, T: SystemParam + 'static> {
/// field: StaticSystemParam<'w, 's, T>,
/// }
/// fn do_thing_generically<T: SystemParam + 'static>(t: StaticSystemParam<T>) {}
///
/// fn check_always_is_system<T: SystemParam + 'static>(){
/// bevy_ecs::system::assert_is_system(do_thing_generically::<T>);
/// }
/// ```
/// Note that in a real case you'd generally want
/// additional bounds on `P`, for your use of the parameter
/// to have a reason to be generic.
///
/// For example, using this would allow a type to be generic over
/// whether a resource is accessed mutably or not, with
/// impls being bounded on [`P: Deref<Target=MyType>`](Deref), and
/// [`P: DerefMut<Target=MyType>`](DerefMut) depending on whether the
/// method requires mutable access or not.
Comment on lines +1285 to +1289
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 plan on trying to use this trick for Assets (to remove the fact that the events are seperate in an ergonomic way)

That will be a seperate PR.

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've done some more thinking about this, and I realise that none of the reading only Assets methods access the events.
I don't actually think that PR would need this struct/this trick. Still, this is much nicer than RunSystem

///
/// The method which doesn't use this type will not compile:
/// ```compile_fail
/// # use bevy_ecs::prelude::*;
/// # use bevy_ecs::system::{SystemParam, StaticSystemParam};
///
/// fn do_thing_generically<T: SystemParam + 'static>(t: T) {}
///
/// #[derive(SystemParam)]
/// struct GenericParam<'w,'s, T: SystemParam> {
/// field: T,
/// #[system_param(ignore)]
/// // Use the lifetimes, as the `SystemParam` derive requires them
/// phantom: core::marker::PhantomData<&'w &'s ()>
/// }
/// # fn check_always_is_system<T: SystemParam + 'static>(){
/// # bevy_ecs::system::assert_is_system(do_thing_generically::<T>);
/// # }
/// ```
///
pub struct StaticSystemParam<'w, 's, P: SystemParam>(SystemParamItem<'w, 's, P>);

impl<'w, 's, P: SystemParam> Deref for StaticSystemParam<'w, 's, P> {
type Target = SystemParamItem<'w, 's, P>;

fn deref(&self) -> &Self::Target {
&self.0
}
}

impl<'w, 's, P: SystemParam> DerefMut for StaticSystemParam<'w, 's, P> {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
}
}

impl<'w, 's, P: SystemParam> StaticSystemParam<'w, 's, P> {
/// Get the value of the parameter
pub fn into_inner(self) -> SystemParamItem<'w, 's, P> {
self.0
}
}

/// The [`SystemParamState`] of [`SystemChangeTick`].
pub struct StaticSystemParamState<S, P>(S, PhantomData<fn() -> P>);
DJMcNab marked this conversation as resolved.
Show resolved Hide resolved

// Safe: This doesn't add any more reads, and the delegated fetch confirms it
unsafe impl<'w, 's, S: ReadOnlySystemParamFetch, P> ReadOnlySystemParamFetch
for StaticSystemParamState<S, P>
{
}

impl<'world, 'state, P: SystemParam + 'static> SystemParam
for StaticSystemParam<'world, 'state, P>
{
type Fetch = StaticSystemParamState<P::Fetch, P>;
}

impl<'world, 'state, S: SystemParamFetch<'world, 'state>, P: SystemParam + 'static>
SystemParamFetch<'world, 'state> for StaticSystemParamState<S, P>
where
P: SystemParam<Fetch = S>,
{
type Item = StaticSystemParam<'world, 'state, P>;

unsafe fn get_param(
state: &'state mut Self,
system_meta: &SystemMeta,
world: &'world World,
change_tick: u32,
) -> Self::Item {
// Safe: We properly delegate SystemParamState
StaticSystemParam(S::get_param(&mut state.0, system_meta, world, change_tick))
}
}

unsafe impl<'w, 's, S: SystemParamState, P: SystemParam + 'static> SystemParamState
for StaticSystemParamState<S, P>
{
fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self {
Self(S::init(world, system_meta), PhantomData)
}

fn new_archetype(&mut self, archetype: &Archetype, system_meta: &mut SystemMeta) {
self.0.new_archetype(archetype, system_meta)
}

fn apply(&mut self, world: &mut World) {
self.0.apply(world)
}
}

#[cfg(test)]
mod tests {
use super::SystemParam;
Expand Down
67 changes: 28 additions & 39 deletions crates/bevy_render/src/render_asset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use bevy_app::{App, Plugin};
use bevy_asset::{Asset, AssetEvent, Assets, Handle};
use bevy_ecs::{
prelude::*,
system::{lifetimeless::*, RunSystem, SystemParam, SystemParamItem},
system::{StaticSystemParam, SystemParam, SystemParamItem},
};
use bevy_utils::{HashMap, HashSet};
use std::marker::PhantomData;
Expand Down Expand Up @@ -55,13 +55,12 @@ impl<A: RenderAsset> Default for RenderAssetPlugin<A> {
impl<A: RenderAsset> Plugin for RenderAssetPlugin<A> {
fn build(&self, app: &mut App) {
if let Ok(render_app) = app.get_sub_app_mut(RenderApp) {
let prepare_asset_system = PrepareAssetSystem::<A>::system(&mut render_app.world);
render_app
.init_resource::<ExtractedAssets<A>>()
.init_resource::<RenderAssets<A>>()
.init_resource::<PrepareNextFrameAssets<A>>()
.add_system_to_stage(RenderStage::Extract, extract_render_asset::<A>)
.add_system_to_stage(RenderStage::Prepare, prepare_asset_system);
.add_system_to_stage(RenderStage::Prepare, prepare_assets::<A>);
}
}
}
Expand Down Expand Up @@ -122,14 +121,6 @@ fn extract_render_asset<A: RenderAsset>(
});
}

/// Specifies all ECS data required by [`PrepareAssetSystem`].
pub type RenderAssetParams<R> = (
SResMut<ExtractedAssets<R>>,
SResMut<RenderAssets<R>>,
SResMut<PrepareNextFrameAssets<R>>,
<R as RenderAsset>::Param,
);

// TODO: consider storing inside system?
/// All assets that should be prepared next frame.
pub struct PrepareNextFrameAssets<A: RenderAsset> {
Expand All @@ -146,38 +137,36 @@ impl<A: RenderAsset> Default for PrepareNextFrameAssets<A> {

/// This system prepares all assets of the corresponding [`RenderAsset`] type
/// which where extracted this frame for the GPU.
pub struct PrepareAssetSystem<R: RenderAsset>(PhantomData<R>);

impl<R: RenderAsset> RunSystem for PrepareAssetSystem<R> {
type Param = RenderAssetParams<R>;

fn run(
(mut extracted_assets, mut render_assets, mut prepare_next_frame, mut param): SystemParamItem<Self::Param>,
) {
let mut queued_assets = std::mem::take(&mut prepare_next_frame.assets);
for (handle, extracted_asset) in queued_assets.drain(..) {
match R::prepare_asset(extracted_asset, &mut param) {
Ok(prepared_asset) => {
render_assets.insert(handle, prepared_asset);
}
Err(PrepareAssetError::RetryNextUpdate(extracted_asset)) => {
prepare_next_frame.assets.push((handle, extracted_asset));
}
fn prepare_assets<R: RenderAsset>(
mut extracted_assets: ResMut<ExtractedAssets<R>>,
mut render_assets: ResMut<RenderAssets<R>>,
mut prepare_next_frame: ResMut<PrepareNextFrameAssets<R>>,
param: StaticSystemParam<<R as RenderAsset>::Param>,
) {
let mut param = param.into_inner();
let mut queued_assets = std::mem::take(&mut prepare_next_frame.assets);
for (handle, extracted_asset) in queued_assets.drain(..) {
match R::prepare_asset(extracted_asset, &mut param) {
Ok(prepared_asset) => {
render_assets.insert(handle, prepared_asset);
}
Err(PrepareAssetError::RetryNextUpdate(extracted_asset)) => {
prepare_next_frame.assets.push((handle, extracted_asset));
}
}
}

for removed in std::mem::take(&mut extracted_assets.removed) {
render_assets.remove(&removed);
}
for removed in std::mem::take(&mut extracted_assets.removed) {
render_assets.remove(&removed);
}

for (handle, extracted_asset) in std::mem::take(&mut extracted_assets.extracted) {
match R::prepare_asset(extracted_asset, &mut param) {
Ok(prepared_asset) => {
render_assets.insert(handle, prepared_asset);
}
Err(PrepareAssetError::RetryNextUpdate(extracted_asset)) => {
prepare_next_frame.assets.push((handle, extracted_asset));
}
for (handle, extracted_asset) in std::mem::take(&mut extracted_assets.extracted) {
match R::prepare_asset(extracted_asset, &mut param) {
Ok(prepared_asset) => {
render_assets.insert(handle, prepared_asset);
}
Err(PrepareAssetError::RetryNextUpdate(extracted_asset)) => {
prepare_next_frame.assets.push((handle, extracted_asset));
}
}
}
Expand Down
Loading