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] - Make RunOnce a non-manual System impl #3922

Closed
wants to merge 4 commits into from

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Feb 12, 2022

Objective

  • RunOnce was a manual System implementation.
  • Adding run criteria to stages was yet to be systemyoten

Solution

  • Make it a normal function
  • yeet

Changelog

  • Replaced RunOnce with ShouldRun::once

Migration guide

The run criterion RunOnce, which would make the controlled systems run only once, has been replaced with a new run criterion function ShouldRun::once. Replace all instances of RunOnce with ShouldRun::once.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Feb 12, 2022
@DJMcNab DJMcNab added A-App Bevy apps and plugins A-ECS Entities, components, systems, and events M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide C-Code-Quality A section of code that is hard to understand or change and removed S-Needs-Triage This issue needs to be labelled labels Feb 12, 2022
@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 Feb 14, 2022
@DJMcNab DJMcNab added this to the Bevy 0.7 milestone Feb 24, 2022
@cart
Copy link
Member

cart commented Apr 8, 2022

I do really like simplifying this impl + the general progression we've had toward consolidating system logic under "system functions". But I'm also not particularly happy with "functions returning closure systems" as our public interface.

I don't think its possible with IntoSystem as it stands (because it requires a "nameable type" to assign the returned system type), but I rather like the idea of:

struct RunOnce;
// maybe IntoSystem could default to () for its generics to enable this
impl IntoSystem for RunOnce {
    // this should work if/when "impl trait in associated types" lands.
    type System = impl System<In = (), Out = RunCriteria>;

    fn into_system(self) -> Self::System {
        let mut ran = false;
        (move || {
            if ran {
                ShouldRun::No
            } else {
                ran = true;
                ShouldRun::Yes
            }
        }).system()
    }
} 

This would allow users to transform arbitrary structs (and use their fields) to construct closure function systems. Slightly more boilerplate, but it has the benefit of allowing stuff like:

app
  .add_system(foo.with_run_criteria(FixedTimestep::from_seconds(2.0)))
  .add_system(bar.with_run_criteria(FixedTimestep::from_duration(Duration::from_secs_f64(1.0))))

Compared to stuff like:

app
  .add_system(foo.with_run_criteria(fixed_timestep_from_seconds(2.0)))
  .add_system(bar.with_run_criteria(fixed_timestep_from_duration(Duration::from_secs_f64(1.0))))

The former is much more discoverable, documentable, and "rusty" in my personal opinion.

I'm down to discuss this, but if we agree thats what we should be aiming for ultimately, maybe we should adopt something like this in the interim?

impl RunOnce {
    pub fn new() -> impl FnMut() -> ShouldRun {
        let mut ran = false;
        move || {
            if ran {
                ShouldRun::No
            } else {
                ran = true;
                ShouldRun::Yes
            }
        }
    }
}

Heck, maybe thats better than an IntoSystem impl?

@DJMcNab DJMcNab removed 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 Apr 9, 2022
@@ -44,6 +41,17 @@ pub enum ShouldRun {
NoAndCheckAgain,
}

