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_single variant #2793

Closed
wants to merge 11 commits into from

Conversation

IceSentry
Copy link
Contributor

Objective

The vast majority of .single() usage I've seen is immediately followed by a .unwrap(). Since it seems most people use it without handling the error, I think making it easier to just get what you want fast while also having a more verbose alternative when you want to handle the error could help.

Solution

Instead of having a lot of .unwrap() everywhere, this PR introduces a try_single() variant that behaves like the current .single() and make the new .single() panic on error.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Sep 8, 2021
@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 S-Needs-Triage This issue needs to be labelled labels Sep 8, 2021
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I like this change. In my experience, .single either works 100% of the time, or fails unrecoverably because I had a dumb error in my code that I need to fix.

Changing the default ergonomics to reflect this makes a lot of sense, and will help clarify examples.

examples/game/breakout.rs Outdated Show resolved Hide resolved
@NiklasEi NiklasEi added S-Needs-Migration-Guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Sep 9, 2021
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

I've been meaning to get around to this myself, so thanks!

crates/bevy_ecs/src/system/query.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/query.rs Outdated Show resolved Hide resolved
/// # use bevy_ecs::system::{Query, QuerySingleError};
/// # use bevy_ecs::prelude::IntoSystem;
/// struct PlayerScore(i32);
/// fn player_scoring_system(query: Query<&PlayerScore>) {
Copy link
Member

Choose a reason for hiding this comment

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

The player scoring system would probably access the player score mutably (and it should be a resource with the current scheme)

I'm struggling to come up with a realistic example where you need read only access to a singleton entity's components. The only place I've used it is collision detection - detecting when the player collides with an obstacle.

Copy link
Member

Choose a reason for hiding this comment

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

That is, I wonder if we should just remove the read only variants, or at least make the mutable version the default.

Copy link
Member

Choose a reason for hiding this comment

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

I'm struggling to come up with a realistic example where you need read only access to a singleton entity's components.

There's multiple cases where I just needed the player's position - these cases certainly exist.

Copy link
Contributor Author

@IceSentry IceSentry Sep 9, 2021

Choose a reason for hiding this comment

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

I updated the example to use a Query<&Position, With<Player>> to represent a player with a position.

Copy link
Member

Choose a reason for hiding this comment

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

I'm struggling to come up with a realistic example where you need read only access to a singleton entity's components.

I've also run into this when trying to access a camera's projection to perform math using it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was considering using an example using a Camera because that's what I used the most, but I think a player position is simple and common enough to showcase this feature.

@cart
Copy link
Member

cart commented Sep 10, 2021

I think this should snap to the same convention we use for things like world.entity(entity) (accessor-style convention ... panics on failure) and world.get_entity(entity) (fallible get-style convention ... returns an error).

@IceSentry IceSentry changed the title add try_single variant add get_single variant Sep 10, 2021
@cart
Copy link
Member

cart commented Sep 10, 2021

bors r+

bors bot pushed a commit that referenced this pull request Sep 10, 2021
# Objective

The vast majority of `.single()` usage I've seen is immediately followed by a `.unwrap()`. Since it seems most people use it without handling the error, I think making it easier to just get what you want fast while also having a more verbose alternative when you want to handle the error could help.

## Solution

Instead of having a lot of `.unwrap()` everywhere, this PR introduces a `try_single()` variant that behaves like the current `.single()` and make the new `.single()` panic on error.
@bors
Copy link
Contributor

bors bot commented Sep 10, 2021

Build failed:

@cart
Copy link
Member

cart commented Sep 10, 2021

bors r+

bors bot pushed a commit that referenced this pull request Sep 10, 2021
# Objective

The vast majority of `.single()` usage I've seen is immediately followed by a `.unwrap()`. Since it seems most people use it without handling the error, I think making it easier to just get what you want fast while also having a more verbose alternative when you want to handle the error could help.

## Solution

Instead of having a lot of `.unwrap()` everywhere, this PR introduces a `try_single()` variant that behaves like the current `.single()` and make the new `.single()` panic on error.
@bors bors bot changed the title add get_single variant [Merged by Bors] - add get_single variant Sep 10, 2021
@bors bors bot closed this Sep 10, 2021
@IceSentry IceSentry deleted the try_single branch September 14, 2021 23:04
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.

7 participants