Add warning when a hierarchy component is missing (#5590)

# Objective

A common pitfall since 0.8 is the requirement on `ComputedVisibility`
being present on all ancestors of an entity that itself has
`ComputedVisibility`, without which, the entity becomes invisible.

I myself hit the issue and got very confused, and saw a few people hit
it as well, so it makes sense to provide a hint of what to do when such
a situation is encountered.

- Fixes #5849
- Closes #5616
- Closes #2277 
- Closes #5081

## Solution

We now check that all entities with both a `Parent` and a
`ComputedVisibility` component have parents that themselves have a
`ComputedVisibility` component.

Note that the warning is only printed once.

We also add a similar warning to `GlobalTransform`.

This only emits a warning. Because sometimes it could be an intended
behavior.

Alternatives:
- Do nothing and keep repeating to newcomers how to avoid recurring
  pitfalls
- Make the transform and visibility propagation tolerant to missing
  components (#5616)
- Probably archetype invariants, though the current draft would not
  allow detecting that kind of errors

---

## Changelog

- Add a warning when encountering dubious component hierarchy structure


Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com>
This commit is contained in:
Nicola Papale 2022-09-19 16:12:11 +00:00
parent ac1aebed5e
commit 6c5403cf47
7 changed files with 225 additions and 2 deletions

View file

@ -14,7 +14,9 @@ trace = []
[dependencies]
# bevy
bevy_app = { path = "../bevy_app", version = "0.9.0-dev" }
bevy_core = { path = "../bevy_core", version = "0.9.0-dev" }
bevy_ecs = { path = "../bevy_ecs", version = "0.9.0-dev", features = ["bevy_reflect"] }
bevy_log = { path = "../bevy_log", version = "0.9.0-dev" }
bevy_reflect = { path = "../bevy_reflect", version = "0.9.0-dev", features = ["bevy"] }
bevy_utils = { path = "../bevy_utils", version = "0.9.0-dev" }

View file

@ -16,10 +16,15 @@ pub use child_builder::*;
mod events;
pub use events::*;
mod valid_parent_check_plugin;
pub use valid_parent_check_plugin::*;
#[doc(hidden)]
pub mod prelude {
#[doc(hidden)]
pub use crate::{child_builder::*, components::*, hierarchy::*, HierarchyPlugin};
pub use crate::{
child_builder::*, components::*, hierarchy::*, HierarchyPlugin, ValidParentCheckPlugin,
};
}
use bevy_app::prelude::*;

View file

@ -0,0 +1,89 @@
use std::marker::PhantomData;
use bevy_app::{App, CoreStage, Plugin};
use bevy_core::Name;
use bevy_ecs::{prelude::*, schedule::ShouldRun};
use bevy_log::warn;
use bevy_utils::{get_short_name, HashSet};
use crate::Parent;
/// When enabled, runs [`check_hierarchy_component_has_valid_parent<T>`].
///
/// This resource is added by [`ValidParentCheckPlugin<T>`].
/// It is enabled on debug builds and disabled in release builds by default,
/// you can update this resource at runtime to change the default behavior.
#[derive(Resource)]
pub struct ReportHierarchyIssue<T> {
/// Whether to run [`check_hierarchy_component_has_valid_parent<T>`].
pub enabled: bool,
_comp: PhantomData<fn(T)>,
}
impl<T> Default for ReportHierarchyIssue<T> {
fn default() -> Self {
Self {
enabled: cfg!(debug_assertions),
_comp: PhantomData,
}
}
}
/// System to print a warning for each `Entity` with a `T` component
/// which parent hasn't a `T` component.
///
/// Hierarchy propagations are top-down, and limited only to entities
/// with a specific component (such as `ComputedVisibility` and `GlobalTransform`).
/// This means that entities with one of those component
/// and a parent without the same component is probably a programming error.
/// (See B0004 explanation linked in warning message)
pub fn check_hierarchy_component_has_valid_parent<T: Component>(
parent_query: Query<
(Entity, &Parent, Option<&Name>),
(With<T>, Or<(Changed<Parent>, Added<T>)>),
>,
component_query: Query<(), With<T>>,
mut already_diagnosed: Local<HashSet<Entity>>,
) {
for (entity, parent, name) in &parent_query {
let parent = parent.get();
if !component_query.contains(parent) && !already_diagnosed.contains(&entity) {
already_diagnosed.insert(entity);
warn!(
"warning[B0004]: {name} with the {ty_name} component has a parent without {ty_name}.\n\
This will cause inconsistent behaviors! See https://bevyengine.org/learn/errors/#B0004",
ty_name = get_short_name(std::any::type_name::<T>()),
name = name.map_or("An entity".to_owned(), |s| format!("The {s} entity")),
);
}
}
}
/// Run criteria that only allows running when [`ReportHierarchyIssue<T>`] is enabled.
pub fn on_hierarchy_reports_enabled<T>(report: Res<ReportHierarchyIssue<T>>) -> ShouldRun
where
T: Component,
{
report.enabled.into()
}
/// Print a warning for each `Entity` with a `T` component
/// whose parent doesn't have a `T` component.
///
/// See [`check_hierarchy_component_has_valid_parent`] for details.
pub struct ValidParentCheckPlugin<T: Component>(PhantomData<fn() -> T>);
impl<T: Component> Default for ValidParentCheckPlugin<T> {
fn default() -> Self {
Self(PhantomData)
}
}
impl<T: Component> Plugin for ValidParentCheckPlugin<T> {
fn build(&self, app: &mut App) {
app.init_resource::<ReportHierarchyIssue<T>>()
.add_system_to_stage(
CoreStage::Last,
check_hierarchy_component_has_valid_parent::<T>
.with_run_criteria(on_hierarchy_reports_enabled::<T>),
);
}
}

View file

@ -18,6 +18,7 @@ mod spatial_bundle;
pub mod texture;
pub mod view;
use bevy_hierarchy::ValidParentCheckPlugin;
pub use extract_param::Extract;
pub mod prelude {
@ -34,6 +35,7 @@ pub mod prelude {
}
pub use once_cell;
use prelude::ComputedVisibility;
use crate::{
camera::CameraPlugin,
@ -315,7 +317,8 @@ impl Plugin for RenderPlugin {
});
}
app.add_plugin(WindowRenderPlugin)
app.add_plugin(ValidParentCheckPlugin::<ComputedVisibility>::default())
.add_plugin(WindowRenderPlugin)
.add_plugin(CameraPlugin)
.add_plugin(ViewPlugin)
.add_plugin(MeshPlugin)

View file

@ -14,6 +14,7 @@ pub mod prelude {
use bevy_app::prelude::*;
use bevy_ecs::prelude::*;
use bevy_hierarchy::ValidParentCheckPlugin;
use prelude::{GlobalTransform, Transform};
/// A [`Bundle`] of the [`Transform`] and [`GlobalTransform`]
@ -86,6 +87,7 @@ impl Plugin for TransformPlugin {
fn build(&self, app: &mut App) {
app.register_type::<Transform>()
.register_type::<GlobalTransform>()
.add_plugin(ValidParentCheckPlugin::<GlobalTransform>::default())
// add transform systems to startup so the first update is "correct"
.add_startup_system_to_stage(
StartupStage::PostStartup,

119
errors/B0004.md Normal file
View file

@ -0,0 +1,119 @@
# B0004
A runtime warning.
An [`Entity`] with a hierarchy-inherited component has a [`Parent`]
without the hierarchy-inherited component in question.
The hierarchy-inherited components defined in bevy include:
- [`ComputedVisibility`]
- [`GlobalTransform`]
Third party plugins may also define their own hierarchy components, so
read the warning message carefully and pay attention to the exact type
of the missing component.
To fix this warning, add the missing hierarchy component to all ancestors
of entities with the hierarchy component you wish to use.
The following code will cause a warning to be emitted:
```rust,no_run
use bevy::prelude::*;
// WARNING: this code is buggy
fn setup_cube(
mut commands: Commands,
mut meshes: ResMut<Assets<Mesh>>,
mut materials: ResMut<Assets<StandardMaterial>>,
) {
commands
.spawn_bundle(TransformBundle::default())
.with_children(|parent| {
// cube
parent.spawn_bundle(PbrBundle {
mesh: meshes.add(Mesh::from(shape::Cube { size: 1.0 })),
material: materials.add(Color::rgb(0.8, 0.7, 0.6).into()),
transform: Transform::from_xyz(0.0, 0.5, 0.0),
..default()
});
});
// camera
commands.spawn_bundle(Camera3dBundle {
transform: Transform::from_xyz(-2.0, 2.5, 5.0).looking_at(Vec3::ZERO, Vec3::Y),
..default()
});
}
fn main() {
App::new()
.add_plugins(DefaultPlugins)
.add_startup_system(setup_cube)
.run();
}
```
This code **will not** show a cube on screen.
This is because the entity spawned with `commands.spawn_bundle(…)`
doesn't have a [`ComputedVisibility`] component.
Since the cube is spawned as a child of an entity without the
[`ComputedVisibility`] component, it will not be visible at all.
To fix this, you must use [`SpatialBundle`] over [`TransformBundle`],
as follows:
```rust,no_run
use bevy::prelude::*;
fn setup_cube(
mut commands: Commands,
mut meshes: ResMut<Assets<Mesh>>,
mut materials: ResMut<Assets<StandardMaterial>>,
) {
commands
// We use SpatialBundle instead of TransformBundle, it contains the
// ComputedVisibility component needed to display the cube,
// In addition to the Transform and GlobalTransform components.
.spawn_bundle(SpatialBundle::default())
.with_children(|parent| {
// cube
parent.spawn_bundle(PbrBundle {
mesh: meshes.add(Mesh::from(shape::Cube { size: 1.0 })),
material: materials.add(Color::rgb(0.8, 0.7, 0.6).into()),
transform: Transform::from_xyz(0.0, 0.5, 0.0),
..default()
});
});
// camera
commands.spawn_bundle(Camera3dBundle {
transform: Transform::from_xyz(-2.0, 2.5, 5.0).looking_at(Vec3::ZERO, Vec3::Y),
..default()
});
}
fn main() {
App::new()
.add_plugins(DefaultPlugins)
.add_startup_system(setup_cube)
.run();
}
```
A similar problem occurs when the [`GlobalTransform`] component is missing.
However, when a parent [`GlobalTransform`] is missing,
it will simply prevent all transform propagation,
including when updating the [`Transform`] component of the child.
You will most likely encounter this warning when loading a scene
as a child of a pre-existing [`Entity`] that does not have the proper components.
[`ComputedVisibility`]: https://docs.rs/bevy/*/bevy/render/view/struct.ComputedVisibility.html
[`GlobalTransform`]: https://docs.rs/bevy/*/bevy/transform/components/struct.GlobalTransform.html
[`Transform`]: https://docs.rs/bevy/*/bevy/transform/components/struct.Transform.html
[`Parent`]: https://docs.rs/bevy/*/bevy/hierarchy/struct.Parent.html
[`Entity`]: https://docs.rs/bevy/*/bevy/ecs/entity/struct.Entity.html
[`SpatialBundle`]: https://docs.rs/bevy/*/bevy/render/prelude/struct.SpatialBundle.html
[`TransformBundle`]: https://docs.rs/bevy/*/bevy/transform/struct.TransformBundle.html

View file

@ -6,3 +6,6 @@ pub struct B0002;
#[doc = include_str!("../B0003.md")]
pub struct B0003;
#[doc = include_str!("../B0004.md")]
pub struct B0004;