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

Nward/commands error handling #11184

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

EngoDev
Copy link

@EngoDev EngoDev commented Jan 2, 2024

Objective

Fixes: #2004

Original PR: #2241
Thank you very much @NathanSWard for all your hard work! ⭐ I mostly just updated their code to work with the current version of bevy.

Also this is my first bevy PR, feedback is very much appreciated :)

Changelog

Added

  • FallibleCommand

    • this is for command that can potentially fail, and allow people to provide error handlers if an error occurs
  • CommandErrorHandling

    • this is for function like types that can handle command errors.
  • CommandErrorHandler

    • Contains handlers for typical use cases of handling errors like:
      • ignore
      • panic
      • log - This is the default behavior instead of panic. The error will be logged via error!.

Changed

If a command is fallible, then new return type is a Config type.
The Config type allows a user to optionally provide an error handler via on_err.

Example

#[derive(Component)]
struct Marker(u32);

#[derive(Resource)]
struct Food(u32);

fn system(mut commands: Commands) {
    commands.entity(e)
        .insert(Marker(32))
        .on_err(|error, ctx| { /*..*/ });

    command
        .remove_resource::<Food>()
        .on_err(CommandErrorHandler::ignore);

    commands
        .entity(e)
        .despawn()
        .on_err(CommandErrorHandler::panic);

    commands
        .entity(e)
        .insert(Marker(32))
        .insert(Marker(64)); // you can also ignore the error handling and just chain regular commands
}

Copy link
Contributor

github-actions bot commented Jan 2, 2024

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

Copy link
Contributor

github-actions bot commented Jan 2, 2024

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

Copy link
Contributor

github-actions bot commented Jan 2, 2024

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

1 similar comment
Copy link
Contributor

github-actions bot commented Jan 2, 2024

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@matiqo15 matiqo15 added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events X-Controversial There is active debate or serious implications around merging this PR labels Jan 2, 2024
@SecretPocketCat
Copy link
Contributor

This's anecdotal, but I've hit spurious panics in several bevy jam 4 entries and this could help remedy some of those cases (the rest would probly be handled by relations 🌈🤞), so this would be nice.

That being said, should the default behaviour change from panicking to logging? I think I'm in favour of that approach, but would it be possible to make that configurable for those that would like to default to panicking?

@dmyyy
Copy link
Contributor

dmyyy commented Jan 2, 2024

@SecretPocketCat There's an issue tracking commands needing to be infallible whenever possible here. General consensus seems to be that it's needed - just needs to be implemented.

@hymm hymm added the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label Jan 3, 2024
@EngoDev
Copy link
Author

EngoDev commented Jan 3, 2024

@SecretPocketCat I actually took this issue because I was getting hit with that exact problem when developing my game for the bevy jam and now with my current project, on both occasions I tried to debug extensively but couldn't find where the multiple despawns happened and it was irritating, it feels like you get no control over your program. I believe that crates should never panic and give the user the ability to decide what to do with the error, after all Rust already has great support for Result why not use it right? :)

You raise a good point about making the default behavior configurable, as @dmyyy noted it was already discussed in the issue and in the previous PR. I suggest maybe adding a feature flag that will allow to change the default behavior back to panic, what do you think?

I also see that the Needs-Benchmarking label has been added, @hymm what do you think would be the best approach for starting to benchmark this solution?

@hymm
Copy link
Contributor

hymm commented Jan 4, 2024

@hymm what do you think would be the best approach for starting to benchmark this solution?

In the benches folder there is a bunch of benchmarks. Probably should adapt at least the commands/spawn tests to show perf both with and without the error handler. We might want to do all the benches in the "commands" folder. But I'm not sure if the other command types would show anything significantly different.

How to run benchmarks

  1. On this pr commit, change to the benches folder and run cargo bench commands -- --save-baseline pr. commands filters to just the benchmarks with "commands" in their name. "pr" saves the results with that name.
  2. Change to the main branch commit that this pr is based off of and run cargo bench commands -- --save-baseline main.
  3. Use the critcmp crate to get some nice output by running critcmp pr main. Copy and paste that output into github to share it.

If your system has a tendency to thermally throttle (i.e. using a laptop), you may need to do something like fixing your cpu clocks to the base clock speed and/or use eco mode to reduce noise.

