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] - Query::get_unique #1263

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 57 additions & 1 deletion crates/bevy_ecs/src/system/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::{
world::{Mut, World},
};
use bevy_tasks::TaskPool;
use std::any::TypeId;
use std::{any::TypeId, fmt::Debug};
use thiserror::Error;

/// Provides scoped access to a World according to a given [WorldQuery] and query filter
Expand Down Expand Up @@ -212,6 +212,42 @@ where
Err(QueryComponentError::MissingWriteAccess)
}
}

pub fn get_unique(&self) -> Result<<Q::Fetch as Fetch<'_>>::Item, UniqueQueryError<'_, Q>>
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to box the error to shrink the return type.

Copy link
Member Author

Choose a reason for hiding this comment

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

How would that help?

Copy link
Contributor

Choose a reason for hiding this comment

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

This could improve performance if the smaller return type fits into two registers by avoiding stack accesses.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I see... Personally I don't think this would change the performance much, but I don't have a way to benchmark that. This seems quite micro-optimization-y, so I'd rather wait on this until we have realistic uses of this function we can benchmark.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we don't need to return Item at all in the Error? The query isn't consumed by this operation, so users that want the other values in the event of an error can always iterate over them. Generally when someone wants a "single" item and there is more than one, there is nothing particularly special about the first item.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should resolve this conversation before merging. My preference is to remove "result: Item" from Error. What do you think @TheRawMeatball ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, while I prefer it as-is I suppose the use case is very slim and I'm willing to compromise to get the ball rolling. I'll update the PR to remove this now.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

where
Q::Fetch: ReadOnlyFetch,
{
let mut query = self.iter();
let first = query.next();
let extra = query.next().is_some();

match (first, extra) {
(Some(r), false) => Ok(r),
(None, _) => Err(UniqueQueryError::NoEntities(std::any::type_name::<Self>())),
(Some(r), _) => Err(UniqueQueryError::MultipleEntities {
result: r,
query_name: std::any::type_name::<Self>(),
}),
}
}

/// See [`Query::get_unique`]
pub fn get_unique_mut(
&mut self,
) -> Result<<Q::Fetch as Fetch<'_>>::Item, UniqueQueryError<'_, Q>> {
let mut query = self.iter_mut();
let first = query.next();
let extra = query.next().is_some();

match (first, extra) {
(Some(r), false) => Ok(r),
(None, _) => Err(UniqueQueryError::NoEntities(std::any::type_name::<Self>())),
(Some(r), _) => Err(UniqueQueryError::MultipleEntities {
result: r,
query_name: std::any::type_name::<Self>(),
}),
}
}
}

/// An error that occurs when retrieving a specific [Entity]'s component from a [Query]
Expand All @@ -226,3 +262,23 @@ pub enum QueryComponentError {
#[error("The requested entity does not exist.")]
NoSuchEntity,
}

#[derive(Error)]
pub enum UniqueQueryError<'a, Q: WorldQuery> {
TheRawMeatball marked this conversation as resolved.
Show resolved Hide resolved
#[error("No entities fit the query {0}")]
NoEntities(&'static str),
Copy link

@Bromeon Bromeon Mar 8, 2021

Choose a reason for hiding this comment

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

Would it make sense to have the same layout for NoEntities as for MultipleEntities,
i.e NoEntities { query_name: &'static str }?

This removes the slightly strange asymmetry and would allow to extend the error information in the future.

#[error("Multiple entities fit the query {query_name}!")]
MultipleEntities {
result: <Q::Fetch as Fetch<'a>>::Item,
query_name: &'static str,
},
}

impl<'a, Q: WorldQuery> Debug for UniqueQueryError<'a, Q> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
Self::NoEntities(_) => f.debug_tuple("NoEntities").finish(),
Self::MultipleEntities { .. } => f.debug_tuple("MultipleEntities").finish(),
}
}
}
5 changes: 2 additions & 3 deletions examples/game/alien_cake_addict.rs
Original file line number Diff line number Diff line change
Expand Up @@ -357,9 +357,8 @@ fn rotate_bonus(game: Res<Game>, time: Res<Time>, mut transforms: Query<&mut Tra

// update the score displayed during the game
fn scoreboard_system(game: Res<Game>, mut query: Query<&mut Text>) {
for mut text in query.iter_mut() {
text.sections[0].value = format!("Sugar Rush: {}", game.score);
}
let mut text = query.get_unique_mut().unwrap();
text.sections[0].value = format!("Sugar Rush: {}", game.score);
}

// restart the game when pressing spacebar
Expand Down
11 changes: 5 additions & 6 deletions examples/game/breakout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ fn paddle_movement_system(
keyboard_input: Res<Input<KeyCode>>,
mut query: Query<(&Paddle, &mut Transform)>,
) {
for (paddle, mut transform) in query.iter_mut() {
if let Ok((paddle, mut transform)) = query.get_unique_mut() {
let mut direction = 0.0;
if keyboard_input.pressed(KeyCode::Left) {
direction -= 1.0;
Expand All @@ -196,15 +196,14 @@ fn ball_movement_system(time: Res<Time>, mut ball_query: Query<(&Ball, &mut Tran
// clamp the timestep to stop the ball from escaping when the game starts
let delta_seconds = f32::min(0.2, time.delta_seconds());

for (ball, mut transform) in ball_query.iter_mut() {
if let Ok((ball, mut transform)) = ball_query.get_unique_mut() {
transform.translation += ball.velocity * delta_seconds;
}
}

fn scoreboard_system(scoreboard: Res<Scoreboard>, mut query: Query<&mut Text>) {
for mut text in query.iter_mut() {
text.sections[1].value = scoreboard.score.to_string();
}
let mut text = query.get_unique_mut().unwrap();
text.sections[0].value = format!("Score: {}", scoreboard.score);
}

fn ball_collision_system(
Expand All @@ -213,7 +212,7 @@ fn ball_collision_system(
mut ball_query: Query<(&mut Ball, &Transform, &Sprite)>,
collider_query: Query<(Entity, &Collider, &Transform, &Sprite)>,
) {
for (mut ball, ball_transform, sprite) in ball_query.iter_mut() {
if let Ok((mut ball, ball_transform, sprite)) = ball_query.get_unique_mut() {
Copy link
Member

Choose a reason for hiding this comment

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

In general however, I think constraining uses like this not ideal. For example, the current version would trivially extend to having multiple fields with the same controls (to allow something like those things where people play multiple pokemon games with one set of inputs)

Although on the other hand, for this simple example this is probably fine.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah this is sort of the crux of my hesitance to adopt get_unique(). It encourages people to design around "single entity" patterns. Maybe thats a good thing, but its a paradigm shift with implications we need to consider.

Copy link
Member

@DJMcNab DJMcNab Jan 24, 2021

Choose a reason for hiding this comment

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

I expect that the cases where we don't mind this pattern would be for rendering Resources, i.e. it's just for getting data from resources onto the screen, which I've called out as a sensible use above. That is, it's a logic error to have multiple scoreboard texts but a single Score resource

But this case is just using that API for the sake of using it, where there's not actually an especially compelling reason to.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I agree. In this case, there might be a future "breakout++" where multiple balls come into play. And I think that for most things even if there is only one item we should encourage query.iter(). Imo get_unique() should be reserved for cases that are truly game breaking.

let ball_size = sprite.size;
let velocity = &mut ball.velocity;

Expand Down