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

[Merged by Bors] - Auto-label function systems with SystemTypeIdLabel #4224

Closed
wants to merge 6 commits into from

Conversation

cart
Copy link
Member

@cart cart commented Mar 16, 2022

This adds the concept of "default labels" for systems (currently scoped to "parallel systems", but this could just as easily be implemented for "exclusive systems"). Function systems now include their function's SystemTypeIdLabel by default.

This enables the following patterns:

// ordering two systems without manually defining labels
app
  .add_system(update_velocity)
  .add_system(movement.after(update_velocity))

// ordering sets of systems without manually defining labels
app
  .add_system(foo)
  .add_system_set(
    SystemSet::new()
      .after(foo)
      .with_system(bar)
      .with_system(baz)
  )

Fixes: #4219
Related to: #4220

Credit to @aevyrie @alice-i-cecile @DJMcNab (and probably others) for proposing (and supporting) this idea about a year ago. I was a big dummy that both shut down this (very good) idea and then forgot I did that. Sorry. You all were right!

@cart cart added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Mar 16, 2022
@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 16, 2022
@cart
Copy link
Member Author

cart commented Mar 16, 2022

Note that I implemented this because it was "very easy", not necessarily because I'm voting for it as the final solution.

@alice-i-cecile alice-i-cecile removed the S-Needs-Triage This issue needs to be labelled label Mar 16, 2022
@alice-i-cecile
Copy link
Member

(currently scoped to "parallel systems", but this could just as easily be implemented for "exclusive systems").

IMO this isn't worth our time; we should pursue #4166 instead.

But that would require adapting before() and after() to accept that trait (and implementing the trait for all SystemLabels).
Definitely worth considering. But its a lot of extra complexity.

IMO this is worth doing, but maybe not right now. The ergonomics win is quite significant here.

.label() would conflict with system_b.label(MyCustomLabel). We can (and should) discuss renaming this to system_b.with_label(MyCustomLabel) to free up the system_a.after(system_b.label(()).

I don't think either usage is clearer, and I'd prefer to a) avoid breaking user code and b) pursue the .system_label-yeet solution above.

@cart
Copy link
Member Author

cart commented Mar 16, 2022

IMO this is worth doing, but maybe not right now. The ergonomics win is quite significant here.

I'll give it a try

@cart
Copy link
Member Author

cart commented Mar 16, 2022

That works! Only outstanding issue is one of our tests failing because the test checks the "first labels" of ambiguous systems (which are now the default labels) instead of checking the manual "string labels" that were assigned.

@cart
Copy link
Member Author

cart commented Mar 16, 2022

Updated the description to account for the better ergonomics.

@cart
Copy link
Member Author

cart commented Mar 16, 2022

Biggest downside is that before(system_name) will monomorphize its impls for every unique system function signature passed in. But its a relatively small function and you only pay for what you actually use. And its a "lateral move" for custom labels, which already behaved that way. And it should be an identical compile time cost when compared to per-system-custom-labels.

@cart
Copy link
Member Author

cart commented Mar 16, 2022

Alrighty I fixed the ambiguity test. Now that the UX downsides have been eliminated, I think I'm ready to vote to merge this. Its a big (non-breaking) ergo win for almost zero effort. Future "owned ordering" SystemGraph-like solutions are still on the table, even if we merge this.

@alice-i-cecile
Copy link
Member

We should update the SystemLabel docs to reflect this change; I think that's the best location for it.

@cart
Copy link
Member Author

cart commented Mar 16, 2022

Makes sense. Also forgot to call out a corner case of this impl: closure systems. They will be labeled with an anonymous TypeId that can only be retrieved if you have an instance of the closure. This feels very reasonable to me. You can either retrieve the SystemTypeIdLabel from the instance and pass it around, or you can create a new custom label and manually apply it.

@@ -489,3 +506,21 @@ macro_rules! impl_system_function {
}

all_tuples!(impl_system_function, 0, 16, F);