@SecretPocketCat
Copy link
Contributor

You raise a good point about making the default behavior configurable, as @dmyyy noted it was already discussed in the issue and in the previous PR. I suggest maybe adding a feature flag that will allow to change the default behavior back to panic, what do you think?

Sounds reasonable, but maybe I was bringing up smt. that should be just a follow-up PR instead.

Copy link
Contributor

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

Copy link
Contributor

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@EngoDev
Copy link
Author

EngoDev commented Jan 22, 2024

could you commit the spawn_commands_error_handler benches? I can try to run them.

Sorry, I thought I commited those changes. Added them now. Thank you for the support :)

@EngoDev
Copy link
Author

EngoDev commented Jan 22, 2024

I ran the tests locally a few times (I did cargo clean), everything is green. Is it possible the CI has a cache problem?

@hymm
Copy link
Contributor

hymm commented Jan 22, 2024

For some reason there's some difference between the errors in CI and some platforms. You should just use the error messages that CI has and accept that your local compile fail tests are going to fail.

@EngoDev
Copy link
Author

EngoDev commented Jan 22, 2024

For some reason there's some difference between the errors in CI and some platforms. You should just use the error messages that CI has and accept that your local compile fail tests are going to fail.

Good to know! I reverted the change and the CI seems to run fine now

@EngoDev
Copy link
Author

EngoDev commented Jan 27, 2024

@hymm Did you get a chance to run the benchmark yourself?

@hymm
Copy link
Contributor

hymm commented Jan 27, 2024

Did you get a to run the benchmark yourself?

Going to try and do it today. I ran them before without the new benches and was seeing a regression on fake commands. I'll post my results in a bit.

@EngoDev
Copy link
Author

EngoDev commented Jan 27, 2024

Did you get a to run the benchmark yourself?

Going to try and do it today. I ran them before without the new benches and was seeing a regression on fake commands. I'll post my results in a bit.

Looking forward to seeing the results, thank you! ⭐

@hymm
Copy link
Contributor

hymm commented Jan 28, 2024

If there's any changes it's pretty much in the noise. Ran them on main and this pr 3 times each. Depending on which run one could be faster than the other, but all the runs were in the same range. Good to see that making the error handler configurable didn't slow things down.

group                                         main-1                                 pr-1
-----                                         ------                                 ----
empty_commands/0_entities                     1.08      3.7±0.27ns        ? ?/sec    1.00      3.4±0.07ns        ? ?/sec
fake_commands/2000_commands                   1.03      6.0±0.10µs        ? ?/sec    1.00      5.8±0.22µs        ? ?/sec
fake_commands/4000_commands                   1.04     12.0±0.11µs        ? ?/sec    1.00     11.5±0.28µs        ? ?/sec
fake_commands/6000_commands                   1.03     18.0±0.18µs        ? ?/sec    1.00     17.5±0.78µs        ? ?/sec
fake_commands/8000_commands                   1.04     24.0±0.32µs        ? ?/sec    1.00     23.1±0.32µs        ? ?/sec
insert_commands/insert                        1.00   259.7±17.72µs        ? ?/sec    1.02   263.8±16.03µs        ? ?/sec
insert_commands/insert_batch                  1.00   234.8±19.76µs        ? ?/sec    1.02   239.5±21.89µs        ? ?/sec
sized_commands_0_bytes/2000_commands          1.07      3.7±0.15µs        ? ?/sec    1.00      3.5±0.28µs        ? ?/sec
sized_commands_0_bytes/4000_commands          1.00      7.5±0.19µs        ? ?/sec    1.01      7.5±1.63µs        ? ?/sec
sized_commands_0_bytes/6000_commands          1.09     11.1±0.11µs        ? ?/sec    1.00     10.2±0.55µs        ? ?/sec
sized_commands_0_bytes/8000_commands          1.09     15.0±0.84µs        ? ?/sec    1.00     13.8±1.16µs        ? ?/sec
sized_commands_12_bytes/2000_commands         1.13      5.3±1.23µs        ? ?/sec    1.00      4.7±0.21µs        ? ?/sec
sized_commands_12_bytes/4000_commands         1.00      9.3±0.14µs        ? ?/sec    1.03      9.6±0.52µs        ? ?/sec
sized_commands_12_bytes/6000_commands         1.00     13.9±0.32µs        ? ?/sec    1.01     14.0±0.21µs        ? ?/sec
sized_commands_12_bytes/8000_commands         1.00     18.7±0.22µs        ? ?/sec    1.02     19.0±0.49µs        ? ?/sec
sized_commands_512_bytes/2000_commands        1.00     47.6±1.89µs        ? ?/sec    1.09     51.7±3.07µs        ? ?/sec
sized_commands_512_bytes/4000_commands        1.00     97.6±8.97µs        ? ?/sec    1.06    103.1±9.70µs        ? ?/sec
sized_commands_512_bytes/6000_commands        1.00   150.2±23.74µs        ? ?/sec    1.06   158.8±23.11µs        ? ?/sec
sized_commands_512_bytes/8000_commands        1.00   201.8±30.58µs        ? ?/sec    1.05   212.5±36.55µs        ? ?/sec
spawn_commands/2000_entities                  1.00   151.7±11.19µs        ? ?/sec    1.01   153.9±17.37µs        ? ?/sec
spawn_commands/4000_entities                  1.00   294.7±25.83µs        ? ?/sec    1.04   306.4±34.32µs        ? ?/sec
spawn_commands/6000_entities                  1.00   434.7±29.80µs        ? ?/sec    1.05   454.3±30.51µs        ? ?/sec
spawn_commands/8000_entities                  1.01   606.8±85.29µs        ? ?/sec    1.00   603.1±37.72µs        ? ?/sec
spawn_commands_error_handler/2000_entities                                           1.00   145.5±13.30µs        ? ?/sec
spawn_commands_error_handler/4000_entities                                           1.00   326.6±65.51µs        ? ?/sec
spawn_commands_error_handler/6000_entities                                           1.00   446.8±52.50µs        ? ?/sec
spawn_commands_error_handler/8000_entities                                           1.00   603.7±37.74µs        ? ?/sec

