SystemRunner param and macro syntax

[ecoskey]

Objective

resolves #16680

Add a new system param for running systems inside other systems. Also, I've included some macros for nice syntax on top.

I'm pretty proud of how nice I was able to make this, but there's still a bit of work to do, especially around generic code :)

Testing

• Ran examples
• Tried to migrate existing pipe and map impls, but failed. This PR is pretty robust for most use cases but isn't fully ready for generic code yet. In particular, it's difficult to make sure the input types match (often have to wrap with StaticSystemInput) and ReadOnlySystem bound is inferred correctly when using for run conditions. These only really matter for combinators like pipe and map though, since otherwise they're run exactly the same.

---

Showcase

Click to view showcase

fn count_a(a: Query<&A>) -> u32 {
    a.count()
}

fn count_b(b: Query<&B>) -> u32 {
    b.count()
}

let get_sum = (
    ParamBuilder::system(count_a),
    ParamBuilder::system(count_b)
)
.build_system(
    |mut run_a: SystemRunner<(), u32>, mut run_b: SystemRunner<(), u32>| -> Result<u32, RunSystemError> {
        let a = run_a.run()?;
        let b = run_b.run()?;
        Ok(a + b)
    }
);

let get_sum = compose! {
    || -> Result<u32, RunSystemError> {
        let a = run!(count_a)?;
        let b = run!(count_b)?;
        Ok(a + b)
    }
}

[chescock]

This is really cool!

I think this will close #16680, so you might want to link that in the PR description.

I left a lot of comments, but they're mostly style nitpicks and brainstorming for future possibilities. The only thing that I think really needs attention is the map_err(RunSystemError::Skipped) part, since that will silently ignore missing resources. And the CI failure :).

[chescock]

Is this here only to support with_name? (It's also used in get_last_run/set_last_run, but I think those are only supposed to be valid on initialized systems.) It might be better to just store a DebugName. If the builder is large, this could wind up being the largest variant and taking up space even after the system is built.

[chescock]

The other CI failure is because it can't tell whether the return type is u32 or Result<u32>. You can supply it like

Suggested change

	/// .build_system(
	/// .build_system::<_, u32, _, _>(

But it might look nicer to run the system and annotate the result, like

/// let result: usize = world.run_system_once(get_sum).unwrap();

[ecoskey]

how does add_systems infer which to use?

[chescock]

how does add_systems infer which to use?

It just always requires Out = () for systems and Out = bool for conditions.

[ecoskey]

Oh yeah true. And the other combinators can set it explicitly, sounds good.

[chescock]

Would it make sense to accept a function definition instead of or in addition to a closure? That would make it easier for these systems to have names. Something like

compose! {
    fn system_name()  -> Result<(), RunSystemError> {
        run!(system_a)
    }
}

For that matter, would it make sense to use an attribute macro, like

#[compose]
fn system_name() -> Result<(), RunSystemError> {
    run!(system_a)
}

?

... Oh, maybe not, because what you'd really want that to expand to is const system_name: impl System = const { ... };, but there's no way to write the type for the const.

[chescock]

• Tried to migrate existing pipe and map impls, but failed. This PR is pretty robust for most use cases but isn't fully ready for generic code yet. In particular, it's difficult to make sure the input types match (often have to wrap with StaticSystemInput) and ReadOnlySystem bound is inferred correctly when using for run conditions. These only really matter for combinators like pipe and map though, since otherwise they're run exactly the same.

I think I got pipe and and mostly working on this branch! I had to add lots of 'static annotations, but I think those are mostly harmless.

As you mentioned, the big problem is that the resulting systems take StaticSystemInput<In> instead of In, which means they can't be used in the schedule because that expects In = () and not In = StaticSystemInput<()>. Maybe we could create a wrapper type that converts a System<In = StaticSystemInput<In>> to a System<In = In>?

pub trait IntoSystem<In: SystemInput, Out, Marker>: Sized {
// ...
    fn pipe2<B, BIn, BOut, MarkerB>(
        self,
        system: B,
    ) -> impl System<In = StaticSystemInput<'static, In>, Out = BOut>
    where
        Out: 'static,
        B: IntoSystem<BIn, BOut, MarkerB> + 'static,
        for<'a> BIn: SystemInput<Inner<'a> = Out> + 'static,
        In: 'static,
        BOut: 'static,
        Marker: 'static,
        MarkerB: 'static,
        Self: 'static,
    {
        compose_with!(
            |StaticSystemInput(input): StaticSystemInput<In>| -> Result<BOut, RunSystemError> {
                let value = run!(self, input)?;
                run!(system, value)
            }
        )
    }

pub trait SystemCondition<Marker, In: SystemInput = ()>:
// ...
    fn and2<M: 'static, C: SystemCondition<M, In> + 'static>(
        self,
        and: C,
    ) -> impl ReadOnlySystem<In = StaticSystemInput<'static, In>, Out = bool>
    where
        for<'a> In: SystemInput<Inner<'a>: Copy> + 'static,
        Self: 'static,
        Marker: 'static,
    {
        compose_with!(
            |StaticSystemInput(input): StaticSystemInput<In>| -> Result<bool, RunSystemError> {
                Ok(run!(self, input)? && run!(and, input)?)
            }
        )
    }

[ecoskey]

Thanks for the input! I'll spin up a few PRs for BuilderSystem and RemapInputSystem (or whatever we call it) and start cleaning things up tonight or tomorrow

Also I realized SystemInput::unwrap is probably unnecessary since you can just destructure StaticSystemInput :P

[ecoskey]

rebased and ready for review! :)

[chescock]

Oh, yay!

Before I approve, a few of my comments from my earlier review still seem to apply:

https://github.com/bevyengine/bevy/pull/21811/changes#r2518521366
https://github.com/bevyengine/bevy/pull/21811/changes#r2518563988
https://github.com/bevyengine/bevy/pull/21811/changes#r2518608196

I don't mean for any of them to be blocking, but since you thumbs-upped some of them I thought you intended to make some changes there.

[chescock]

Hmm, another option here is to validate the inner param inside SystemRunner::validate_param. So if the inner system takes a Single or If<Res> then we'd skip the outer system.

I think that would be more powerful in general. If you want to run the outer system unconditionally, that's still possible by taking system: Result<SystemRunner<_>, SystemParamValidationError> and then doing system?.run()?.

But the existing combinators all want lazy validation, so maybe nobody will ever want eager validation. And that Result type is verbose and hard to discover. So maybe we should leave it like this to make lazy validation convenient, even if it makes eager validation impossible.

[kfc35]

I don’t feel particularly qualified to land an approval, but happy to promote this for reviews once chescock’s review comments have been all resolved.

I read and learned some things, and didn’t find any glaring issues with any of the non macro code