Skip to content

Commit

Permalink
List components for QueryEntityError::QueryDoesNotMatch (bevyengine#1…
Browse files Browse the repository at this point in the history 
…5435)

# Objective

Make it easier to debug why an entity doesn't match a query.

## Solution

List the entities components in `QueryEntityError::QueryDoesNotMatch`'s
message, e.g. `The query does not match the entity 0v1, which has
components foo::Bar, foo::Baz`.
This covers most cases as expected components are typically known and
filtering for change detection is rare when assessing a query by entity
id.

## Testing

Added a test confirming the new message matches the entity's components.

## Migration Guide

- `QueryEntityError` now has a lifetime. Convert it to a custom error if
you need to store it.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: poopy <gonesbird@gmail.com>
  • Loading branch information
@SpecificProtagonist @alice-i-cecile @atornity
3 people authored Sep 26, 2024
1 parent 9a0bfc6 commit 5e6b141
Show file tree
Hide file tree
Showing 5 changed files with 119 additions and 23 deletions.
109 changes: 102 additions & 7 deletions crates/bevy_ecs/src/query/error.rs
View file
Original file line number Diff line number Diff line change
@@ -1,26 +1,93 @@
use thiserror::Error;

use crate::entity::Entity;
use crate::{entity::Entity, world::unsafe_world_cell::UnsafeWorldCell};

/// An error that occurs when retrieving a specific [`Entity`]'s query result from [`Query`](crate::system::Query) or [`QueryState`](crate::query::QueryState).
// TODO: return the type_name as part of this error
#[derive(Debug, PartialEq, Eq, Clone, Copy, Error)]
pub enum QueryEntityError {
#[derive(Clone, Copy)]
pub enum QueryEntityError<'w> {
/// The given [`Entity`]'s components do not match the query.
///
/// Either it does not have a requested component, or it has a component which the query filters out.
#[error("The components of entity {0:?} do not match the query")]
QueryDoesNotMatch(Entity),
QueryDoesNotMatch(Entity, UnsafeWorldCell<'w>),
/// The given [`Entity`] does not exist.
#[error("The entity {0:?} does not exist")]
NoSuchEntity(Entity),
/// The [`Entity`] was requested mutably more than once.
///
/// See [`QueryState::get_many_mut`](crate::query::QueryState::get_many_mut) for an example.
#[error("The entity {0:?} was requested mutably more than once")]
AliasedMutability(Entity),
}

impl<'w> std::error::Error for QueryEntityError<'w> {}

impl<'w> std::fmt::Display for QueryEntityError<'w> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match *self {
Self::QueryDoesNotMatch(entity, world) => {
write!(
f,
"The query does not match the entity {entity}, which has components "
)?;
format_archetype(f, world, entity)
}
Self::NoSuchEntity(entity) => write!(f, "The entity {entity} does not exist"),
Self::AliasedMutability(entity) => write!(
f,
"The entity {entity} was requested mutably more than once"
),
}
}
}

impl<'w> std::fmt::Debug for QueryEntityError<'w> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match *self {
Self::QueryDoesNotMatch(entity, world) => {
write!(f, "QueryDoesNotMatch({entity} with components ")?;
format_archetype(f, world, entity)?;
write!(f, ")")
}
Self::NoSuchEntity(entity) => write!(f, "NoSuchEntity({entity})"),
Self::AliasedMutability(entity) => write!(f, "AliasedMutability({entity})"),
}
}
}

fn format_archetype(
f: &mut std::fmt::Formatter<'_>,
world: UnsafeWorldCell<'_>,
entity: Entity,
) -> std::fmt::Result {
// We know entity is still alive
let entity = world
.get_entity(entity)
.expect("entity does not belong to world");
for (i, component_id) in entity.archetype().components().enumerate() {
if i > 0 {
write!(f, ", ")?;
}
let name = world
.components()
.get_name(component_id)
.expect("entity does not belong to world");
write!(f, "{name}")?;
}
Ok(())
}

impl<'w> PartialEq for QueryEntityError<'w> {
fn eq(&self, other: &Self) -> bool {
match (self, other) {
(Self::QueryDoesNotMatch(e1, _), Self::QueryDoesNotMatch(e2, _)) if e1 == e2 => true,
(Self::NoSuchEntity(e1), Self::NoSuchEntity(e2)) if e1 == e2 => true,
(Self::AliasedMutability(e1), Self::AliasedMutability(e2)) if e1 == e2 => true,
_ => false,
}
}
}