@EngoDev
Copy link
Author

EngoDev commented Jan 28, 2024

If there's any changes it's pretty much in the noise. Ran them on main and this pr 3 times each. Depending on which run one could be faster than the other, but all the runs were in the same range. Good to see that making the error handler configurable didn't slow things down.

group                                         main-1                                 pr-1
-----                                         ------                                 ----
empty_commands/0_entities                     1.08      3.7±0.27ns        ? ?/sec    1.00      3.4±0.07ns        ? ?/sec
fake_commands/2000_commands                   1.03      6.0±0.10µs        ? ?/sec    1.00      5.8±0.22µs        ? ?/sec
fake_commands/4000_commands                   1.04     12.0±0.11µs        ? ?/sec    1.00     11.5±0.28µs        ? ?/sec
fake_commands/6000_commands                   1.03     18.0±0.18µs        ? ?/sec    1.00     17.5±0.78µs        ? ?/sec
fake_commands/8000_commands                   1.04     24.0±0.32µs        ? ?/sec    1.00     23.1±0.32µs        ? ?/sec
insert_commands/insert                        1.00   259.7±17.72µs        ? ?/sec    1.02   263.8±16.03µs        ? ?/sec
insert_commands/insert_batch                  1.00   234.8±19.76µs        ? ?/sec    1.02   239.5±21.89µs        ? ?/sec
sized_commands_0_bytes/2000_commands          1.07      3.7±0.15µs        ? ?/sec    1.00      3.5±0.28µs        ? ?/sec
sized_commands_0_bytes/4000_commands          1.00      7.5±0.19µs        ? ?/sec    1.01      7.5±1.63µs        ? ?/sec
sized_commands_0_bytes/6000_commands          1.09     11.1±0.11µs        ? ?/sec    1.00     10.2±0.55µs        ? ?/sec
sized_commands_0_bytes/8000_commands          1.09     15.0±0.84µs        ? ?/sec    1.00     13.8±1.16µs        ? ?/sec
sized_commands_12_bytes/2000_commands         1.13      5.3±1.23µs        ? ?/sec    1.00      4.7±0.21µs        ? ?/sec
sized_commands_12_bytes/4000_commands         1.00      9.3±0.14µs        ? ?/sec    1.03      9.6±0.52µs        ? ?/sec
sized_commands_12_bytes/6000_commands         1.00     13.9±0.32µs        ? ?/sec    1.01     14.0±0.21µs        ? ?/sec
sized_commands_12_bytes/8000_commands         1.00     18.7±0.22µs        ? ?/sec    1.02     19.0±0.49µs        ? ?/sec
sized_commands_512_bytes/2000_commands        1.00     47.6±1.89µs        ? ?/sec    1.09     51.7±3.07µs        ? ?/sec
sized_commands_512_bytes/4000_commands        1.00     97.6±8.97µs        ? ?/sec    1.06    103.1±9.70µs        ? ?/sec
sized_commands_512_bytes/6000_commands        1.00   150.2±23.74µs        ? ?/sec    1.06   158.8±23.11µs        ? ?/sec
sized_commands_512_bytes/8000_commands        1.00   201.8±30.58µs        ? ?/sec    1.05   212.5±36.55µs        ? ?/sec
spawn_commands/2000_entities                  1.00   151.7±11.19µs        ? ?/sec    1.01   153.9±17.37µs        ? ?/sec
spawn_commands/4000_entities                  1.00   294.7±25.83µs        ? ?/sec    1.04   306.4±34.32µs        ? ?/sec
spawn_commands/6000_entities                  1.00   434.7±29.80µs        ? ?/sec    1.05   454.3±30.51µs        ? ?/sec
spawn_commands/8000_entities                  1.01   606.8±85.29µs        ? ?/sec    1.00   603.1±37.72µs        ? ?/sec
spawn_commands_error_handler/2000_entities                                           1.00   145.5±13.30µs        ? ?/sec
spawn_commands_error_handler/4000_entities                                           1.00   326.6±65.51µs        ? ?/sec
spawn_commands_error_handler/6000_entities                                           1.00   446.8±52.50µs        ? ?/sec
spawn_commands_error_handler/8000_entities                                           1.00   603.7±37.74µs        ? ?/sec