impl ShouldRun {
pub fn once(mut ran: Local<bool>) -> ShouldRun {
Copy link
Member

@cart cart Apr 10, 2022

Choose a reason for hiding this comment

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

Ooh this is interesting. One more small consistency thing to discuss api on:

This uses "normal system" add-using-function-name syntax add_system(foo.with_run_criteria(ShouldRun::once)), which has the benefit of being exactly what you would do for a "non-closure system".

However if were were to add a theoretical ShouldRun::n_times, it would need to return a closure to store the value of n.

So we would have the inconsistent styles:

app.
  .add_system(foo.with_run_criteria(ShouldRun::once))
  .add_system(foo.with_run_criteria(ShouldRun::n_times(42))

So the question is: is this ok? (this is not a loaded question ... i dont have strong opinions here). When using the "functions on structs" pattern for systems, what are our "rules"?
Should they be:

  1. When using struct-functions, always use functions returning impl Fn/FnMut for consistency
  2. When using struct-functions, prefer "normal system" style when possible and return impl Fn/FnMut only when required by the context.
  3. When using struct-functions, prefer "normal system" style when possible, but prefer consistency across functions on the struct if there is a mismatch.
  4. Use normal_system when possible. Only use struct-functions for closure-style systems.

(3) (4) (edit: accidentally referenced the wrong item here ... see my correction below) loses the organizational benefits of struct style (which would be very confusing), so thats the only option I'm actively against.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Use const generics. ShouldRun::n_times::<{42}>.

I'd generally lean towards option 2. I think we need to do the same migration for FixedTimestep, but I hadn't done it yet because of stageless.

Copy link
Member

Choose a reason for hiding this comment

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

My preference is 4: I think they're clearer in general (especially in the docs), and we should have strong reasons to use struct-functions.

3 is definitely bad.

Copy link
Member

Choose a reason for hiding this comment

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

Arg sorry I inserted an extra list item, invalidating my reference to (3). Sorry for the confusion (bad cart!). I meant to say:

(4) loses the organizational benefits of struct style (which would be very confusing), so thats the only option I'm actively against.

Copy link
Member

Choose a reason for hiding this comment

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

I think this would be actively bad UX:

app
  .add_system(foo.with_run_criteria(should_run_once)
  .add_system(foo.with_run_criteria(ShouldRun::n_times(42))

Copy link
Member

Choose a reason for hiding this comment

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

Use const generics. ShouldRun::n_times::<{42}>.

Good point. There are cases where this would significantly increase the amount of internal statefullness / Local hackery required (ex: a timer that ticks down from 42 rather than up from 0 would need to have an init phase enforced with locals). But it would have the benefit of not using function calls, so its worth considering. It feels like there might be cases where closures are still required though (ex: the input is set at runtime).

Copy link
Member Author

@DJMcNab DJMcNab Apr 10, 2022

Choose a reason for hiding this comment

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

This discussion shouldn't block this PR landing, right?

I think in a lot of cases, we can avoid having systems being externally configurable. At some point, we'll probably want something Local-like initialised from a const parameter. Not sure if that's possible at the moment.

My preference is option 2. Option 2 keeps the rules for struct scoped systems the same as module scoped systems.
It's worth noting that as far as I know, the _system suffix is idiomatic for functions which return system functions, which applies both when struct scoped and module scoped.

Edit: Change from option 3 to option 2, since I got it wrong.

Copy link
Member

Choose a reason for hiding this comment

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

This discussion shouldn't block this PR landing, right?

Only in that it would be a breaking change to switch from the current style to a "return FnMut for everything on a struct" style. I do think "only use FnMut when you need it" is pretty consistent with what we do today (there is already precedent for the current pattern in bevy).

So yeah Option 2 seems like a reasonable thing to default to. Lets roll with that and we can revisit later if we feel inclined.

@DJMcNab DJMcNab 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 Apr 10, 2022
@alice-i-cecile alice-i-cecile modified the milestones: Bevy 0.7, Bevy 0.8 Apr 25, 2022
@cart
Copy link
Member

cart commented May 4, 2022

bors r+

bors bot pushed a commit that referenced this pull request May 4, 2022
# Objective

- `RunOnce` was a manual `System` implementation.
- Adding run criteria to stages was yet to be systemyoten

## Solution

- Make it a normal function
- yeet

##  Changelog

- Replaced `RunOnce` with `ShouldRun::once`

## Migration guide

The run criterion `RunOnce`, which would make the controlled systems run only once, has been replaced with a new run criterion function `ShouldRun::once`. Replace all instances of `RunOnce` with `ShouldRun::once`.
@bors bors bot changed the title Make RunOnce a non-manual System impl [Merged by Bors] - Make RunOnce a non-manual System impl May 4, 2022
@bors bors bot closed this May 4, 2022
@DJMcNab DJMcNab deleted the runonce_and_yeet branch May 4, 2022 18:59
robtfm pushed a commit to robtfm/bevy that referenced this pull request May 10, 2022
# Objective

- `RunOnce` was a manual `System` implementation.
- Adding run criteria to stages was yet to be systemyoten

## Solution

- Make it a normal function
- yeet

##  Changelog

- Replaced `RunOnce` with `ShouldRun::once`

## Migration guide

The run criterion `RunOnce`, which would make the controlled systems run only once, has been replaced with a new run criterion function `ShouldRun::once`. Replace all instances of `RunOnce` with `ShouldRun::once`.
exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022
# Objective

- `RunOnce` was a manual `System` implementation.
- Adding run criteria to stages was yet to be systemyoten

## Solution

- Make it a normal function
- yeet

##  Changelog

- Replaced `RunOnce` with `ShouldRun::once`

## Migration guide

The run criterion `RunOnce`, which would make the controlled systems run only once, has been replaced with a new run criterion function `ShouldRun::once`. Replace all instances of `RunOnce` with `ShouldRun::once`.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- `RunOnce` was a manual `System` implementation.
- Adding run criteria to stages was yet to be systemyoten

## Solution

- Make it a normal function
- yeet

##  Changelog

- Replaced `RunOnce` with `ShouldRun::once`

## Migration guide

The run criterion `RunOnce`, which would make the controlled systems run only once, has been replaced with a new run criterion function `ShouldRun::once`. Replace all instances of `RunOnce` with `ShouldRun::once`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-App Bevy apps and plugins A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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.

4 participants