impl<'w> Eq for QueryEntityError<'w> {}

/// An error that occurs when evaluating a [`Query`](crate::system::Query) or [`QueryState`](crate::query::QueryState) as a single expected result via
/// [`get_single`](crate::system::Query::get_single) or [`get_single_mut`](crate::system::Query::get_single_mut).
#[derive(Debug, Error)]
Expand All @@ -32,3 +99,31 @@ pub enum QuerySingleError {
#[error("Multiple entities fit the query {0}")]
MultipleEntities(&'static str),
}

#[cfg(test)]
mod test {
use crate as bevy_ecs;
use crate::prelude::World;
use bevy_ecs_macros::Component;

#[test]
fn query_does_not_match() {
let mut world = World::new();

#[derive(Component)]
struct Present1;
#[derive(Component)]
struct Present2;
#[derive(Component, Debug)]
struct NotPresent;

let entity = world.spawn((Present1, Present2)).id();

let err = world
.query::<&NotPresent>()
.get(&world, entity)
.unwrap_err();

assert_eq!(format!("{err:?}"), "QueryDoesNotMatch(0v1 with components bevy_ecs::query::error::test::query_does_not_match::Present1, bevy_ecs::query::error::test::query_does_not_match::Present2)");
}
}
26 changes: 13 additions & 13 deletions crates/bevy_ecs/src/query/state.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -744,7 +744,7 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
&mut self,
world: &'w World,
entity: Entity,
) -> Result<ROQueryItem<'w, D>, QueryEntityError> {
) -> Result<ROQueryItem<'w, D>, QueryEntityError<'w>> {
self.update_archetypes(world);
// SAFETY: query is read only
unsafe {
Expand Down Expand Up @@ -794,7 +794,7 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
&mut self,
world: &'w World,
entities: [Entity; N],
) -> Result<[ROQueryItem<'w, D>; N], QueryEntityError> {
) -> Result<[ROQueryItem<'w, D>; N], QueryEntityError<'w>> {
self.update_archetypes(world);

// SAFETY:
Expand All @@ -818,7 +818,7 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
&mut self,
world: &'w mut World,
entity: Entity,
) -> Result<D::Item<'w>, QueryEntityError> {
) -> Result<D::Item<'w>, QueryEntityError<'w>> {
self.update_archetypes(world);
let change_tick = world.change_tick();
let last_change_tick = world.last_change_tick();
Expand Down Expand Up @@ -868,15 +868,15 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
/// let invalid_entity = world.spawn_empty().id();
///
/// assert_eq!(query_state.get_many_mut(&mut world, [wrong_entity]).unwrap_err(), QueryEntityError::NoSuchEntity(wrong_entity));
/// assert_eq!(query_state.get_many_mut(&mut world, [invalid_entity]).unwrap_err(), QueryEntityError::QueryDoesNotMatch(invalid_entity));
/// assert_eq!(match query_state.get_many_mut(&mut world, [invalid_entity]).unwrap_err() {QueryEntityError::QueryDoesNotMatch(entity, _) => entity, _ => panic!()}, invalid_entity);
/// assert_eq!(query_state.get_many_mut(&mut world, [entities[0], entities[0]]).unwrap_err(), QueryEntityError::AliasedMutability(entities[0]));
/// ```
#[inline]
pub fn get_many_mut<'w, const N: usize>(
&mut self,
world: &'w mut World,
entities: [Entity; N],
) -> Result<[D::Item<'w>; N], QueryEntityError> {
) -> Result<[D::Item<'w>; N], QueryEntityError<'w>> {
self.update_archetypes(world);

let change_tick = world.change_tick();
Expand Down Expand Up @@ -911,7 +911,7 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
&self,
world: &'w World,
entity: Entity,
) -> Result<ROQueryItem<'w, D>, QueryEntityError> {
) -> Result<ROQueryItem<'w, D>, QueryEntityError<'w>> {
self.validate_world(world.id());
// SAFETY: query is read only and world is validated
unsafe {
Expand All @@ -937,7 +937,7 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
&mut self,
world: UnsafeWorldCell<'w>,
entity: Entity,
) -> Result<D::Item<'w>, QueryEntityError> {
) -> Result<D::Item<'w>, QueryEntityError<'w>> {
self.update_archetypes_unsafe_world_cell(world);
self.get_unchecked_manual(world, entity, world.last_change_tick(), world.change_tick())
}
Expand All @@ -960,7 +960,7 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
entity: Entity,
last_run: Tick,
this_run: Tick,
) -> Result<D::Item<'w>, QueryEntityError> {
) -> Result<D::Item<'w>, QueryEntityError<'w>> {
let location = world
.entities()
.get(entity)
Expand All @@ -969,7 +969,7 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
.matched_archetypes
.contains(location.archetype_id.index())
{
return Err(QueryEntityError::QueryDoesNotMatch(entity));
return Err(QueryEntityError::QueryDoesNotMatch(entity, world));
}
let archetype = world
.archetypes()
Expand All @@ -989,7 +989,7 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
if F::filter_fetch(&mut filter, entity, location.table_row) {
Ok(D::fetch(&mut fetch, entity, location.table_row))
} else {
Err(QueryEntityError::QueryDoesNotMatch(entity))
Err(QueryEntityError::QueryDoesNotMatch(entity, world))
}
}

