Add try_despawn methods to World/Commands #15480
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alice-i-cecile
added
D-Trivial
|
@alice-i-cecile added in the |
MiniaczQ
reviewed
Oct 1, 2024
Comment on lines
23
to
36
| /// Despawns the given entity and all its children recursively without warnings | ||
| #[derive(Debug)] | ||
| pub struct TryDespawnRecursive { | ||
| /// Target entity | ||
| pub entity: Entity, | ||
| } | ||
|
|
||
| /// Despawns the given entity's children recursively without warnings | ||
| #[derive(Debug)] | ||
| pub struct TryDespawnChildrenRecursive { | ||
| /// Target entity | ||
| pub entity: Entity, | ||
| } | ||
|
|
There was a problem hiding this comment.
We can add warn flag to the existing commands
There was a problem hiding this comment.
I'm somewhat against this due to the additional skew it creates for the PR, and that this introduces a breaking change. A similar pattern of non-try/try pairing exists for Query::single and Query::get_single, which I would prefer here as well.
There was a problem hiding this comment.
Just pushed some commits with this change though.
There was a problem hiding this comment.
Yeah, I just noticed the new changes
How I see it is, we can keep user command/world API like despawn, try_despawn
but all the internals can just use the flag, so that's command structs and functions that do the despawning
that way we have no breaking changes and we can keep the internals concise
There was a problem hiding this comment.
Sorry, if I've been a bit clearer it'd save you some work
There was a problem hiding this comment.
My understanding is that the DespawnRecursive and DespawnChildrenRecursive command structs are exposed to users, so this would still introduce a breaking change, no?
There was a problem hiding this comment.
A lot of the engine is public to the user if they desire to access it,
but I'd consider the despawn, etc. methods to be the main way of using this API
So changing the command structs wouldn't be considered breaking change for typical user
I hope that makes sense?
MiniaczQ
reviewed
Oct 1, 2024
MiniaczQ
reviewed
Oct 1, 2024
MiniaczQ
reviewed
Oct 1, 2024
MiniaczQ
reviewed
Oct 1, 2024
|
@MiniaczQ PTAL at the changes |
|
@mockersf @janhohenheim PTAL when you get the chance, thanks! |
alice-i-cecile
added
S-Ready-For-Final-Review
|
Related to #2004 and #14231. @rudderbucky, have you benchmarked this to make sure we're not getting any performance regressions when despawning large numbers of entities due to the branching? |
|
Here is the control: And this branch: The outcomes do change basically every time I run this, even with N=5000. @alice-i-cecile up to you for interpretation |
|
I'm going to call this "basically within noise". As a scientist, I don't have any degree of confidence that there's a regression here, and if it is, it's a small regression on a minor (but not zero) operation. Merging. |
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Oct 4, 2024
# Objective Fixes bevyengine#14511. `despawn` allows you to remove entities from the world. However, if the entity does not exist, it emits a warning. This may not be intended behavior for many users who have use cases where they need to call `despawn` regardless of if the entity actually exists (see the issue), or don't care in general if the entity already doesn't exist. (Also trying to gauge interest on if this feature makes sense, I'd personally love to have it, but I could see arguments that this might be a footgun. Just trying to help here 😄 If there's no contention I could also implement this for `despawn_recursive` and `despawn_descendants` in the same PR) ## Solution Add `try_despawn`, `try_despawn_recursive` and `try_despawn_descendants`. Modify `World::despawn_with_caller` to also take in a `warn` boolean argument, which is then considered when logging the warning. Set `log_warning` to `true` in the case of `despawn`, and `false` in the case of `try_despawn`. ## Testing Ran `cargo run -p ci` on macOS, it seemed fine.
robtfm
pushed a commit
to robtfm/bevy
that referenced
this pull request
Oct 4, 2024
# Objective Fixes bevyengine#14511. `despawn` allows you to remove entities from the world. However, if the entity does not exist, it emits a warning. This may not be intended behavior for many users who have use cases where they need to call `despawn` regardless of if the entity actually exists (see the issue), or don't care in general if the entity already doesn't exist. (Also trying to gauge interest on if this feature makes sense, I'd personally love to have it, but I could see arguments that this might be a footgun. Just trying to help here 😄 If there's no contention I could also implement this for `despawn_recursive` and `despawn_descendants` in the same PR) ## Solution Add `try_despawn`, `try_despawn_recursive` and `try_despawn_descendants`. Modify `World::despawn_with_caller` to also take in a `warn` boolean argument, which is then considered when logging the warning. Set `log_warning` to `true` in the case of `despawn`, and `false` in the case of `try_despawn`. ## Testing Ran `cargo run -p ci` on macOS, it seemed fine.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Fixes #14511.
despawnallows you to remove entities from the world. However, if the entity does not exist, it emits a warning. This may not be intended behavior for many users who have use cases where they need to calldespawnregardless of if the entity actually exists (see the issue), or don't care in general if the entity already doesn't exist.(Also trying to gauge interest on if this feature makes sense, I'd personally love to have it, but I could see arguments that this might be a footgun. Just trying to help here 😄 If there's no contention I could also implement this for
despawn_recursiveanddespawn_descendantsin the same PR)Solution
Add
try_despawn,try_despawn_recursiveandtry_despawn_descendants.Modify
World::despawn_with_callerto also take in awarnboolean argument, which is then considered when logging the warning. Setlog_warningtotruein the case ofdespawn, andfalsein the case oftry_despawn.Testing
Ran
cargo run -p cion macOS, it seemed fine.