pub trait AsSystemLabel<Marker> {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a short doc comment stating this is used for system -> SystemLabel coercion plus a pointer to SystemLabel which illustrates the ergonomics.

It'd also stave off the perpetually missing #[forbid(missing_docs)] for the crate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call!

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

Broadly in favour. I'd rather AsSystemLabel consumed and wasn't a method, but don't block on that.

crates/bevy_ecs/src/schedule/stage.rs Show resolved Hide resolved
@@ -489,3 +507,21 @@ macro_rules! impl_system_function {
}

all_tuples!(impl_system_function, 0, 16, F);

pub trait AsSystemLabel<Marker> {
fn as_system_label(&self) -> Box<dyn SystemLabel>;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fn as_system_label(&self) -> Box<dyn SystemLabel>;
fn as_system_label(this: &Self) -> Box<dyn SystemLabel>;

I don't think we need this to be a method, for similar reasons we don't need the analagous IntoSystem::into_system as a method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Idk if "non method Self functions" is a pattern I want to encourage generally. We had a good reason to do that for IntoSystem: deprecating .system() let us "encourage" users to use the more ergonomic pattern without outright breaking their code. We don't have that context here. Given that methods are the more common pattern, I think the more relevant question is: why not make this a method?

Copy link
Member

Choose a reason for hiding this comment

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

Mostly it's never meant to be used by end users, but will clutter up the flyimport suggests when users are attempting to find .before or .after, especially as alphabetically it is in between those two options.

Copy link
Member

Choose a reason for hiding this comment

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

Cluttering auto-complete imports is actually a compelling argument here :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Mostly it's never meant to be used by end users,

I'm not sure I agree, given that getting the "closure" label is something you'd want to do with an as_system_label call (which would benefit from method-style syntax). Other use cases may emerge in the future too (ex: if something needs to accept a list of labels, we can't pass a bunch of different AsSystemLabel impls ... we need to convert to boxed labels before the call).

I personally don't think autocomplete is much of an issue here, as I think most people type at least the first letter of what they're looking for in autocomplete. .b for .before, .a for .after ... .after comes before .as in an alphabetically sorted list.

Copy link
Member

Choose a reason for hiding this comment

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

I personally don't think autocomplete is much of an issue here, as I think most people type at least the first letter of what they're looking for in autocomplete. .b for .before, .a for .after ... .after comes before .as in an alphabetically sorted list.

Fair enough.


impl<T: SystemLabel> AsSystemLabel<()> for T {
fn as_system_label(&self) -> Box<dyn SystemLabel> {
self.dyn_clone()
Copy link
Member

Choose a reason for hiding this comment

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

The need to clone here is an unfortunate 'regression'

Is there any downside to just making AsSystemLabel consume self?

Copy link
Member Author

Choose a reason for hiding this comment

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

Consuming self would make this unusable for closure systems, as retrieving the label would consume the IntoSystem impl (aka the closure).

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this doesn't add extra boxing (this replaces the manual boxing that we did in the past). Given that basically all system labels are enums, typeids, or ZSTs, the extra clone probably wouldn't even be measurable, even for many thousands of systems.

Copy link
Member

Choose a reason for hiding this comment

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

It's already hugely unergonomic for closure systems though. Either you create the 'same' closure twice:

fn my_closure_system() -> ... {||{}}
app.with_system(my_closure_system()).with_system(other.after(my_closure_system()))

Or you store it in a local variable:

let closure = || {};
with_system(other.after(&closure)).with_system(closure) // has to be this order.

But yeah, perhaps being able to use the second pattern is worthwhile.

Copy link
Member

Choose a reason for hiding this comment

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

IMO this is so challenging (and niche) for closure systems that we shouldn't go out of our way to support it. Providing an explicit label is a nice clear workaround.

Copy link
Member Author

Choose a reason for hiding this comment

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

This seems like a really reasonable use case / pattern for closure systems. Given that closure systems are now "the" way to pass in-scope configuration into a system, and closures will also exist in that scope, I think its reasonable to support ordering without needing to define custom labels. At the very least, I think this should be possible (and it currently is):

let closure = || {};
app
  .add_system(foo.after(closure.as_system_label())))
  .add_system(closure)

But I'm also open to making @DJMcNab's second suggestion work.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just made AsSystemLabel return the actual label type instead of a box. This doesn't "resolve" the extra clone, but it does feel like a cleaner design to me. I really don't think this should consume self, as I don't think we should commit to all system instances being "disposable". "closure systems" already aren't disposable, and things like the current manual FixedTimestep system implementation also "hold state" and shouldn't be disposed of here (I know that the FixedTimestep impl is controversial, but its illustrative of the "custom system" pattern). I stand by the thought that an extra clone here won't hurt anyone, especially when labels are basically exclusively ZSTs and enums.

@superdump
Copy link
Contributor

What do the resulting system labels look like? Do they have a string form? How do they look in tracing spans? And do these automatic labels work well for things like ExtractComponentPlugin’s extraction system?

@cart
Copy link
Member Author

cart commented Mar 19, 2022

What do the resulting system labels look like?

SystemTypeIdLabel(TypeId), where TypeId is a u64 hash.

Do they have a string form?

They could, if we're willing to also store the type_name in the SystemTypeIdLabel (or if the "hash" is acceptable).

How do they look in tracing spans?

We don't put system labels in tracing spans atm.

And do these automatic labels work well for things like ExtractComponentPlugin’s extraction system?

Now that #3817 is merged, that extract system is a "function system" so in theory it works. However currently that is a private system. If we want to treat that as part of our public api / let people make their systems depend on it, we can make the system pub.

@james7132
Copy link
Member

james7132 commented Mar 19, 2022

Do they have a string form?

They could, if we're willing to also store the type_name in the SystemTypeIdLabel (or if the "hash" is acceptable).

Could we not do the following?

#[derive(SystemLabel, ...)]
pub struct SystemLabelId<T: 'static> {
  marker_: PhantomData<T>,
}

Then when implementing Hash, Debug, etc. we use TypeId::of<T> and TypeId::type_name<T>? This would make the type a ZST and still provide type data for debugging. Downside is the additional compile time needed for each system, though if it transparently uses the Debug and Hash implementations of TypeId, a bunch of those impls would be flattened out or combined during compilation.


#[derive(Debug, Hash, PartialEq, Eq, Copy, Clone)]
/// A [`SystemLabel`] that was automatically generated for a system on the basis of its `TypeId`.
pub struct SystemTypeIdLabel(pub(crate) TypeId);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub struct SystemTypeIdLabel(pub(crate) TypeId);
pub(crate) struct SystemTypeIdLabel(pub(crate) TypeId);

Or if we can afford it, remove the pub entirely.

Like #4250. This is more of an implementation detail than anything else, probably shouldn't made public.

Copy link
Member Author

Choose a reason for hiding this comment

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

Imo this label is a part of our public api (especially with my recent changes, which remove the box from AsSystemLabel). I think there might be cases where this should be accessible to users.

@alice-i-cecile alice-i-cecile added this to the Bevy 0.7 milestone Mar 20, 2022
@cart
Copy link
Member Author

cart commented Mar 23, 2022

Just pushed changes that remove the boxing from AsSystemLabel, which feels less "hackey" to me. Also made SystemTypeIdLabel a ZST, per @james7132's suggestion.

Barring further feedback, I think I'll merge this.

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

Nothing blocking.

crates/bevy_ecs/src/schedule/stage.rs Show resolved Hide resolved
/// "system functions" to their [`SystemTypeIdLabel`].
pub trait AsSystemLabel<Marker> {
type SystemLabel: SystemLabel;
fn as_system_label(&self) -> Self::SystemLabel;
Copy link
Member

Choose a reason for hiding this comment

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

I'll record that I'm still not super happy with this being a method, but I don't think that's changing.

Copy link
Member Author

@cart cart Mar 23, 2022

Choose a reason for hiding this comment

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

Yeah I'm not (yet) convinced. I think there are good reasons to make "manually retrieving the label from a system instance" a part of our api. We don't abstract out "other" labels and we let those instances float around. Why abstract out these?
I'm down to continue discussing, but I dont want to block on this.

}

/// A [`SystemLabel`] that was automatically generated for a system on the basis of its `TypeId`.
pub struct SystemTypeIdLabel<T: 'static>(PhantomData<fn() -> T>);
Copy link
Member

Choose a reason for hiding this comment

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

This couldn't be derive(Default) because of the type parameter bounds, but should it still be Default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting. This would enable people to construct these labels without an instance to a system type. Maybe thats good. Is there a use case?

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. This would enable people to construct these labels without an instance to a system type. Maybe thats good. Is there a use case?

This could be useful if you wanted to initialize an array of these things, especially if we also impl'd Copy. I'm not immediately sure why you'd want to do that though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha I'm impatient, so I'm just going to merge this now. I'm happy to add this if we decide its needed.

crates/bevy_ecs/src/system/system.rs Show resolved Hide resolved
examples/ecs/system_sets.rs Show resolved Hide resolved
@cart
Copy link
Member Author

cart commented Mar 23, 2022

bors r+

bors bot pushed a commit that referenced this pull request Mar 23, 2022
This adds the concept of "default labels" for systems (currently scoped to "parallel systems", but this could just as easily be implemented for "exclusive systems"). Function systems now include their function's `SystemTypeIdLabel` by default.

This enables the following patterns:

```rust
// ordering two systems without manually defining labels
app
  .add_system(update_velocity)
  .add_system(movement.after(update_velocity))

// ordering sets of systems without manually defining labels
app
  .add_system(foo)
  .add_system_set(
    SystemSet::new()
      .after(foo)
      .with_system(bar)
      .with_system(baz)
  )
```

Fixes: #4219
Related to: #4220 

Credit to @aevyrie @alice-i-cecile @DJMcNab (and probably others) for proposing (and supporting) this idea about a year ago. I was a big dummy that both shut down this (very good) idea and then forgot I did that. Sorry. You all were right!
@bors bors bot changed the title Auto-label function systems with SystemTypeIdLabel [Merged by Bors] - Auto-label function systems with SystemTypeIdLabel Mar 23, 2022
@bors bors bot closed this Mar 23, 2022
bors bot pushed a commit that referenced this pull request Apr 1, 2022
# Objective

- Since #4224, using labels which only refer to one system doesn't make sense.

## Solution

- Remove some of those.

## Future work

- We should remove the ability to use strings as system labels entirely. I haven't in this PR because there are tests which use this, and that's a lot of code to change.
   - The only use cases for labels are either intra-crate, which use #4224, or inter-crate, which should either use #4224 or explicit types. Neither of those should use strings.
aevyrie pushed a commit to aevyrie/bevy that referenced this pull request Jun 7, 2022
This adds the concept of "default labels" for systems (currently scoped to "parallel systems", but this could just as easily be implemented for "exclusive systems"). Function systems now include their function's `SystemTypeIdLabel` by default.

This enables the following patterns:

```rust
// ordering two systems without manually defining labels
app
  .add_system(update_velocity)
  .add_system(movement.after(update_velocity))

// ordering sets of systems without manually defining labels
app
  .add_system(foo)
  .add_system_set(
    SystemSet::new()
      .after(foo)
      .with_system(bar)
      .with_system(baz)
  )
```

Fixes: bevyengine#4219
Related to: bevyengine#4220 

Credit to @aevyrie @alice-i-cecile @DJMcNab (and probably others) for proposing (and supporting) this idea about a year ago. I was a big dummy that both shut down this (very good) idea and then forgot I did that. Sorry. You all were right!
aevyrie pushed a commit to aevyrie/bevy that referenced this pull request Jun 7, 2022
# Objective

- Since bevyengine#4224, using labels which only refer to one system doesn't make sense.

## Solution

- Remove some of those.

## Future work

- We should remove the ability to use strings as system labels entirely. I haven't in this PR because there are tests which use this, and that's a lot of code to change.
   - The only use cases for labels are either intra-crate, which use bevyengine#4224, or inter-crate, which should either use bevyengine#4224 or explicit types. Neither of those should use strings.
bors bot pushed a commit that referenced this pull request Dec 25, 2022
# Objective

Fix #5653.

## Solution

- Add an example of how systems can be ordered from within a stage.
- Update some docs from before #4224
bors bot pushed a commit that referenced this pull request Dec 25, 2022
# Objective

Fix #5653.

## Solution

- Add an example of how systems can be ordered from within a stage.
- Update some docs from before #4224
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
# Objective

Fix bevyengine#5653.

## Solution

- Add an example of how systems can be ordered from within a stage.
- Update some docs from before bevyengine#4224
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

Fix bevyengine#5653.

## Solution

- Add an example of how systems can be ordered from within a stage.
- Update some docs from before bevyengine#4224
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
This adds the concept of "default labels" for systems (currently scoped to "parallel systems", but this could just as easily be implemented for "exclusive systems"). Function systems now include their function's `SystemTypeIdLabel` by default.

This enables the following patterns:

```rust
// ordering two systems without manually defining labels
app
  .add_system(update_velocity)
  .add_system(movement.after(update_velocity))

// ordering sets of systems without manually defining labels
app
  .add_system(foo)
  .add_system_set(
    SystemSet::new()
      .after(foo)
      .with_system(bar)
      .with_system(baz)
  )
```

Fixes: bevyengine#4219
Related to: bevyengine#4220 

Credit to @aevyrie @alice-i-cecile @DJMcNab (and probably others) for proposing (and supporting) this idea about a year ago. I was a big dummy that both shut down this (very good) idea and then forgot I did that. Sorry. You all were right!
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Since bevyengine#4224, using labels which only refer to one system doesn't make sense.

## Solution

- Remove some of those.

## Future work

- We should remove the ability to use strings as system labels entirely. I haven't in this PR because there are tests which use this, and that's a lot of code to change.
   - The only use cases for labels are either intra-crate, which use bevyengine#4224, or inter-crate, which should either use bevyengine#4224 or explicit types. Neither of those should use strings.
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-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically label systems with their TypeId
5 participants