-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Add Join system combinator, allow for multiple multiple input piping #8715
Conversation
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
|
||
impl<In, A, B> Combine<A, B> for CartesianProduct | ||
where | ||
In: Copy, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bound feels incorrect: if we're joining the input we should just be able to pass it by value all the way through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value gets passed to the first system but not returned, so in order to pass it to the second system we have to be able to copy or clone it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I've tried to remove the Copy, but wasn't able to
examples/ecs/system_piping.rs
Outdated
@@ -56,6 +58,26 @@ fn handler_system(In(result): In<Result<usize, ParseIntError>>) { | |||
} | |||
} | |||
|
|||
fn calc1() -> i32 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably try an make these example a little more useful / representative. I'll chew on simple demos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My original use case was for a puzzle game where a couple of individual systems queried the puzzle to see if the puzzle was solved (think of checking rows and columns for a sudoku) and then return a boolean if the requirement was met.
The puzzle is solved if all the systems returned true
. Technically this can also work with the and
combinators, but if you also wanted to display to the user which condition wasn't met, those combinators wouldn't work and you'd need a specific handler system.
I'm in favor of this: it greatly extends the expressiveness of this piping API in intuitive ways. I'd ultimately like to see this paired as a
Agreed that this is the optimal API: that would lean on the
These will end up running sequentially, although I'm reasonably sure you won't be able to write multiple input systems with incompatible data accesses. System piping basically works by "gluing" the systems together, creating a single atomic chunk for the purposes of the scheduler. There's probably a good argument to make this "atomic" behavior independent of the system piping mechanism (see bevyengine/rfcs#46 for more context). Definitely far outside the scope of an initial PR here. |
That would be ideal. If the goal is to pipe multiple systems into one, this is the most direct way to express it, rather than expressing that you want to build the product of the outputs one after the other and feed that into another system.
I'm taking a look at it and the macro part is relatively easy, but I'm stuck at figuring out the types. If you can manually write the 2-tuple and 3-tuple version, I'll gladly turn it into a macro invocation for you.
Whether |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks correct to me.
My one concern is that I'm not sure how useful this would be in general. And given how easy it is to write custom system combinators, this doesn't strictly need to be built in to bevy.
I'm not opposed to adding this, though.
|
||
impl<In, A, B> Combine<A, B> for CartesianProduct | ||
where | ||
In: Copy, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value gets passed to the first system but not returned, so in order to pass it to the second system we have to be able to copy or clone it.
Okay, I will likely revert this commit, but I think it's an interesting idea. In order to try and get the This works(ish), but has several problems.
My conclusion is that a better notation would probably be along the lines of join(SystemA, SystemB, SystemC, SystemD).pipe(handler_system) |
If you end up going the tuple route, it'd be better to implement |
I ran into wanting this today for easily composable one-shot systems. |
I had another thought. Will the If I call This PR is starting to grow on me, but I'd like to make sure we don't ship something that horribly breaks scheduling. |
Digging a little deeper, seems like it's because This is incredibly subtle, I'd like to make this more explicit somehow. |
Here's another solution for join. pub fn join<A, B, AMarker, BMarker>(
mut system_a: A,
mut system_b: B,
) -> impl FnMut(In<A::In>, ParamSet<(A::Param, B::Param)>) -> (A::Out, B::Out)
where
A: SystemParamFunction<AMarker>,
B: SystemParamFunction<BMarker, In = A::In>,
A::In: Copy,
{
move |In(input), mut params| {
let out_a = system_a.run(input, params.p0());
let out_b = system_b.run(input, params.p1());
(out_a, out_b)
}
} Instead of |
This works wonders thank you!
fails. Is there a workaround for this? My logic looks something like this:
|
In order to pass a system into this style of combinator, the system needs to use this style of combinator all the way down, which pub fn pipe<A, B, AMarker, BMarker>(
mut system_a: A,
mut system_b: B,
) -> impl FnMut(In<A::In>, ParamSet<(A::Param, B::Param)>) -> B::Out
where
A: SystemParamFunction<AMarker>,
B: SystemParamFunction<BMarker, In = A::Out>,
{
move |In(input), mut params| {
let value = system_a.run(input, params.p0());
system_b.run(value, params.p1())
}
} Which you could then use like .add_systems(
Update,
join(
pipe(gather_player_movement, vectorise_input_flags),
pipe(get_max_power_vectors, get_relative_strengths),
)
.pipe(apply_thrust_stages),
) |
Again thank you very much this worked like a charm!
This takes a long time to compile though, even on latest nightly, currently rendering |
Another option is to turn each of those systems into a regular function which accepts a custom |
Right now manually threading systems is not to bad, with only around 8 different system params threaded through. But my current system has a few systems whose computations are used multiple times in different contexts, so by piping them using use bevy::prelude::*;
// example types
#[derive(Resource, Clone)] // Clone for simplicity, there may be a way to relax this requirement
pub struct ExampleCollectReturn;
/// do some computation, maybe read from a [Query]
pub fn collect() -> ExampleCollectReturn {
ExampleCollectReturn
}
/// time for demonstration purposes, you can put any system params you like
pub fn consumer_1(In(_collect_return): In<ExampleCollectReturn>, time: Res<Time>) {
// do something with the result
}
/// query for demonstration purposes, you can put any system params you like
pub fn consumer_2(In(_collect_return): In<ExampleCollectReturn>, query: Query<Entity>) {
// do something with the result
}
// optimization specific types
#[derive(Resource)]
pub struct _PipingResource {
collect_return: ExampleCollectReturn,
}
#[derive(Hash, PartialEq, Eq, Debug, Clone, SystemSet)]
pub struct _PipingSet;
fn _store_collect_return(In(collect_return): In<ExampleCollectReturn>, mut commands: Commands) {
commands.insert_resource(_PipingResource { collect_return });
}
fn _get_collect_return(_data: Res<_PipingResource>) -> ExampleCollectReturn {
// There is probably way to avoid cloning and use references / Box, but its fine for my use cases
_data.collect_return.clone()
}
// example usage (plugin)
pub struct ExamplePlugin;
impl Plugin for ExamplePlugin {
fn build(&self, app: &mut App) {
// not optimized way
app.add_systems(Update, (collect.pipe(consumer_1), collect.pipe(consumer_2)));
// optimized way
app.add_systems(
Update,
(
collect.pipe(_store_collect_return).in_set(_PipingSet),
_get_collect_return.pipe(consumer_1).after(_PipingSet),
_get_collect_return.pipe(consumer_2).after(_PipingSet),
),
);
}
} If this pattern is desirable, it is possible to design a macro-free API for this using a new derive macro, assuming you can break the |
Objective
pipe
multiple systems into a handler system.Solution
(systemA(input), systemB(input)
. This makes the following construction possible:Changelog
Added
Some notes
The main drawback of this approach is the excessive number of parems that turn up in the handler system (I do like a good Lisp, but I digress). Maybe the optimal API would look like this,
which would be cleaner, but also requires some macro work I'm not sure I can do. There's also a small question of running all the I/O systems in parallel or sequentially. My first guess would be to use abilities Bevy already has with its scheduling, but since I don't know how that works, I can't speak on it.