Implement a SystemBuilder for building SystemParams

[james-j-obrien]

Objective

• Implement a general purpose mechanism for building SystemParam.

Solution

• Implement a SystemBuilder type.

Examples

Here are some simple test cases for the builder:

fn local_system(local: Local<u64>) -> u64 {
    *local
}

fn query_system(query: Query<()>) -> usize {
    query.iter().count()
}

fn multi_param_system(a: Local<u64>, b: Local<u64>) -> u64 {
    *a + *b + 1
}

#[test]
fn local_builder() {
    let mut world = World::new();

    let system = SystemBuilder::<()>::new(&mut world)
        .builder::<Local<u64>>(|x| *x = 10)
        .build(local_system);

    let result = world.run_system_once(system);
    assert_eq!(result, 10);
}

#[test]
fn query_builder() {
    let mut world = World::new();

    world.spawn(A);
    world.spawn_empty();

    let system = SystemBuilder::<()>::new(&mut world)
        .builder::<Query<()>>(|query| {
            query.with::<A>();
        })
        .build(query_system);

    let result = world.run_system_once(system);
    assert_eq!(result, 1);
}

#[test]
fn multi_param_builder() {
    let mut world = World::new();

    world.spawn(A);
    world.spawn_empty();

    let system = SystemBuilder::<()>::new(&mut world)
        .param::<Local<u64>>()
        .param::<Local<u64>>()
        .build(multi_param_system);

    let result = world.run_system_once(system);
    assert_eq!(result, 1);
}

This will be expanded as this PR is iterated.

[iiYese]

I'll give a more detailed review later but just from the examples I think providing the system before the system params the wrong way to do this. The system should be provided as a param to the builder at the end after all the parameter builders are defined.

[james-j-obrien]

I'll give a more detailed review later but just from the examples I think providing the system before the system params the wrong way to do this. The system should be provided as a param to the builder at the end after all the parameter builders are defined.

I initially implemented it that way but then you don't get the type deduction for the builder closures until you add the system at the end. The alternative would be to force you to write out the arguments as a tuple during creation of the builder which comes across as redundant when using the builder on a static system.

If we expect this to be primarily used with closures as the system then that would probably be fine as hopefully you would get the type deduction for the closure.

[iiYese]

I initially implemented it that way but then you don't get the type deduction for the builder closures until you add the system at the end. The alternative would be to force you to write out the arguments as a tuple during creation of the builder which comes across as redundant when using the builder on a static system.

I wouldn't mind the repetition. Alternatively what I think would be better is removing the type deduction, making it more dynamic & having the builder fill in the missing params from the provided system with a way to opt out of this to make it more strict.

Something like this:

fn sys(a: Query<&A>, bc: Query<(&B, &C)>) {
    // ..
}

let system = world
    .system_builder()
    // alternatively dynamic version: .param(0, |query: Query<()>| {})
    // - Adds param0 with ((), With<X>)
    // - For query params in the typed version of the API the terms provided to the query builder must be a 
    // subset of the terms present in the type signature of the corresponding param of the system.
    // Ambiguous terms like `(&A, &A)` when we have relations must be resolved within the builder.
    .param::<0>(|query: Query<()>| {
        query.with::<X>();
    })
    // Fills missing terms:
    // - Adds &A to param0
    // - Adds param1 with (&B, &C)
    .system(sys) // could also be inline closure
    .build();
    
let strict_system = world
    .system_builder()
    .param::<0>(|query: Query<()>| {
        query.with::<X>();
    })
    .strict()
    // will fail because system types do not match the types constructed by the builder
    .system(sys)
    .build();

[james-j-obrien]

I wish we could easily implement such a dynamic API but that would require far more extensive changes then I've made here. You could dynamically fill the tuple with a macro, but being able to deduce that the more minimal query is allowed/supposed to be transmuted into the more specific query you would either need some new trait that encoded which SystemParam can be transmuted into which other SystemParam or just allow any params in any callback and have it all checked and panic at runtime. Even then the implementation to have it choose the correct transmute function seems non-trivial. If you give it a go and have success I'm happy to merge it.

