From 44f677a38a6c99b8dcf79426d5b615a1266dde2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9l=C3=A8ne=20Amanita?= <134181069+Selene-Amanita@users.noreply.github.com> Date: Mon, 28 Aug 2023 17:47:25 +0100 Subject: [PATCH] Improve documentation relating to `Frustum` and `HalfSpace` (#9136) # Objective This PR's first aim is to fix a mistake in `HalfSpace`'s documentation. When defining a `Frustum` myself in bevy_basic_portals, I realised that the "distance" of the `HalfSpace` is not, as the current doc defines, the "distance from the origin along the normal", but actually the opposite of that. See the example I gave in this PR. This means one of two things: 1. The documentation about `HalfSpace` is wrong (it is either way because of the `n.p + d > 0` formula given later anyway, which is how it behaves, but in that formula `d` is indeed the opposite of the "distance from the origin along the normal", otherwise it should be `n.p > d`) 2. The distance is supposed to be the "distance from the origin along the normal" but when used in a Frustum it's used as the opposite, and it is a mistake 3. Same as 2, but it is somehow intended Since I think `HalfSpace` is only used for `Frustum`, and it's easier to fix documentation than code, I assumed for this PR we're in case number 1. If we're in case number 3, the documentation of `Frustum` needs to change, and in case number 2, the code needs to be fixed. While I was at it, I also : - Tried to improve the documentation for `Frustum`, `Aabb`, and `VisibilitySystems`, among others, since they're all related to `Frustum`. - Fixed documentation about frustum culling not applying to 2d objects, which is not true since https://github.com/bevyengine/bevy/pull/7885 ## Remarks and questions - What about a `HalfSpace` with an infinite distance, is it allowed and does it represents the whole space? If so it should probably be mentioned. - I referenced the `update_frusta` system in `bevy_render::view::visibility` directly instead of referencing its system set, should I reference the system set instead? It's a bit annoying since it's in 3 sets. - `visibility_propagate` is not public for some reason, I think it probably should be, but for now I only documented its system set, should I make it public? I don't think that would count as a breaking change? - Why is `Aabb` inserted by a system, with `NoFrustumCulling` as an opt-out, instead of having it inserted by default in `PbrBundle` for example and then the system calculating it when it's added? Is it because there is still no way to have an optional component inside a bundle? --------- Co-authored-by: SpecificProtagonist Co-authored-by: Alice Cecile --- crates/bevy_render/src/primitives/mod.rs | 91 ++++++++++++++++--- crates/bevy_render/src/view/visibility/mod.rs | 38 ++++++-- crates/bevy_sprite/src/lib.rs | 7 ++ 3 files changed, 114 insertions(+), 22 deletions(-) diff --git a/crates/bevy_render/src/primitives/mod.rs b/crates/bevy_render/src/primitives/mod.rs index 2cb067ded1..d150fe08e9 100644 --- a/crates/bevy_render/src/primitives/mod.rs +++ b/crates/bevy_render/src/primitives/mod.rs @@ -3,7 +3,32 @@ use bevy_math::{Affine3A, Mat3A, Mat4, Vec3, Vec3A, Vec4, Vec4Swizzles}; use bevy_reflect::Reflect; use bevy_utils::HashMap; -/// An axis-aligned bounding box. +/// An axis-aligned bounding box, defined by: +/// - a center, +/// - the distances from the center to each faces along the axis, +/// the faces are orthogonal to the axis. +/// +/// It is typically used as a component on an entity to represent the local space +/// occupied by this entity, with faces orthogonal to its local axis. +/// +/// This component is notably used during "frustum culling", a process to determine +/// if an entity should be rendered by a [`Camera`] if its bounding box intersects +/// with the camera's [`Frustum`]. +/// +/// It will be added automatically by the systems in [`CalculateBounds`] to entities that: +/// - could be subject to frustum culling, for example with a [`Handle`] +/// or `Sprite` component, +/// - don't have the [`NoFrustumCulling`] component. +/// +/// It won't be updated automatically if the space occupied by the entity changes, +/// for example if the vertex positions of a [`Mesh`] inside a `Handle` are +/// updated. +/// +/// [`Camera`]: crate::camera::Camera +/// [`NoFrustumCulling`]: crate::view::visibility::NoFrustumCulling +/// [`CalculateBounds`]: crate::view::visibility::VisibilitySystems::CalculateBounds +/// [`Mesh`]: crate::mesh::Mesh +/// [`Handle`]: crate::mesh::Mesh #[derive(Component, Clone, Copy, Debug, Default, Reflect)] #[reflect(Component)] pub struct Aabb { @@ -76,11 +101,28 @@ impl Sphere { } } -/// A bisecting plane that partitions 3D space into two regions. +/// A region of 3D space, specifically an open set whose border is a bisecting 2D plane. +/// This bisecting plane partitions 3D space into two infinite regions, +/// the half-space is one of those regions and excludes the bisecting plane. /// -/// Each instance of this type is characterized by the bisecting plane's unit normal and distance from the origin along the normal. -/// Any point `p` is considered to be within the `HalfSpace` when the distance is positive, -/// meaning: if the equation `n.p + d > 0` is satisfied. +/// Each instance of this type is characterized by: +/// - the bisecting plane's unit normal, normalized and pointing "inside" the half-space, +/// - the signed distance along the normal from the bisecting plane to the origin of 3D space. +/// +/// The distance can also be seen as: +/// - the distance along the inverse of the normal from the origin of 3D space to the bisecting plane, +/// - the opposite of the distance along the normal from the origin of 3D space to the bisecting plane. +/// +/// Any point `p` is considered to be within the `HalfSpace` when the length of the projection +/// of p on the normal is greater or equal than the opposite of the distance, +/// meaning: if the equation `normal.dot(p) + distance > 0.` is satisfied. +/// +/// For example, the half-space containing all the points with a z-coordinate lesser +/// or equal than `8.0` would be defined by: `HalfSpace::new(Vec3::NEG_Z.extend(-8.0))`. +/// It includes all the points from the bisecting plane towards `NEG_Z`, and the distance +/// from the plane to the origin is `-8.0` along `NEG_Z`. +/// +/// It is used to define a [`Frustum`], but is also a useful mathematical primitive for rendering tasks such as light computation. #[derive(Clone, Copy, Debug, Default)] pub struct HalfSpace { normal_d: Vec4, @@ -88,8 +130,8 @@ pub struct HalfSpace { impl HalfSpace { /// Constructs a `HalfSpace` from a 4D vector whose first 3 components - /// represent the bisecting plane's unit normal, and the last component signifies - /// the distance from the origin to the plane along the normal. + /// represent the bisecting plane's unit normal, and the last component is + /// the signed distance along the normal from the plane to the origin. /// The constructor ensures the normal vector is normalized and the distance is appropriately scaled. #[inline] pub fn new(normal_d: Vec4) -> Self { @@ -104,23 +146,46 @@ impl HalfSpace { Vec3A::from(self.normal_d) } - /// Returns the distance from the origin to the bisecting plane along the plane's unit normal vector. - /// This distance helps determine the position of a point `p` on the bisecting plane, as per the equation `n.p + d = 0`. + /// Returns the signed distance from the bisecting plane to the origin along + /// the plane's unit normal vector. #[inline] pub fn d(&self) -> f32 { self.normal_d.w } - /// Returns the bisecting plane's unit normal vector and the distance from the origin to the plane. + /// Returns the bisecting plane's unit normal vector and the signed distance + /// from the plane to the origin. #[inline] pub fn normal_d(&self) -> Vec4 { self.normal_d } } -/// A frustum made up of the 6 defining half spaces. -/// Half spaces are ordered left, right, top, bottom, near, far. -/// The normal vectors of the half spaces point towards the interior of the frustum. +/// A region of 3D space defined by the intersection of 6 [`HalfSpace`]s. +/// +/// Frustums are typically an apex-truncated square pyramid (a pyramid without the top) or a cuboid. +/// +/// Half spaces are ordered left, right, top, bottom, near, far. The normal vectors +/// of the half-spaces point towards the interior of the frustum. +/// +/// A frustum component is used on an entity with a [`Camera`] component to +/// determine which entities will be considered for rendering by this camera. +/// All entities with an [`Aabb`] component that are not contained by (or crossing +/// the boundary of) the frustum will not be rendered, and not be used in rendering computations. +/// +/// This process is called frustum culling, and entities can opt out of it using +/// the [`NoFrustumCulling`] component. +/// +/// The frustum component is typically added from a bundle, either the `Camera2dBundle` +/// or the `Camera3dBundle`. +/// It is usually updated automatically by [`update_frusta`] from the +/// [`CameraProjection`] component and [`GlobalTransform`] of the camera entity. +/// +/// [`Camera`]: crate::camera::Camera +/// [`NoFrustumCulling`]: crate::view::visibility::NoFrustumCulling +/// [`update_frusta`]: crate::view::visibility::update_frusta +/// [`CameraProjection`]: crate::camera::CameraProjection +/// [`GlobalTransform`]: bevy_transform::components::GlobalTransform #[derive(Component, Clone, Copy, Debug, Default, Reflect)] #[reflect(Component)] pub struct Frustum { diff --git a/crates/bevy_render/src/view/visibility/mod.rs b/crates/bevy_render/src/view/visibility/mod.rs index 9a58b4ada7..538b6a03ee 100644 --- a/crates/bevy_render/src/view/visibility/mod.rs +++ b/crates/bevy_render/src/view/visibility/mod.rs @@ -153,7 +153,13 @@ pub struct VisibilityBundle { pub computed: ComputedVisibility, } -/// Use this component to opt-out of built-in frustum culling for Mesh entities +/// Use this component to opt-out of built-in frustum culling for entities, see +/// [`Frustum`]. +/// +/// It can be used for example: +/// - when a [`Mesh`] is updated but its [`Aabb`] is not, which might happen with animations, +/// - when using some light effects, like wanting a [`Mesh`] out of the [`Frustum`] +/// to appear in the reflection of a [`Mesh`] within. #[derive(Component, Default, Reflect)] #[reflect(Component, Default)] pub struct NoFrustumCulling; @@ -161,15 +167,12 @@ pub struct NoFrustumCulling; /// Collection of entities visible from the current view. /// /// This component contains all entities which are visible from the currently -/// rendered view. The collection is updated automatically by the [`check_visibility()`] -/// system, and renderers can use it to optimize rendering of a particular view, to +/// rendered view. The collection is updated automatically by the [`VisibilitySystems::CheckVisibility`] +/// system set, and renderers can use it to optimize rendering of a particular view, to /// prevent drawing items not visible from that view. /// /// This component is intended to be attached to the same entity as the [`Camera`] and /// the [`Frustum`] defining the view. -/// -/// Currently this component is ignored by the sprite renderer, so sprite rendering -/// is not optimized per view. #[derive(Clone, Component, Default, Debug, Reflect)] #[reflect(Component)] pub struct VisibleEntities { @@ -193,13 +196,21 @@ impl VisibleEntities { #[derive(Debug, Hash, PartialEq, Eq, Clone, SystemSet)] pub enum VisibilitySystems { + /// Label for the [`calculate_bounds`] and `calculate_bounds_2d` systems, + /// calculating and inserting an [`Aabb`] to relevant entities. CalculateBounds, + /// Label for the [`apply_deferred`] call after [`VisibilitySystems::CalculateBounds`] CalculateBoundsFlush, + /// Label for the [`update_frusta`] system. UpdateOrthographicFrusta, + /// Label for the [`update_frusta`] system. UpdatePerspectiveFrusta, + /// Label for the [`update_frusta`] system. UpdateProjectionFrusta, + /// Label for the system propagating the [`ComputedVisibility`] in a + /// [`hierarchy`](bevy_hierarchy). VisibilityPropagate, - /// Label for the [`check_visibility()`] system updating each frame the [`ComputedVisibility`] + /// Label for the [`check_visibility`] system updating [`ComputedVisibility`] /// of each entity and the [`VisibleEntities`] of each view. CheckVisibility, } @@ -253,6 +264,10 @@ impl Plugin for VisibilityPlugin { } } +/// Computes and adds an [`Aabb`] component to entities with a +/// [`Handle`](Mesh) component and without a [`NoFrustumCulling`] component. +/// +/// This system is used in system set [`VisibilitySystems::CalculateBounds`]. pub fn calculate_bounds( mut commands: Commands, meshes: Res>, @@ -267,6 +282,11 @@ pub fn calculate_bounds( } } +/// Updates [`Frustum`]. +/// +/// This system is used in system sets [`VisibilitySystems::UpdateProjectionFrusta`], +/// [`VisibilitySystems::UpdatePerspectiveFrusta`], and +/// [`VisibilitySystems::UpdateOrthographicFrusta`]. pub fn update_frusta( mut views: Query< (&GlobalTransform, &T, &mut Frustum), @@ -345,9 +365,9 @@ fn propagate_recursive( Ok(()) } -/// System updating the visibility of entities each frame. +/// Updates the visibility of entities each frame. /// -/// The system is part of the [`VisibilitySystems::CheckVisibility`] set. Each frame, it updates the +/// This system is part of the [`VisibilitySystems::CheckVisibility`] set. Each frame, it updates the /// [`ComputedVisibility`] of all entities, and for each view also compute the [`VisibleEntities`] /// for that view. pub fn check_visibility( diff --git a/crates/bevy_sprite/src/lib.rs b/crates/bevy_sprite/src/lib.rs index 72038b0427..b337df92d9 100644 --- a/crates/bevy_sprite/src/lib.rs +++ b/crates/bevy_sprite/src/lib.rs @@ -108,6 +108,13 @@ impl Plugin for SpritePlugin { } } +/// System calculating and inserting an [`Aabb`] component to entities with either: +/// - a `Mesh2dHandle` component, +/// - a `Sprite` and `Handle` components, +/// - a `TextureAtlasSprite` and `Handle` components, +/// and without a [`NoFrustumCulling`] component. +/// +/// Used in system set [`VisibilitySystems::CalculateBounds`]. pub fn calculate_bounds_2d( mut commands: Commands, meshes: Res>,