From 5e6b141c132df1d429038baa98b9b9f097a60112 Mon Sep 17 00:00:00 2001 From: SpecificProtagonist Date: Thu, 26 Sep 2024 15:31:11 +0200 Subject: [PATCH] List components for QueryEntityError::QueryDoesNotMatch (#15435) # 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 Co-authored-by: poopy --- crates/bevy_ecs/src/query/error.rs | 109 ++++++++++++++++++-- crates/bevy_ecs/src/query/state.rs | 26 ++--- crates/bevy_ecs/src/world/mod.rs | 2 +- crates/bevy_render/src/render_phase/draw.rs | 3 +- crates/bevy_transform/src/helper.rs | 2 +- 5 files changed, 119 insertions(+), 23 deletions(-) diff --git a/crates/bevy_ecs/src/query/error.rs b/crates/bevy_ecs/src/query/error.rs index 1f1f33f2c4d56..bdf503b32cf41 100644 --- a/crates/bevy_ecs/src/query/error.rs +++ b/crates/bevy_ecs/src/query/error.rs @@ -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)] @@ -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)"); + } +} diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index ab9947f80be30..07c0f8bd47239 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -744,7 +744,7 @@ impl QueryState { &mut self, world: &'w World, entity: Entity, - ) -> Result, QueryEntityError> { + ) -> Result, QueryEntityError<'w>> { self.update_archetypes(world); // SAFETY: query is read only unsafe { @@ -794,7 +794,7 @@ impl QueryState { &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: @@ -818,7 +818,7 @@ impl QueryState { &mut self, world: &'w mut World, entity: Entity, - ) -> Result, QueryEntityError> { + ) -> Result, QueryEntityError<'w>> { self.update_archetypes(world); let change_tick = world.change_tick(); let last_change_tick = world.last_change_tick(); @@ -868,7 +868,7 @@ impl QueryState { /// 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] @@ -876,7 +876,7 @@ impl QueryState { &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(); @@ -911,7 +911,7 @@ impl QueryState { &self, world: &'w World, entity: Entity, - ) -> Result, QueryEntityError> { + ) -> Result, QueryEntityError<'w>> { self.validate_world(world.id()); // SAFETY: query is read only and world is validated unsafe { @@ -937,7 +937,7 @@ impl QueryState { &mut self, world: UnsafeWorldCell<'w>, entity: Entity, - ) -> Result, QueryEntityError> { + ) -> Result, QueryEntityError<'w>> { self.update_archetypes_unsafe_world_cell(world); self.get_unchecked_manual(world, entity, world.last_change_tick(), world.change_tick()) } @@ -960,7 +960,7 @@ impl QueryState { entity: Entity, last_run: Tick, this_run: Tick, - ) -> Result, QueryEntityError> { + ) -> Result, QueryEntityError<'w>> { let location = world .entities() .get(entity) @@ -969,7 +969,7 @@ impl QueryState { .matched_archetypes .contains(location.archetype_id.index()) { - return Err(QueryEntityError::QueryDoesNotMatch(entity)); + return Err(QueryEntityError::QueryDoesNotMatch(entity, world)); } let archetype = world .archetypes() @@ -989,7 +989,7 @@ impl QueryState { 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)) } } @@ -1008,7 +1008,7 @@ impl QueryState { 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) { @@ -1042,7 +1042,7 @@ impl QueryState { 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 { @@ -1442,7 +1442,7 @@ impl QueryState { /// # 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])); /// ``` /// diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 00be7aee4d68f..568a70c0dc7e7 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -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] { diff --git a/crates/bevy_render/src/render_phase/draw.rs b/crates/bevy_render/src/render_phase/draw.rs index 0880ae797efd3..41a5f49897ec9 100644 --- a/crates/bevy_render/src/render_phase/draw.rs +++ b/crates/bevy_render/src/render_phase/draw.rs @@ -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) } }, diff --git a/crates/bevy_transform/src/helper.rs b/crates/bevy_transform/src/helper.rs index 091e39e189bac..4a77f3fa72604 100644 --- a/crates/bevy_transform/src/helper.rs +++ b/crates/bevy_transform/src/helper.rs @@ -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)