Skip to content
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 system.map(...) for transforming the output of a system #8526

Merged
merged 32 commits into from
Aug 28, 2023

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented May 1, 2023

Objective

Any time we wish to transform the output of a system, we currently use system piping to do so:

my_system.pipe(|In(x)| do_something(x))

Unfortunately, system piping is not a zero cost abstraction. Each call to .pipe requires allocating two extra access sets: one for the second system and one for the combined accesses of both systems. This also adds extra work to each call to update_archetype_component_access, which stacks as one adds multiple layers of system piping.

Solution

Add the AdapterSystem abstraction: similar to CombinatorSystem, this allows you to implement a trait to generically control how a system is run and how its inputs and outputs are processed. Unlike CombinatorSystem, this does not have any overhead when computing world accesses which makes it ideal for simple operations such as inverting or ignoring the output of a system.

Add the extension method .map(...): this is similar to .pipe(...), only it accepts a closure as an argument instead of an In<T> system.

my_system.map(do_something)

This has the added benefit of making system names less messy: a system that ignores its output will just be called my_system, instead of Pipe(my_system, ignore)


Changelog

TODO

Migration Guide

The system_adapter functions have been deprecated: use .map instead, which is a lightweight alternative to .pipe.

// Before:
my_system.pipe(system_adapter::ignore)
my_system.pipe(system_adapter::unwrap)
my_system.pipe(system_adapter::new(T::from))

// After:
my_system.map(std::mem::drop)
my_system.map(Result::unwrap)
my_system.map(T::from)

// Before:
my_system.pipe(system_adapter::info)
my_system.pipe(system_adapter::dbg)
my_system.pipe(system_adapter::warn)
my_system.pipe(system_adapter::error)

// After:
my_system.map(bevy_utils::info)
my_system.map(bevy_utils::dbg)
my_system.map(bevy_utils::warn)
my_system.map(bevy_utils::error)

@james7132 james7132 self-requested a review May 6, 2023 22:33
@james7132 james7132 added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events labels May 19, 2023
@@ -286,6 +311,7 @@ pub mod adapter {
/// println!("{x:?}");
/// }
/// ```
#[deprecated = "use `.map(...)` instead"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clever: I like being able to deprecate these.

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good complement to pipe. Simpler interface (avoids In<T>). .map is a common rust convention. Saves on some expenses.

I think in a world where we find a way to remove In<T> (in favor of just T) I might advocate for removing map, as I'm not sure avoiding the startup allocations for name() is worth "duplicate" abstractions. We could probably make name lazily computed if that is a concern as most games won't even need system names at runtime.

crates/bevy_ecs/src/system/adapter_system.rs Show resolved Hide resolved
@JoJoJet
Copy link
Member Author

JoJoJet commented Jun 13, 2023

I've fixed the type_id method, and I added an API that lets system adapters change the system name. Now, calling not(resource_exists<T>) will correctly result in a system called !resource_exists<T>.
Using .map() will not change the name of the inner system -- IMO this is cleaner, but I'm willing to change it if you guys think it's important for the system name to include the mapping function.

Also, I'm going to temporarily mark this as a draft. I want to see if I can make this abstraction usable for #8705 -- this will require giving the system adapter access to the system's name, but I don't have time to experiment with that at the moment.

@JoJoJet JoJoJet marked this pull request as draft June 13, 2023 17:48
@JoJoJet JoJoJet changed the title Add a zero-cost abstraction for transforming the output of a system Add system.map(...) for transforming the output of a system Jun 14, 2023
@JoJoJet JoJoJet marked this pull request as ready for review June 14, 2023 01:59
@JoJoJet
Copy link
Member Author

JoJoJet commented Jun 14, 2023

Alright, I've simplified the API for system names which will make it easier to use this in the aforementioned PR (and makes it consistent with CombinatorSystem.

I believe I have addressed all of the concerns raised :).

@ItsDoot
Copy link
Contributor

ItsDoot commented Aug 28, 2023

What's the status on this PR? Interesting in using .map.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Aug 28, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 28, 2023
Merged via the queue into bevyengine:main with commit 474b55a Aug 28, 2023
@JoJoJet JoJoJet deleted the system-mapping branch August 28, 2023 20:52
@TimJentzsch
Copy link
Contributor

Is it intentional that the changelog is still "TODO"?

@JoJoJet
Copy link
Member Author

