Skip to content

Commit

Permalink
Add warning when a hierarchy component is missing
Browse files Browse the repository at this point in the history
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.

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 propagations tolerant to missing
  components
  • Loading branch information
nicopap committed Aug 6, 2022
1 parent 1152111 commit a95bc6a
Show file tree
Hide file tree
Showing 6 changed files with 161 additions and 3 deletions.
1 change: 1 addition & 0 deletions crates/bevy_hierarchy/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ trace = []
# bevy
bevy_app = { path = "../bevy_app", 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" }

Expand Down
30 changes: 28 additions & 2 deletions crates/bevy_hierarchy/src/hierarchy.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
use crate::components::{Children, Parent};
use bevy_ecs::{
component::Component,
entity::Entity,
system::{Command, EntityCommands},
query::{Added, Changed, Or, With},
system::{Command, EntityCommands, Local, Query},
world::{EntityMut, World},
};
use bevy_utils::tracing::debug;
use bevy_log::warn;
use bevy_utils::{get_short_name, tracing::debug};

/// Despawns the given entity and all its children recursively
#[derive(Debug)]
Expand All @@ -20,6 +23,29 @@ pub struct DespawnChildrenRecursive {
pub entity: Entity,
}

/// System to print a warning if at any time we detect an inconsistent hierarchy.
pub fn hierarchy_healthcheck_system<T: Component>(
parent_query: Query<&Parent, (With<T>, Or<(Changed<Parent>, Added<T>)>)>,
component_query: Query<(), With<T>>,
mut already_diagnosed: Local<bool>,
) {
if *already_diagnosed {
return;
}
for parent_of_component in &parent_query {
let parent = parent_of_component.get();
if !component_query.contains(parent) {
*already_diagnosed = true;
warn!(
"warning[B0004]: An entity 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>()),
);
return;
}
}
}

/// Function for despawning an entity and all its children
pub fn despawn_with_children_recursive(world: &mut World, entity: Entity) {
// first, make the entity's own parent forget about it
Expand Down
9 changes: 8 additions & 1 deletion crates/bevy_render/src/view/visibility/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ pub use render_layers::*;
use bevy_app::{CoreStage, Plugin};
use bevy_asset::{Assets, Handle};
use bevy_ecs::prelude::*;
use bevy_hierarchy::{Children, Parent};
use bevy_hierarchy::{hierarchy_healthcheck_system, Children, Parent};
use bevy_reflect::std_traits::ReflectDefault;
use bevy_reflect::Reflect;
use bevy_transform::components::GlobalTransform;
Expand Down Expand Up @@ -170,6 +170,9 @@ pub enum VisibilitySystems {
/// Label for the [`check_visibility()`] system updating each frame the [`ComputedVisibility`]
/// of each entity and the [`VisibleEntities`] of each view.
CheckVisibility,
/// Check if [`ComputedVisibility`]
/// entities' parents also have a [`ComputedVisibility`] component.
HealthCheck,
}

pub struct VisibilityPlugin;
Expand Down Expand Up @@ -214,6 +217,10 @@ impl Plugin for VisibilityPlugin {
.after(UpdateProjectionFrusta)
.after(VisibilityPropagate)
.after(TransformSystem::TransformPropagate),
)
.add_system_to_stage(
CoreStage::Last,
hierarchy_healthcheck_system::<ComputedVisibility>.label(HealthCheck),
);
}
}
Expand Down
8 changes: 8 additions & 0 deletions crates/bevy_transform/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ pub mod prelude {

use bevy_app::prelude::*;
use bevy_ecs::prelude::*;
use bevy_hierarchy::hierarchy_healthcheck_system;
use prelude::{GlobalTransform, Transform};

/// A [`Bundle`] of the [`Transform`] and [`GlobalTransform`]
Expand Down Expand Up @@ -81,6 +82,9 @@ impl From<Transform> for TransformBundle {
pub enum TransformSystem {
/// Propagates changes in transform to children's [`GlobalTransform`](crate::components::GlobalTransform)
TransformPropagate,
/// Check if [`GlobalTransform`](crate::components::GlobalTransform)
/// entities' parents also have a `GlobalTransform` component.
HealthCheck,
}

/// The base plugin for handling [`Transform`] components
Expand All @@ -99,6 +103,10 @@ impl Plugin for TransformPlugin {
.add_system_to_stage(
CoreStage::PostUpdate,
systems::transform_propagate_system.label(TransformSystem::TransformPropagate),
)
.add_system_to_stage(
CoreStage::Last,
hierarchy_healthcheck_system::<GlobalTransform>.label(TransformSystem::HealthCheck),
);
}
}
113 changes: 113 additions & 0 deletions errors/B0004.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
# 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 are:

- [`ComputedVisibility`]
- [`GlobalTransform`]

For example, 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 follow:


```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 encouter 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
3 changes: 3 additions & 0 deletions errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,6 @@ pub struct B0002;

#[doc = include_str!("../B0003.md")]
pub struct B0003;

#[doc = include_str!("../B0004.md")]
pub struct B0004;

0 comments on commit a95bc6a

Please sign in to comment.