Indepth Optional System Params

[JonahPlusPlus]

Objective

• There is boilerplate for T and Option<T> system params where you have to implement SystemParamState and SystemParamFetch for both types.

Solution

• This removes that boilerplate by making it necessary to only implement SystemParam or OptionalSystemParam and their respective traits.

[joseph-gio]

I'm always a fan of PRs that remove more than they add :)

[joseph-gio]

This needs a better panic message.

[JonahPlusPlus]

Okay, I changed it to "Failed to get system param <type_name of T::Item>"

[joseph-gio]

Using Option<T> for the state doesn't make sense here, since it will always be Some. It would be better to just use a simple wrapper type for the state -- something like

pub struct OptionalState<T: OptionalSystemParamState>(T);

[JonahPlusPlus]

@JoJoJet Actually, I think it's better this way. If I changed it to something like OptionState, then you would have to import it to create your optional system params, like OptionState<ResState<Foo>>, which I think is less ergonomic than Option<ResState<Foo>>.

[joseph-gio]

I don't think that is true? Implementing an optional system param should just look like

struct MyParam(...);

impl OptionalSystemParam for MyParam {
    type Fetch = MyParamState;
}

struct MyParamState(...);

impl<'w, 's> OptionalSystemParamFetch<'w, 's> for MyParamState {
    type Item = MyParam;
    ...
}

The blanket impl would fill in OptionState<MyParamState> for you.

[JonahPlusPlus]

@JoJoJet Here is an example of an optional system param in a program that I verified works with this branch:

use bevy::{
  prelude::*,
  ecs::system::{ResState, SystemParam, SystemParamState, SystemParamFetch, SystemMeta, OptionalSystemParam, OptionalSystemParamFetch}
};

fn main() {
  App::new()
    .insert_resource(Bar {something: 653})
    .add_system(foo)
    .run();
}

#[derive(Resource)]
struct Bar {
  something: u32
}

#[derive(Debug)]
struct Foo<'w> {
  value: &'w u32
}

impl<'w> OptionalSystemParam for Foo<'w> {
    type Fetch = FooState;
}

struct FooState {
    res_state: Option<ResState<Bar>>,
}

unsafe impl SystemParamState for FooState {
  fn init(world: &mut World, system_meta: &mut bevy::ecs::system::SystemMeta) -> Self {
    Self {
      res_state: Some(ResState::init(world, system_meta))
    }
  }
}

impl<'w, 's> OptionalSystemParamFetch<'w, 's> for FooState {
  type Item = Foo<'w>;

  #[inline]
  unsafe fn get_param(
    state: &'s mut Self,
    system_meta: &SystemMeta,
    world: &'w World,
    change_tick: u32,
  ) -> Option<Self::Item> {
    let bar = <<Option<Res<Bar>> as SystemParam>::Fetch as SystemParamFetch>::get_param(&mut state.res_state, system_meta, world, change_tick);
    bar.map(|b| {
      let b = b.into_inner();
      Foo {
        value: &b.something
      }
    })
  }
}

fn foo(foo: Option<Foo>) {
  match foo {
    Some(f) => println!("{f:?}"),
    None => println!("Foo not found!")
  }
}

As you can see, res_state is Option<ResState<Bar>> and I call SystemParamFetch::get_param on it. This only works if Option<T> implements SystemParamState because SystemParamFetch requires it.

If I were to change Option<T> to OptionState<T>, then I would figure out some way to convert Option<T> to OptionState<T>, probably through an OptionalSystemParamState trait, which seems like a bunch of trouble without any apparent benefit.

[joseph-gio]

If I were to change Option<T> to OptionState<T>, then I would figure out some way to convert Option<T> to OptionState<T>

Why would you need to do this?

[JonahPlusPlus]

Because if OptionState<T> was used, then you would have to say res_state: OptionState<ResState<Bar>>, which isn't ergonomic, so Option<ResState<Bar>> would need some way to become OptionState<ResState<Bar>>, which seems like more trouble than it's worth. I'm against adding another trait or struct when the current solution works just fine.

[joseph-gio]

if OptionState<T> was used, then you would have to say res_state: OptionState<ResState<Bar>>, which isn't ergonomic

I suggest we change it from Option<ResState<T>> to OptionState<Res<T>>. There isn't any loss in ergonomics IMO. Having to name the OptionState type like this at all is quite rare, so I don't see any issue with having to import it.

[JonahPlusPlus]

Okay, I think this now looks good. I'll just wait for James' feedback before merging.

[JonahPlusPlus]

Okay, I changed it to "Failed to get system param <type_name of T::Item>"

[JonahPlusPlus]

@JoJoJet Actually, I think it's better this way. If I changed it to something like OptionState, then you would have to import it to create your optional system params, like OptionState<ResState<Foo>>, which I think is less ergonomic than Option<ResState<Foo>>.

[JonahPlusPlus]

Ok, I switched it from Option<T> to OptionState<T>, since @JoJoJet was right, it does better reflect the fact that it is the state of an option, not an optional state.

I also used OptionState to implement ReadOnlySystemParamFetch for all Option<T::Fetch: ReadOnlySystemParamFetch>.