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] - Add get_entity to Commands #5854

Closed
wants to merge 8 commits into from

Conversation

james-j-obrien
Copy link
Contributor

@james-j-obrien james-j-obrien commented Sep 1, 2022

Objective

Solution

  • As described in the issue, added a get_entity method on Commands that returns an Option<EntityCommands>

Changelog

  • Added the new method with a simple doc test
  • I have re-used get_entity in entity, similarly to how get_single is used in single while additionally preserving the error message
  • Add #[inline] to both functions

Entities that have commands queued to despawn system will still return commands when get_entity is called but that is representative of the fact that the entity is still around until those commands are flushed.

A potential contains_entity could also be added in this PR if desired, that would effectively be replacing Entities.contains but may be more discoverable if this is a common use case.

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

I dig it!

crates/bevy_ecs/src/system/commands/mod.rs Show resolved Hide resolved
@mockersf
Copy link
Member

mockersf commented Sep 1, 2022

Could you mention in the doc that it doesn't ensure the command will succeed, as the entity may not exist anymore by the time the command is executed?

@IceSentry IceSentry added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Sep 1, 2022
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

I agree with Francois, but other than that it's a nice and simple addition

@mockersf mockersf 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 1, 2022
Copy link
Contributor

@djeedai djeedai left a comment

Choose a reason for hiding this comment

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

Minor suggestions for clarity.

crates/bevy_ecs/src/system/commands/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/commands/mod.rs Outdated Show resolved Hide resolved
@cart
Copy link
Member

cart commented Sep 1, 2022

bors r+

bors bot pushed a commit that referenced this pull request Sep 1, 2022
# Objective

- Fixes #5850 

## Solution

- As described in the issue, added a `get_entity` method on `Commands` that returns an `Option<EntityCommands>`

## Changelog
- Added the new method with a simple doc test
- I have re-used `get_entity` in `entity`, similarly to how `get_single` is used in `single` while additionally preserving the error message
- Add `#[inline]` to both functions

Entities that have commands queued to despawn system will still return commands when `get_entity` is called but that is representative of the fact that the entity is still around until those commands are flushed.

A potential `contains_entity` could also be added in this PR if desired, that would effectively be replacing Entities.contains but may be more discoverable if this is a common use case.


Co-authored-by: Carter Anderson <[email protected]>
@bors bors bot changed the title Add get_entity to Commands [Merged by Bors] - Add get_entity to Commands Sep 1, 2022
@bors bors bot closed this Sep 1, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

- Fixes bevyengine#5850 

## Solution

- As described in the issue, added a `get_entity` method on `Commands` that returns an `Option<EntityCommands>`

## Changelog
- Added the new method with a simple doc test
- I have re-used `get_entity` in `entity`, similarly to how `get_single` is used in `single` while additionally preserving the error message
- Add `#[inline]` to both functions

Entities that have commands queued to despawn system will still return commands when `get_entity` is called but that is representative of the fact that the entity is still around until those commands are flushed.

A potential `contains_entity` could also be added in this PR if desired, that would effectively be replacing Entities.contains but may be more discoverable if this is a common use case.


Co-authored-by: Carter Anderson <[email protected]>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Fixes bevyengine#5850 

## Solution

- As described in the issue, added a `get_entity` method on `Commands` that returns an `Option<EntityCommands>`

## Changelog
- Added the new method with a simple doc test
- I have re-used `get_entity` in `entity`, similarly to how `get_single` is used in `single` while additionally preserving the error message
- Add `#[inline]` to both functions

Entities that have commands queued to despawn system will still return commands when `get_entity` is called but that is representative of the fact that the entity is still around until those commands are flushed.

A potential `contains_entity` could also be added in this PR if desired, that would effectively be replacing Entities.contains but may be more discoverable if this is a common use case.


Co-authored-by: Carter Anderson <[email protected]>
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.

A way to determine if an entity exists with Commands
5 participants