At this point I would much rather wait to refactor the implementation of system state to be more dynamic (not that I have an immediate plan of how this should be done) and make do with this for now then push the current abuse of tuples any further.

I don't disagree that this API isn't perfect but it unblocks a desirable use case and I think is the most manageable way to implement it. It's also trivially extensible for user defined params.

[iiYese]

but being able to deduce that the more minimal query ..

You don't deduce the more minimal query you always prefer the query in the system signature & it's not from type information it's from runtime metadata either provided by the dynamic API or generated by the types (shouldn't need more than we already have). So:

// This:
.param::<0>(|query: Query<&A>| {
    query.with::<X>();
})
.system(|query: Query<(&A, &B)>| {
    // ..
});

// would be equivalent to this:
.param::<0>(|query: Query<()>| {
    query.term::<&A>()
         .with::<X>()
         // not actually part of the API just for illustration
         .term_if_absent::<&A>()
         .term_if_absent::<&B>()
         .add_bound::<&A>()
         .add_bound::<&B>()
         .all_bound()?;
})

// These will work
.param::<0>(|query: Query<&A>| {
    // ..
})
.system(|query: Query<(&A, &B)>| {
    // ..
});

.param::<0>(|query: Query<()>| {
    query.term::<&A>();
})
.system(|query: Query<(&A, &B)>| {
    // ..
});

// These will not work
.param::<0>(|query: Query<(&A, &B)>| {
    // ..
})
.system(|query: Query<&A>| {
    // ..
});

.param::<0>(|query: Query<()>| {
    query.term::<&A>()
         .term::<&B>();
})
.system(|query: Query<&A>| {
    // ..
});

[james-j-obrien]

I understand the API you're proposing, but in order to turn one query state into another at some point you need to call the query transmute method to re-create the correct query state with the additional statically defined terms. Our query state is also stored in tuples so we don't have a way to dynamically "upcast" one query into another without type information. In order to set the initial partial query you need some kind of type erased storage for the system param or you need to be building a tuple as you go, then at some point you need to take the tuple/dynamic storage and match up the params so you can convert their states to the final states you desire.

It could be possible to implement but the complexity is much higher, currently the builders are simple callbacks stored in options in a tuple and the traits/macros to generate the current API are already obtuse. If you think there is a simple implementation that provides the API you want then feel free to give it a go, but I don't want to spend hours tinkering with a more complex API that could either blow up the size of the PR or not be implementable. I'd rather reserve improvements to follow-ups and unblock the feature with a simpler implementation now.

[james-j-obrien]

Did a refactor to base it on partial tuples bringing it more in line with the API both @iiYese and I would prefer.

The main rough edge here is that you need to explicitly lay out all of your params again even if you have them defined in a static function since implementing a way to "fill in" the rest of the param state would require nested calls to all_tuples and that would be a huge amount of code generation. In most cases though I think this API is fine.

[chescock]

Would it make sense to have a function that creates a SystemState<T> from a SystemBuilder<T>? I think you have all the necessary values available. That would make dynamic system params usable even outside function systems.

[chescock]

This has an unfortunate interaction with impl Clone for FunctionSystem, which will reset the param_state of a cloned system and lose the data passed in by the builder. (I don't think it's a blocking issue and I don't see an easy way to improve the situation, but it seemed worth noting.)

[james-j-obrien]

There is a param method that will add any SystemParam even if it's not "buildable", but I agree in the sense there could also be helper methods for these i.e. .resource::<T> or similar.

[alice-i-cecile]

Awesome, that's better than I expected. Let's do helper methods + docs then. If it happens to be ready for 0.14 I have no objections to merging it in this release.

[alice-i-cecile]

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to https://github.com/bevyengine/bevy-website/issues/1323 if you'd like to help out.