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.

Sign up for GitHub

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

Jump to bottom

Add panicking versions of get_component and get #9443

Closed
Shatur opened this issue Aug 14, 2023 · 9 comments
Closed

Add panicking versions of get_component and get #9443

Shatur opened this issue Aug 14, 2023 · 9 comments
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide

Comments

@Shatur
Copy link
Contributor

Shatur commented Aug 14, 2023

What problem does this solve or what need does it fill?

It's annoying to call unwrap or expect on get or get_component calls if you know that the entity will contain the component for sure. Happens a lot if you working with hierarchy or reference entities in components.

What solution would you like?

It would be great to have panicking versions for convenience. I think that get_component(_mut) could have component(_mut) alternative. But get need to be renamed to get_entity in order to have panicking entity function. I would like to hear your thoughts about it.
@Shatur Shatur added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Aug 14, 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 and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Aug 14, 2023
@alice-i-cecile
Copy link
Member

Hmm, I can see the value in that, especially for consistency with .resource.

Maybe .get should be renamed regardless, to more clearly communicate that it fetches the selected query data for the entity?

Or we could do get_unchecked, as I think generally users should be checking and matching on this.

@Shatur
Copy link
Contributor Author

Shatur commented Aug 14, 2023

Or we could do get_unchecked, as I think generally users should be checking and matching on this.

This sounds like something that requires unsafe, so I would rename .get, yes.

@alice-i-cecile
Copy link
Member

Perhaps Query::get_data and Query::get_data_mut? I think that using entity in the name gives the wrong impression / mental model, since you don't have access to all of it's components (and it's not somehow fetching an Entity identifier).

@Shatur
Copy link
Contributor Author

Shatur commented Aug 14, 2023

Agree, I like it!
I also thought about components (plural), but it's too similar to component.

@alice-i-cecile alice-i-cecile added D-Trivial Nice and easy! A great choice to get started with Bevy M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Aug 14, 2023
@IAmSegfault
Copy link

Hi, I'm a new contributor and would like to take this on as a first issue. I do have some experience porting the bracket-bevy module in bracket-lib to newer versions of Bevy.

What does this function do in particular? I noticed it calls get_component and it would be helpful to know if it needs to call the new methods as well. Also if it's agreed upon I'll label the methods Query::get_data and Query::get_data_mut.

@Shatur
Copy link
Contributor Author

Shatur commented Aug 15, 2023

What does this function do in particular?

It checks if a query with &mut can return immutable components.

it would be helpful to know if it needs to call the new methods as well.

Yes, you will need to call new methods here.

@Shatur
Copy link
Contributor Author

Shatur commented Aug 27, 2023 •
edited
Loading

@IAmSegfault are you planning to start working on it?
Asking because I considering implementing it myself :)

@IAmSegfault
Copy link

Sure if you want to take over that's fine. I've been busy with work lately and was going to try squeeze in time before the next release.

@Shatur Shatur mentioned this issue Aug 31, 2023
Merged
@Shatur Shatur changed the title Add paniking versions of get_component and get Add panicking versions of get_component and get Sep 3, 2023
github-merge-queue bot pushed a commit that referenced this issue Sep 4, 2023
@Shatur
Add panicking helpers for getting components from Query (#9659)
9309d89 
# Objective

- Currently we don't have panicking alternative for getting components
from `Query` like for resources. Partially addresses #9443.

## Solution

- Add these functions.

---

## Changelog

### Added

- `Query::component` and `Query::component_mut` to get specific
component from query and panic on error.
@alice-i-cecile alice-i-cecile removed the D-Trivial Nice and easy! A great choice to get started with Bevy label Sep 20, 2023
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this issue Jan 9, 2024
@Shatur
Add panicking helpers for getting components from Query (bevyengine…
368e186 
…#9659)

# Objective

- Currently we don't have panicking alternative for getting components
from `Query` like for resources. Partially addresses bevyengine#9443.

## Solution

- Add these functions.

---

## Changelog

### Added

- `Query::component` and `Query::component_mut` to get specific
component from query and panic on error.
@Shatur
Copy link
Contributor Author

Shatur commented Feb 23, 2024

No longer relevant since get_component was removed.

@Shatur Shatur closed this as completed Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@IAmSegfault @alice-i-cecile @Shatur