improve WithInputWrapper
- Dominant language
- Rust
- Stars
- 48.2k
- Forks
- 4.8k
- Avg merge
- 3d 16h
- Merged PRs (30d)
- 171
Description
## What problem does this solve or what need does it fill?
I have designed several unconventional systems, all of which share the common characteristic of accepting the same external input via SystemInput.
```
fn system(In(entity): In, param1: ..., param2: ...) { ... }
```
I could create a head closure that returns a fixed entity and pass it to the next system via pipe. However, if I want to pass it further to subsequent systems, the system would have to be changed to:
```
fn system(In(entity): In, param1: ..., param2: ...) -> Entity { entity }
```
This scenario is far from ideal, as it forces the developer to manually return the entity to the next system every time. Alternatively, I could use multiple head closures to pass the value individually to each subsequent system, but I need to combine these multiple systems into one, which would require relatively cumbersome initialization and piping of systems that return the empty tuple ().
In IntoSystem, with_input allows the developer to pass a value, but the value is only mutable, not owned:
```
fn with_input(self, value: T) -> WithInputWrapper
where
for<'i> In: SystemInput = &'i mut T>,
T: Send + Sync + 'static,
{
WithInputWrapper::new(self, value)
}
```
This eliminates the need for piping to the next system, but it passes a mutable value. Thus, the aforementioned issue remains: after passing the value to a system, that system can affect the value received by other systems.
## What solution would you like?
After reviewing the relevant code, I don't think we should directly add a with_cloned_input function. Instead, we should modify IntoSystem::with_input to support all SystemInput traits like In, InMut, InRef, just as FunctionSystem does. For In, which transfers ownership, developers should be responsible for calling Clone themselves, as shown below:
```
impl Plugin for CreaturePlugin {
fn build(app: &mut App) {
let value = ...;
app.add_systems(Startup, (
spawn_creature.with_input(In(value.clone())),
other_system.with_input(InMut(&mut value))
));
}
}
```
To achieve this kind of support, WithInputWrapper may need to be redesigned and restructured.
_Originally posted by @MushineLament in https://github.com/bevyengine/bevy/issues/22154#issuecomment-3941175004_
This would change the limitations of WithInputWrapper while also addressing the issue of unwanted modifications to the value passed through pipe. Additionally, it would enable potential optimizations for multiple systems accepting the same value and reduce the overhead that might come from passing values via pipe.
```
fn system(InRef(entity): InRef, param1: ..., param2: ...) { ... }
```
The systems with values would then look like this:
```
let value = ...;
let mut value2 = ...;
(
system_in.with_input(In(value.clone())),
system_ref.with_input(InRef(&value)),
system_mut.with_input(InMut(&mut value2)),
)
```
If multiple WithInputWrapper instances that accept the same value type could be combined into one, it would simplify the usage of such systems:
```
let value = ...;
(
system_in1,
system_in2,
).with_input(In(value.clone()))
(
system_ref1,
system_ref2,
).with_input(InRef(&value))
```
Of course, passing multiple mutable references simultaneously is still not allowed, and Rust's ownership rules must be respected:
```
(
system_ref1,
system_mut2,
).with_input(InMut(&mut value)) // ❌ panic!
```
## What alternative(s) have you considered?
After reviewing and attempting to modify `with_input` and `WithInputWrapper`, I have to say that the data structure of `WithInputWrapper` has relatively significant issues; it is too tightly coupled with `with_input`. My skills are not sufficient to achieve what was described above.
We already have the `SystemInput` trait, and I don't think we should create another trait specifically to make `WithInputWrapper`'s `System>>` support `SystemInput`.
## Additional context
If the above functionality is implemented, when multiple `WithInputWrapper` instances are merged into one, initialization could enable them to be parallelized, further improving performance.
Contributor guide
Research direction
Start by reading IntoSystem::with_input, WithInputWrapper, SystemInput, and FunctionSystem to understand their current contracts. Trace how In, InMut, and InRef are handled and identify the tests covering with_input. Done means WithInputWrapper supports the requested SystemInput forms while preserving Rust ownership and borrowing rules.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- game-dev
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100