Expand All @@ -1008,7 +1008,7 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
entities: [Entity; N],
last_run: Tick,
this_run: Tick,
) -> Result<[ROQueryItem<'w, D>; N], QueryEntityError> {
) -> Result<[ROQueryItem<'w, D>; N], QueryEntityError<'w>> {
let mut values = [(); N].map(|_| MaybeUninit::uninit());

for (value, entity) in std::iter::zip(&mut values, entities) {
Expand Down Expand Up @@ -1042,7 +1042,7 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
entities: [Entity; N],
last_run: Tick,
this_run: Tick,
) -> Result<[D::Item<'w>; N], QueryEntityError> {
) -> Result<[D::Item<'w>; N], QueryEntityError<'w>> {
// Verify that all entities are unique
for i in 0..N {
for j in 0..i {
Expand Down Expand Up @@ -1442,7 +1442,7 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
/// # let invalid_entity = world.spawn_empty().id();
///
/// # assert_eq!(query_state.get_many_mut(&mut world, [wrong_entity]).unwrap_err(), QueryEntityError::NoSuchEntity(wrong_entity));
/// # assert_eq!(query_state.get_many_mut(&mut world, [invalid_entity]).unwrap_err(), QueryEntityError::QueryDoesNotMatch(invalid_entity));
/// assert_eq!(match query_state.get_many_mut(&mut world, [invalid_entity]).unwrap_err() {QueryEntityError::QueryDoesNotMatch(entity, _) => entity, _ => panic!()}, invalid_entity);
/// # assert_eq!(query_state.get_many_mut(&mut world, [entities[0], entities[0]]).unwrap_err(), QueryEntityError::AliasedMutability(entities[0]));
/// ```
///
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/world/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -723,7 +723,7 @@ impl World {
/// # Errors
///
/// If any entities are duplicated.
fn verify_unique_entities(entities: &[Entity]) -> Result<(), QueryEntityError> {
fn verify_unique_entities(entities: &[Entity]) -> Result<(), QueryEntityError<'static>> {
for i in 0..entities.len() {
for j in 0..i {
if entities[i] == entities[j] {
Expand Down
3 changes: 2 additions & 1 deletion crates/bevy_render/src/render_phase/draw.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,8 @@ where
Ok(view) => view,
Err(err) => match err {
QueryEntityError::NoSuchEntity(_) => return Err(DrawError::ViewEntityNotFound),
QueryEntityError::QueryDoesNotMatch(_) | QueryEntityError::AliasedMutability(_) => {
QueryEntityError::QueryDoesNotMatch(_, _)
| QueryEntityError::AliasedMutability(_) => {
return Err(DrawError::InvalidViewQuery)
}
},
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_transform/src/helper.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ impl<'w, 's> TransformHelper<'w, 's> {
fn map_error(err: QueryEntityError, ancestor: bool) -> ComputeGlobalTransformError {
use ComputeGlobalTransformError::*;
match err {
QueryEntityError::QueryDoesNotMatch(entity) => MissingTransform(entity),
QueryEntityError::QueryDoesNotMatch(entity, _) => MissingTransform(entity),
QueryEntityError::NoSuchEntity(entity) => {
if ancestor {
MalformedHierarchy(entity)
Expand Down

0 comments on commit 5e6b141

Please sign in to comment.