Yes! I'm very happy to see that, thank you for the thorough benchmarking :)
How can we proceed for this PR to be reviewed and merged?

@hymm hymm self-requested a review January 28, 2024 19:23
@hymm
Copy link
Contributor

hymm commented Jan 28, 2024

How can we proceed for this PR to be reviewed and merged?

Needs 2 approvals from ECS-SME's, since this is controversial. Community approval can be helpful too. I'm not an ECS-SME, but I'll try to review sometime in the next couple weeks.

@hymm hymm removed the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label Jan 29, 2024
@EngoDev
Copy link
Author

EngoDev commented Jan 29, 2024

How can we proceed for this PR to be reviewed and merged?

Needs 2 approvals from ECS-SME's, since this is controversial. Community approval can be helpful too. I'm not an ECS-SME, but I'll try to review sometime in the next couple weeks.

Oh good to know, I look forward for your review :)

@hymm hymm added this to the 0.14 milestone Mar 4, 2024
@alice-i-cecile alice-i-cecile added S-Needs-SME Decision or review from an SME is required S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 16, 2024
@alice-i-cecile
Copy link
Member

Sorry for not noticing this! I will get to this in the 0.15 cycle: this is important and valuable work that I think we can build on. Not a good last minute addition though, so I'm cutting from the 0.14 milestone.

@alice-i-cecile alice-i-cecile modified the milestones: 0.14, 0.15 May 16, 2024
@bas-ie
Copy link
Contributor

bas-ie commented Sep 27, 2024

Hi @EngoDev! Just checking in, any chance you could update your branch, I know it's been awhile 🙂 might make it easier to get some reviewin' eyes on it.

@EngoDev
Copy link
Author

EngoDev commented Sep 27, 2024

Hi @EngoDev! Just checking in, any chance you could update your branch, I know it's been awhile 🙂 might make it easier to get some reviewin' eyes on it.

Of course! Thank you, I'll get on it today

@BenjaminBrienen BenjaminBrienen added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Sep 27, 2024
@alice-i-cecile alice-i-cecile removed this from the 0.15 milestone Sep 30, 2024
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Oct 16, 2024
@BenjaminBrienen BenjaminBrienen removed the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Oct 31, 2024
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-Needs-SME Decision or review from an SME is required S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Controversial There is active debate or serious implications around merging this PR
Projects
Status: Open PR
Development

Successfully merging this pull request may close these issues.

Configurable error handling in commands
9 participants