JoJoJet commented Aug 28, 2023

I don't think we still do changelogs. Not sure why it's still in the PR template.

@cart
Copy link
Member

cart commented Aug 29, 2023

We do still do changelogs, we just lean on auto-generation more. Including changelogs in PRs is still welcome and encouraged as it will make the auto-generated changelog better.

Shatur pushed a commit to projectharmonia/bevy that referenced this pull request Aug 30, 2023
…gine#8526)

# Objective

Any time we wish to transform the output of a system, we currently use
system piping to do so:

```rust
my_system.pipe(|In(x)| do_something(x))
```

Unfortunately, system piping is not a zero cost abstraction. Each call
to `.pipe` requires allocating two extra access sets: one for the second
system and one for the combined accesses of both systems. This also adds
extra work to each call to `update_archetype_component_access`, which
stacks as one adds multiple layers of system piping.

## Solution

Add the `AdapterSystem` abstraction: similar to `CombinatorSystem`, this
allows you to implement a trait to generically control how a system is
run and how its inputs and outputs are processed. Unlike
`CombinatorSystem`, this does not have any overhead when computing world
accesses which makes it ideal for simple operations such as inverting or
ignoring the output of a system.

Add the extension method `.map(...)`: this is similar to `.pipe(...)`,
only it accepts a closure as an argument instead of an `In<T>` system.

```rust
my_system.map(do_something)
```

This has the added benefit of making system names less messy: a system
that ignores its output will just be called `my_system`, instead of
`Pipe(my_system, ignore)`

---

## Changelog

TODO

## Migration Guide

The `system_adapter` functions have been deprecated: use `.map` instead,
which is a lightweight alternative to `.pipe`.

```rust
// Before:
my_system.pipe(system_adapter::ignore)
my_system.pipe(system_adapter::unwrap)
my_system.pipe(system_adapter::new(T::from))

// After:
my_system.map(std::mem::drop)
my_system.map(Result::unwrap)
my_system.map(T::from)

// Before:
my_system.pipe(system_adapter::info)
my_system.pipe(system_adapter::dbg)
my_system.pipe(system_adapter::warn)
my_system.pipe(system_adapter::error)

// After:
my_system.map(bevy_utils::info)
my_system.map(bevy_utils::dbg)
my_system.map(bevy_utils::warn)
my_system.map(bevy_utils::error)
```

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@cart cart mentioned this pull request Oct 13, 2023
43 tasks
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
…gine#8526)

# Objective

Any time we wish to transform the output of a system, we currently use
system piping to do so:

```rust
my_system.pipe(|In(x)| do_something(x))
```

Unfortunately, system piping is not a zero cost abstraction. Each call
to `.pipe` requires allocating two extra access sets: one for the second
system and one for the combined accesses of both systems. This also adds
extra work to each call to `update_archetype_component_access`, which
stacks as one adds multiple layers of system piping.

## Solution

Add the `AdapterSystem` abstraction: similar to `CombinatorSystem`, this
allows you to implement a trait to generically control how a system is
run and how its inputs and outputs are processed. Unlike
`CombinatorSystem`, this does not have any overhead when computing world
accesses which makes it ideal for simple operations such as inverting or
ignoring the output of a system.

Add the extension method `.map(...)`: this is similar to `.pipe(...)`,
only it accepts a closure as an argument instead of an `In<T>` system.

```rust
my_system.map(do_something)
```

This has the added benefit of making system names less messy: a system
that ignores its output will just be called `my_system`, instead of
`Pipe(my_system, ignore)`

---

## Changelog

TODO

## Migration Guide

The `system_adapter` functions have been deprecated: use `.map` instead,
which is a lightweight alternative to `.pipe`.

```rust
// Before:
my_system.pipe(system_adapter::ignore)
my_system.pipe(system_adapter::unwrap)
my_system.pipe(system_adapter::new(T::from))

// After:
my_system.map(std::mem::drop)
my_system.map(Result::unwrap)
my_system.map(T::from)

// Before:
my_system.pipe(system_adapter::info)
my_system.pipe(system_adapter::dbg)
my_system.pipe(system_adapter::warn)
my_system.pipe(system_adapter::error)

// After:
my_system.map(bevy_utils::info)
my_system.map(bevy_utils::dbg)
my_system.map(bevy_utils::warn)
my_system.map(bevy_utils::error)
```

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants