Add `Result` handling to `Commands` and `EntityCommands` #17043

JaySpruce · 2024-12-30T18:38:49Z

Objective

todo!();

Redux of #15929
Fixes #2004
Fixes #3845
Fixes #7118
Fixes #10166

Solution

todo!();

Benchmarks

This PR compared to main (all benchmarks with "commands" in their name)

Future Updates

After a universal error handling mode is added, we can change all commands to fail that way by default.
- Once we have all commands failing the same way (which would require either the full removal of try variants or just making them useless while they're deprecated), queue_fallible_with_default could be removed, since its only purpose is to enable commands having different defaults.

commit b6c4d28 Author: JaySpruce <[email protected]> Date: Sun Dec 29 17:17:44 2024 -0600 ci docs commit 492e558 Author: JaySpruce <[email protected]> Date: Sun Dec 29 16:59:31 2024 -0600 messed up the conflict resolve commit 74592fa Merge: e0c6d65 0f2b2de Author: JaySpruce <[email protected]> Date: Sun Dec 29 16:48:37 2024 -0600 Merge branch 'main' into refactor_hierarchy_commands commit e0c6d65 Author: JaySpruce <[email protected]> Date: Sun Dec 29 13:28:07 2024 -0600 refactor to remove structs

JaySpruce · 2024-12-30T18:42:36Z

Pinging @alice-i-cecile as requested. Not totally polished, but I felt like this could use some feedback/cooperation in regards to other fallible stuff going on.

alice-i-cecile · 2024-12-30T19:27:24Z

crates/bevy_ecs/src/system/commands/mod.rs

+        self
+    }
+
+    ///


Reminder to finish these doc strings before we merge :)

alice-i-cecile · 2024-12-30T19:28:47Z

crates/bevy_ecs/src/system/commands/mod.rs

 ///
 /// # Note
 ///
 /// This won't clean up external references to the entity (such as parent-child relationships
 /// if you're using `bevy_hierarchy`), which may leave the world in an invalid state.
 #[track_caller]
