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

add try_insert to entity commands #9844

Merged
merged 6 commits into from
Sep 20, 2023
Merged

Conversation

ethereumdegen
Copy link
Contributor

@ethereumdegen ethereumdegen commented Sep 19, 2023

Objective

  • I spoke with some users in the ECS channel of bevy discord today and they suggested that I implement a fallible form of .insert for components.

  • In my opinion, it would be nice to have a fallible .insert like .try_insert (or to just make insert be fallible!) because it was causing a lot of panics in my game. In my game, I am spawning terrain chunks and despawning them in the Update loop. However, this was causing bevy_xpbd to panic because it was trying to .insert some physics components on my chunks and a race condition meant that its check to see if the entity exists would pass but then the next execution step it would not exist and would do an .insert and then panic. This means that there is no way to avoid a panic with conditionals.

Luckily, bevy_xpbd does not care about inserting these components if the entity is being deleted and so if there were a .try_insert, like this PR provides it could use that instead in order to NOT panic.

( My interim solution for my own game has been to run the entity despawn events in the Last schedule but really this is just a hack and I should not be expected to manage the scheduling of despawns like this - it should just be easy and simple. IF it just so happened that bevy_xpbd ran .inserts in the Last schedule also, this would be an untenable soln overall )

Solution

  • Describe the solution used to achieve the objective above.

Add a new command named TryInsert (entitycommands.try_insert) which functions exactly like .insert except if the entity does not exist it will not panic. Instead, it will log to info. This way, crates that are attaching components in ways which they do not mind that the entity no longer exists can just use try_insert instead of insert.


Changelog

Additional Thoughts

In my opinion, NOT panicing should really be the default and having an .insert that does panic should be the odd edgecase but removing the panic! from .insert seems a bit above my paygrade -- although i would love to see it. My other thought is it would be good for .insert to return an Option AND not panic but it seems it uses an event bus right now so that seems to be impossible w the current architecture.

@github-actions
Copy link
Contributor

Welcome, new contributor!

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

@ethereumdegen ethereumdegen changed the title add try insert add try_insert to entity commands Sep 19, 2023
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Sep 19, 2023
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

Looks good, I think we should merge this. I'd like to see minor adjustments though.

///
/// fn add_combat_stats_system(mut commands: Commands, player: Res<PlayerEntity>) {
///
/// commands.entity(player.entity)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// commands.entity(player.entity)
/// commands.entity(player.entity)

/// health: Health(100),
/// strength: Strength(40),
/// });
///
Copy link
Contributor

@nicopap nicopap Sep 19, 2023

Choose a reason for hiding this comment

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

For the lulz (and illustration) I'd add this line

Suggested change
///
/// commands.entity(player.entity).despawn();

Edit: Actually this should be moved before the insert.

if let Some(mut entity) = world.get_entity_mut(self.entity) {
entity.insert(self.bundle);
} else {
info!("error[B0003]: Could not insert a bundle (of type `{}`) for entity {:?} because it doesn't exist in this World.", std::any::type_name::<T>(), self.entity);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
info!("error[B0003]: Could not insert a bundle (of type `{}`) for entity {:?} because it doesn't exist in this World.", std::any::type_name::<T>(), self.entity);
debug!("[B0003]: Could not insert a bundle (of type `{}`) for entity {:?} because it doesn't exist in this World.", std::any::type_name::<T>(), self.entity);

if let Some(mut entity) = world.get_entity_mut(self.entity) {
entity.insert(self.bundle);
} else {
info!("error[B0003]: Could not insert a bundle (of type `{}`) for entity {:?} because it doesn't exist in this World.", std::any::type_name::<T>(), self.entity);
Copy link
Member

Choose a reason for hiding this comment

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

This log seems like unnecessary noise. If someone calls try_insert that means they don't care about this "error" case, so we can safely ignore it.

///
/// # Note
///
/// The command will not panic if the associated entity does not exist.
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
/// The command will not panic if the associated entity does not exist.
/// Unlike [`Self::insert`], this command will not panic if the associated entity does not exist.

@ethereumdegen
Copy link
Contributor Author

okay updated based on fdbk

Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

LGTM

crates/bevy_ecs/src/system/commands/mod.rs Outdated Show resolved Hide resolved
@JoJoJet JoJoJet 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 Sep 20, 2023
@james7132 james7132 added this pull request to the merge queue Sep 20, 2023
Merged via the queue into bevyengine:main with commit 3ee9edf Sep 20, 2023
22 checks passed
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

- I spoke with some users in the ECS channel of bevy discord today and
they suggested that I implement a fallible form of .insert for
components.

- In my opinion, it would be nice to have a fallible .insert like
.try_insert (or to just make insert be fallible!) because it was causing
a lot of panics in my game. In my game, I am spawning terrain chunks and
despawning them in the Update loop. However, this was causing bevy_xpbd
to panic because it was trying to .insert some physics components on my
chunks and a race condition meant that its check to see if the entity
exists would pass but then the next execution step it would not exist
and would do an .insert and then panic. This means that there is no way
to avoid a panic with conditionals.

Luckily, bevy_xpbd does not care about inserting these components if the
entity is being deleted and so if there were a .try_insert, like this PR
provides it could use that instead in order to NOT panic.

( My interim solution for my own game has been to run the entity despawn
events in the Last schedule but really this is just a hack and I should
not be expected to manage the scheduling of despawns like this - it
should just be easy and simple. IF it just so happened that bevy_xpbd
ran .inserts in the Last schedule also, this would be an untenable soln
overall )

## Solution

- Describe the solution used to achieve the objective above.

Add a new command named TryInsert (entitycommands.try_insert) which
functions exactly like .insert except if the entity does not exist it
will not panic. Instead, it will log to info. This way, crates that are
attaching components in ways which they do not mind that the entity no
longer exists can just use try_insert instead of insert.

---

## Changelog

 

## Additional Thoughts

In my opinion, NOT panicing should really be the default and having an
.insert that does panic should be the odd edgecase but removing the
panic! from .insert seems a bit above my paygrade -- although i would
love to see it. My other thought is it would be good for .insert to
return an Option AND not panic but it seems it uses an event bus right
now so that seems to be impossible w the current architecture.
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 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.

5 participants