-fn despawn(log_warning: bool) -> impl EntityCommand {
+fn despawn() -> impl EntityCommand<World> {
+    #[cfg(feature = "track_change_detection")]


This change seems correct regardless, maybe we should split this out? Ditto the docs above.

I think that would need the impl EntityCommand for impl FnOnce(EntityWorldMut) changes. I do think that should be pulled out of this pr as that is something that could be considered separately.

alice-i-cecile · 2024-12-30T19:29:47Z

crates/bevy_ecs/src/world/error.rs

-#[derive(Clone, Copy)]
-pub enum EntityFetchError<'w> {
+#[derive(Error, Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EntityFetchError {


Wow I much prefer removing the lifetime + UnsafeWorldCell from this. Can we split this out?

crates/bevy_ecs/src/world/mod.rs

alice-i-cecile · 2024-12-30T19:33:28Z

crates/bevy_ecs/src/world/mod.rs

+    /// Fail silently.
+    Silent,
+    /// Send a warning.
+    Warn,


Follow-up: we should add an error level too.

alice-i-cecile · 2024-12-30T19:35:12Z

I'm willing to eat a 5% (worst case) performance regression on Commands here. This is a huge usability problem with Bevy, and I'm convinced that we can't solve this without somehow regressing perf there. The big performance gains to be found here are in command batching anyways.

NthTensor · 2024-12-30T20:30:54Z

I'd like to do a proper review of this, if I can find the time. Writing this as a reminder to myself.

crates/bevy_ecs/src/world/mod.rs

alice-i-cecile · 2024-12-31T00:41:32Z

@EngoDev can I get your opinions on this work? I think I prefer the basic idea here over that of #11184. Making commands fallible by default (with commands that can't fail always returning Ok(())) feels like the right direction.

EngoDev · 2024-12-31T00:58:02Z

@EngoDev can I get your opinions on this work? I think I prefer the basic idea here over that of #11184. Making commands fallible by default (with commands that can't fail always returning Ok(())) feels like the right direction.

I agree with you, this approach makes a lot of sense. I do think we can find a more optimized solution though. Having proper error handling for commands is worth the 5% decrease in performance but I'm positive we can minimize it.

I'll try to find time over the weekend to play around with this PR and give more based feedback.

alice-i-cecile · 2024-12-31T16:30:07Z

crates/bevy_ecs/src/system/commands/mod.rs

@@ -81,6 +86,10 @@ pub use parallel_scope::*;
 pub struct Commands<'w, 's> {
    queue: InternalQueue<'s>,
    entities: &'w Entities,
+    /// How the [`Commands`] instance will response upon encountering an error.
+    ///
+    /// If this is unset ([`None`]), commands can choose their error mode individually.


This feels weird to me. Why would users want to set the error handling at the level of a Commands queue?

My mental model is:

Global default, set per project and per build mode (release, alpha testing, dev...)

Per command overrides to completely silence useless warnings or respond to failures by recovering.

This feels like an intermediate level that doesn't serve anyone well.

I agree with this assessment. I'll add that currently the global handling mode is effectively based on Box<dyn Error> and always panics.

It's the only way I could think for it to work without vastly changing how commands work. Calling queue immediately turns the command into a bunch of unknown bytes, so if you want to customize the command, it has to know what you want before queue is called

Alright, I'm fine with this for now. Leave a comment to this effect on this field please :)

Actually, here's an alternative: we could re-add those helper functions we just removed that return the commands as closures, and we could make them pub to let people queue built-in commands with their own error handlers:
commands.entity(entity).queue_with(insert(bundle), |world, error| { /* whatever */ });

alice-i-cecile · 2024-12-31T16:35:43Z

crates/bevy_ecs/src/world/mod.rs

+    ///
+    /// `fn()` can be a closure if it doesn't capture its environment.
+    ///
+    /// If `CommandErrorMode` is used in a situation without `&mut World` access


This seems like an odd situation. What methods would try to resolve and handle commands without exclusive world access?

alice-i-cecile · 2024-12-31T16:38:40Z

crates/bevy_ecs/src/system/commands/mod.rs

+                CommandErrorMode::Error => error!(
+                    "Attempted to create an EntityCommands for entity {entity}, which does not exist; returned invalid EntityCommands"
+                ),
+                CommandErrorMode::Panic | CommandErrorMode::Custom(_) => panic!(


Aha, here's the panic. This should be called out more clearly at least, but I think that passing down a more complex Result or adding an error arm is the better way. The custom error handler should be able to know whether or not the entity failed to exist at all, and respond accordingly.

I was kind of hoping we wouldn't have to put handling logic on this level. I feel like this sort of match statement should ideally only appear once, and everything should be funneled through it on a lower level. That's just a feeling on my part.

Do we need to remove the panic here? I feel like if the user doesn't want it to panic they should just be using get_entity. I feel like this pr should just be focused on the errors for when commands are applied.

Eventually I think the plan is to collapse our panicking and non-panicking variants into the same call.

Yeah, I'd like to remove get_entity completely in favor of this error handling technique. It's frustrating that there's so many different edge cases to guard for, and no unified way to make sure you've handled them all.

That makes sense, but I still feel like this pr should be focused on the deferred errors. Or are you saying that creating a EntityCommands will only panic when the command queue is applied?

That makes sense, but I still feel like this pr should be focused on the deferred errors

I agree with that, but I think we can do some future-proofing here. It's fine to leave it for follow-up though: I just think it's a bit messy as is.

alice-i-cecile · 2024-12-31T16:39:15Z

crates/bevy_ecs/src/system/commands/mod.rs

+    ///
+    /// `fn()` can be a closure if it doesn't capture its environment.
+    ///
+    /// If an error is encountered in a situation without `&mut World` access


Same note here about docs / alternative fix for this problem.

alice-i-cecile · 2024-12-31T16:49:38Z

crates/bevy_ecs/src/world/mod.rs

+    ///
+    /// If `CommandErrorMode` is used in a situation without `&mut World` access
+    /// (e.g. a non-deferred [`Commands`] method), this mode will instead panic.
+    Custom(fn(&mut World, Result)),


I thought about this a bit more: this should actually have a Box<dyn Error>, not a Result. There's no need for error handling if everything went okay, so we can save our users some repetitive matching.

Then, combined with the ideas about how commands.entity(my_entity) shouldn't panic, we get:

enum CommandError { NoSuchEntity(Entity), CommandFailed(Box<dyn Error>) }

which we pass to the custom error handlers to give them the context needed to solve the problem.

NthTensor · 2024-12-31T17:30:21Z

I like this direction a lot, but I do have a gripe with CommandErrorMode. It seems to me like this should not be an enum, but rather just make everything an Option<fn(&mut World, Box<dyn Error>)>. Then we can have a hot path where it is None and the errors bubble up to the handler embedded in the schedule (through a fallible ApplyDeferred), and we can have a #[cold] path that calls the function as part of command application.

You have queue_fallible insert the None and something like a queue_fallible_with(command, panic_on_error) where panic_on_error is a normal function.

These are all suggestions mind you. You're doing great work, and I wouldn't block the current line of work over any of this. It just strikes me that using function pointers might be more extensible. It might also be lower overhead with a specific #[cold] path, idk.

hymm · 2024-12-31T17:49:48Z

crates/bevy_ecs/src/system/commands/mod.rs

@@ -444,23 +457,25 @@ impl<'w, 's> Commands<'w, 's> {
    #[inline]
    #[track_caller]
    pub fn entity(&mut self, entity: Entity) -> EntityCommands {
-        #[inline(never)]
-        #[cold]
-        #[track_caller]


Some of the perf regressions are probably from losing these annotations. We should try to add them back.

Good catch, that would do it.

I did smaller benchmarks before and after making that change (only the insert_commands::insert benchmark, because that had been the most volatile) and didn't see any difference. We're aiming to remove the panic from entity anyway, I'm not sure there's a place for those

hymm · 2024-12-31T20:20:17Z

Hmm, I'm trying to figure out what would even cause regressions here if it's not the #[cold] annotation. I feel like there hasn't been much change to the code paths for fake_commands and sized_commands_0_bytes. The only thing I really see is that we now need to drop the result from command.apply. Not sure what else it could be.

https://github.com/bevyengine/bevy/pull/17043/files#diff-badf600e116f81aa58524486dea99ee4bb337143cca99a57339dda74f4661bc4R190

JaySpruce · 2024-12-31T20:27:39Z

I think I'll revert entity anyway, handling the error there doesn't really fit and changing it to return Result would make a massive diff (344 references across 144 files...) which ought to be its own PR

JaySpruce · 2024-12-31T22:50:42Z

How would y'all feel about something like this:

pub mod handler {
	pub fn log_warning() -> fn(&mut World, Box<dyn Error>) {
		|_, error| warn!("{error}")
	}
	[more of them]
}

pub mod command {
	pub fn insert(impl Bundle) -> impl EntityCommand {
		move |EntityWorldMut| {
			/*command code*/
		}
	}
	[more of them]
}

So that users could do this if they wanted:
commands.entity(entity).queue_with(command::insert(bundle), handler::log_warning());

I'm unsure what the stance is on using pub mod like namespaces.

this isn't relevant to this PR at all anymore, it should be separate

JaySpruce added 7 commits December 29, 2024 18:36

initial error handling

9c4884b

incorporate command struct refactors

1d63497

remove String from EntityFetchError, fix benches

b8d4938

rename queue_with_error_handling to queue_fallible

0d75dac

let Commands::entity return invalid EntityCommands

62abe41

forgot about EntityEntryCommands

1ea5b50

alice-i-cecile requested review from alice-i-cecile and NthTensor December 30, 2024 19:24

alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Dec 30, 2024

alice-i-cecile reviewed Dec 30, 2024

View reviewed changes

crates/bevy_ecs/src/system/commands/mod.rs Outdated

self

}

///

Copy link

Member

alice-i-cecile Dec 30, 2024

Choose a reason for hiding this comment

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

Reminder to finish these doc strings before we merge :)

alice-i-cecile reviewed Dec 30, 2024

View reviewed changes

crates/bevy_ecs/src/world/mod.rs Show resolved Hide resolved

alice-i-cecile reviewed Dec 30, 2024

View reviewed changes

crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Dec 30, 2024

View reviewed changes

JaySpruce added 2 commits December 30, 2024 16:06

add Error and Override error modes

5028d02

Merge branch 'bevyengine:main' into command_error_handling

60ddd45

alice-i-cecile mentioned this pull request Dec 31, 2024

Allow users to choose how Commands and EntityCommands should fail #15929

Closed

alice-i-cecile reviewed Dec 31, 2024

View reviewed changes

crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved

alice-i-cecile mentioned this pull request Dec 31, 2024

Nward/commands error handling #11184

Closed

JaySpruce added 3 commits December 30, 2024 23:21

give world and error to custom error mode, small renames

819897c

add docs

0e0eb15

maintain error_mode on reborrow

e814dbd

alice-i-cecile reviewed Dec 31, 2024

View reviewed changes

hymm reviewed Dec 31, 2024

View reviewed changes

JaySpruce added 7 commits January 1, 2025 19:56

implement suggestions, try out the above idea

ae978e6

Merge branch 'main' into command_error_handling

51f0634

fix track_location

eb3503d

fix EntityFetchError::NoSuchEntity

1708b44

revert all EntityFetchError changes (we have our own now)

97bffd9

this isn't relevant to this PR at all anymore, it should be separate

cargo fmt

2bc92c6

nevermind we do need it lmao

9e48772

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Result` handling to `Commands` and `EntityCommands` #17043

Add `Result` handling to `Commands` and `EntityCommands` #17043

JaySpruce commented Dec 30, 2024 •

edited

Loading

JaySpruce commented Dec 30, 2024

alice-i-cecile Dec 30, 2024

alice-i-cecile Dec 30, 2024

hymm Dec 31, 2024

alice-i-cecile Dec 30, 2024

alice-i-cecile Dec 30, 2024

alice-i-cecile commented Dec 30, 2024

NthTensor commented Dec 30, 2024

alice-i-cecile commented Dec 31, 2024

EngoDev commented Dec 31, 2024

alice-i-cecile Dec 31, 2024

NthTensor Dec 31, 2024

JaySpruce Dec 31, 2024

alice-i-cecile Dec 31, 2024

JaySpruce Dec 31, 2024

alice-i-cecile Dec 31, 2024

alice-i-cecile Dec 31, 2024

NthTensor Dec 31, 2024

hymm Dec 31, 2024 •

edited

Loading

NthTensor Dec 31, 2024

alice-i-cecile Dec 31, 2024

hymm Dec 31, 2024

alice-i-cecile Dec 31, 2024

alice-i-cecile Dec 31, 2024

alice-i-cecile Dec 31, 2024

NthTensor commented Dec 31, 2024 •

edited

Loading

hymm Dec 31, 2024

NthTensor Dec 31, 2024

JaySpruce Dec 31, 2024 •

edited

Loading

hymm commented Dec 31, 2024

JaySpruce commented Dec 31, 2024

JaySpruce commented Dec 31, 2024 •

edited

Loading

+                      self
+                  }
+                  ///

Add Result handling to Commands and EntityCommands #17043

Are you sure you want to change the base?

Add Result handling to Commands and EntityCommands #17043

Conversation

JaySpruce commented Dec 30, 2024 • edited Loading

Objective

Solution

Benchmarks

Future Updates

JaySpruce commented Dec 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile commented Dec 30, 2024

NthTensor commented Dec 30, 2024

alice-i-cecile commented Dec 31, 2024

EngoDev commented Dec 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hymm Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NthTensor commented Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaySpruce Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

hymm commented Dec 31, 2024

JaySpruce commented Dec 31, 2024

JaySpruce commented Dec 31, 2024 • edited Loading

Add `Result` handling to `Commands` and `EntityCommands` #17043

Add `Result` handling to `Commands` and `EntityCommands` #17043

JaySpruce commented Dec 30, 2024 •

edited

Loading

hymm Dec 31, 2024 •

edited

Loading

NthTensor commented Dec 31, 2024 •

edited

Loading

JaySpruce Dec 31, 2024 •

edited

Loading

JaySpruce commented Dec 31, 2024 •

edited

Loading