Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve documentation relating to Frustum and HalfSpace #9136

Merged
merged 14 commits into from
Aug 28, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions crates/bevy_render/src/camera/projection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ impl<T: CameraProjection + Component + GetTypeRegistration> Plugin for CameraPro
/// to recompute the camera projection matrix of the [`Camera`] component attached to
/// the same entity as the component implementing this trait.
///
/// Components implementing this trait include: [`Projection`], [`PerspectiveProjection`]
/// and [`OrthographicProjection`].
///
/// [`Camera`]: crate::camera::Camera
pub trait CameraProjection {
fn get_projection_matrix(&self) -> Mat4;
Expand Down
66 changes: 56 additions & 10 deletions crates/bevy_render/src/primitives/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,18 @@ use bevy_reflect::Reflect;
use bevy_utils::HashMap;

/// An axis-aligned bounding box.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably needs to mention Handle<Mesh>

Copy link
Member Author

@Selene-Amanita Selene-Amanita Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't applied only to Handle<Mesh>, people can find what it applies to if they follow the doc of CalculateBounds, but maybe I can add a "for example" section?

Edit: Mentionned it here:

/// It will be added automatically by the systems in [`CalculateBounds`] to entities that:
/// - could be subject to frustum culling, for example with a [`Handle<Mesh>`]
/// or `Sprite` component,
/// - don't have the [`NoFrustumCulling`] component.

///
/// It represents a box covering the local space occupied by the entity, with faces
/// orthogonal to the axis. It is used as a component on an entity to determine
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence is convoluted because it tries to define the AABB as two different things:

  1. An axis-aligned bounding box: cube aligned to axis
  2. The space occcupied by mesh entities.

I'd suggest to keeping to (1) as definition, and mentioning "this is also used as" in a subsequent sentence (or maybe remove it, you describe (2) already in the rest of this paragraph.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adressed by: 936a912

/// if it should be rendered by a [`Camera`](crate::camera::Camera) entity if it
/// intersects with its [`Frustum`], in a process called frustum culling.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "in a process called frustum culling" Here feels dissociated from the rest of the sentence. You probably need to start a sentence, using it a subject, in order to make the sentence coherent.

Copy link
Member Author

@Selene-Amanita Selene-Amanita Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used as a component on an entity during "frustum culling", a process to determine if the entity should be rendered [...]?

///
/// It will be added automatically to entities that could be subject to frustum
/// culling, and don't have the [`NoFrustumCulling`](crate::view::visibility::NoFrustumCulling)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless this is describe in NoFrustrumCulling it could be nice to specify what a "entity that could be subject to frustum culling" is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't but I'm not sure myself why would you want to opt out of it actually.

Copy link
Contributor

@nicopap nicopap Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #4971. Also in situations related to global lighting/reflection

Copy link
Member Author

@Selene-Amanita Selene-Amanita Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because in animation the Mesh's vertices positions are updated, but the Aabb is not, right?

Lighting/reflection is for when you have a Mesh with a reflective material and you want to see something in it that is out of frustum, right?

I'm trying to write meaningful examples, I have this:

/// 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.```

/// component, using the systems in
/// [`CalculateBounds`](crate::view::visibility::VisibilitySystems::CalculateBounds).
///
/// It won't be updated automatically if the space occupied by the entity changes.
Copy link
Contributor

@nicopap nicopap Jul 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is confusing. Here is a suggestion: "Currently, bevy doesn't update AABB based on changes to the Mesh pointed to by the handle."

Copy link
Member Author

@Selene-Amanita Selene-Amanita Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add that in a "for example" section after (I think it's a good idea), but Aabb don't apply only to Handle<Mesh>.

Edit: added this:

/// 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<Mesh>` are
/// updated.

#[derive(Component, Clone, Copy, Debug, Default, Reflect)]
#[reflect(Component)]
pub struct Aabb {
Expand Down Expand Up @@ -81,11 +93,29 @@ impl Sphere {
}
}

/// A bisecting plane that partitions 3D space into two regions.
/// A region of 3D space, specifically a closed 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 includes the bisecting plane.
///
/// Each instance of this type is characterized by:
/// - the bisecting plane's unit normal, normalized and pointing "inside" the half-space,
/// - the signed distance from the bisecting plane to the origin of 3D space along the normal
///
/// The distance can also be seen as:
/// - the distance from the origin of 3D space to the bisecting plane along the inverse of the normal,
/// - the opposite of the distance from the origin of 3D space to the bisecting plane along the normal.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these are necessary.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, it would make more sense intuitively that the distance would be from the origin to the plane along the normal (so the opposite of what it is) so I wanted to put the emphasis on that.

///
/// 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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like how "normal" is used here. It should be "plane going through the origin with given normal" or something along those lines.

Copy link
Member Author

@Selene-Amanita Selene-Amanita Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plane doesn't (necessarilly, if the distance is not 0.) go through the origin? I don't understand sorry.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm struggling to understand this paragraph as well.

It's not clear what "the length of the projection of p on the normal" is referring to (what type of object is the normal? How is it constructed?) and I don't understand what the "opposite of the distance" means.

/// meaning: if the equation `n.p + d >= 0` is satisfied,
/// where `n` is the normal, `d` the distance, and `n.p` is a dot product.
///
/// 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.
/// 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`].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

among other things

Copy link
Member Author

@Selene-Amanita Selene-Amanita Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it used for something else in Bevy? I didn't want to imply it's the only usage, I just wanted to say it's only used for that in Bevy.

Edit: it is used for lights also, right?

Copy link
Member Author

@Selene-Amanita Selene-Amanita Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to It is used to define a [Frustum], and for light computation. in dee7a95

Is that okay?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as aabb, those are general types and component, doc shouldn't restrict how they are used.

I'm not sure I see the point of documenting that here anyway. being able to go from the frustrum doc to the half space is interesting, not the reverse in my opinion

Copy link
Member Author

@Selene-Amanita Selene-Amanita Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's good to have a concrete example of what it is used for instead of just having it and not knowing how it can be used or fits in Bevy.

I don't really see how my sentence implies it can't be used for something else? I can add "for example", or "among other things" (which feels a bit like lying because in official Bevy it's not), at the end?

Edit: added "for example".

#[derive(Clone, Copy, Debug, Default)]
pub struct HalfSpace {
normal_d: Vec4,
Expand All @@ -94,7 +124,7 @@ 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.
/// the signed distance from the plane to the origin along the normal.
/// The constructor ensures the normal vector is normalized and the distance is appropriately scaled.
#[inline]
pub fn new(normal_d: Vec4) -> Self {
Expand All @@ -109,8 +139,9 @@ 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 distance from the bisecting plane to the origin along the plane's unit normal vector.
/// This distance helps determine the position of a point `p` relative to the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: (I know it is pre-existing text) the "helps determine" is complex, could the sentence be simplified?

Copy link
Member Author

@Selene-Amanita Selene-Amanita Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I would kinda be in favor of removing that sentence completely, it's already explained in the HalfSpace doc and you can do nothing with the distance alone anyway you need the normal for the formula, if this formula should be copied somewhere I would argue it should be in normal_d.

This sentence was written for Plane to know on which side of the plane we are.

Edit: I just removed it.

/// bisecting plane, as per the equation `n.p + d >= 0`.
#[inline]
pub fn d(&self) -> f32 {
self.normal_d.w
Expand All @@ -123,9 +154,24 @@ impl HalfSpace {
}
}

/// 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 frustum defined as the intersection of 6 [`HalfSpace`].
/// Usually a six sided polyhedron with sides inside the bisecting planes of the half-spaces.
Copy link
Contributor

@nicopap nicopap Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Usually a six sided polyhedron with sides inside the bisecting planes of the half-spaces.

It kinda piles up math jargon on top and I don't think it helps much.

I prefer "pyramid without the top" personally

Copy link
Member Author

@Selene-Amanita Selene-Amanita Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be a cuboid for orthogonal projection, or I guess technically it could have less than six sides or extend to infinity.

But I did say "Usually" and a Cuboid is a special case of truncated pyramid I guess.

According to wikipedia, the real name of the truncated pyramid is either "Quadrilateral frustum" (which doesn't help much here) or "apex-truncated square pyramid" which I think works? Are you okay with the last one?

Edit: changed to

/// A frustum defined as the intersection of 6 [`HalfSpace`].
/// 
/// Usually 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`](crate::camera::Camera)
/// component to determine which entities will be considered for rendering
/// by this camera, all entities with an [`Aabb`] component that does not intersect
/// with 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`](crate::view::visibility::NoFrustumCulling) component.
///
/// The frustum component is typically added by adding a bundle, either the `Camera2dBundle`
/// or the `Camera3dBundle`, and is typically updated automatically by
/// [`update_frusta`](crate::view::visibility::update_frusta) from the
/// [`CameraProjection`](crate::camera::CameraProjection) component of the camera entity.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sentence would benefit from being split in two, such as the updating can be explained in isolation from the adding.

Copy link
Member Author

@Selene-Amanita Selene-Amanita Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My French brain struggles to make short sentences 😄 (I agree)

Edit: changed to:

/// 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.

#[derive(Component, Clone, Copy, Debug, Default, Reflect)]
#[reflect(Component)]
pub struct Frustum {
Expand Down
31 changes: 24 additions & 7 deletions crates/bevy_render/src/view/visibility/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,23 +153,21 @@ 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`].
#[derive(Component, Default, Reflect)]
#[reflect(Component, Default)]
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 {
Expand All @@ -193,13 +191,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<OrthographicProjection>`] system.
UpdateOrthographicFrusta,
/// Label for the [`update_frusta<PerspectiveProjection>`] system.
UpdatePerspectiveFrusta,
/// Label for the [`update_frusta<Projection>`] 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,
}
Expand Down Expand Up @@ -253,6 +259,10 @@ impl Plugin for VisibilityPlugin {
}
}

/// System calculating and inserting an [`Aabb`] component to entities with a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// System calculating and inserting an [`Aabb`] component to entities with a
/// Computes and adds an [`Aabb`] component to entities with a

I find sentences in the present participle (-ing) are much more difficult to parse than in the simple present. I'd suggest avoiding (I know) them. Ideally avoid indirection in your sentences, it helps clarity; Much like in programming languages.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the sentence bellow to keep the information that it's a system: Used in system set -> This system is used [...]

/// [`Handle<Mesh>`](Mesh) component and without a [`NoFrustumCulling`] component.
///
/// Used in system set [`VisibilitySystems::CalculateBounds`].
pub fn calculate_bounds(
mut commands: Commands,
meshes: Res<Assets<Mesh>>,
Expand All @@ -267,6 +277,13 @@ pub fn calculate_bounds(
}
}

/// System updating the [`Frustum`] component of an entity, typically a [`Camera`],
/// using its [`GlobalTransform`] and the projection matrix from its [`CameraProjection`]
/// component.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at least remove "of an entity" here. Components are necessarily part of an entity, it's redundant. Also, if split into two paragraphs, it will be much easier to read. Though I'm not sure the second paragraph is necessary, since it doesn't provide much more information than the type signature of the function.

Suggested change
/// System updating the [`Frustum`] component of an entity, typically a [`Camera`],
/// using its [`GlobalTransform`] and the projection matrix from its [`CameraProjection`]
/// component.
/// Updates [`Frustrum`].

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I should still mention Camera, or Camera3dBundle/Camera2dBundle, to add context?

I guess the context is in the doc of Frustum and CameraProjection already.

///
/// Used in system sets [`VisibilitySystems::UpdateProjectionFrusta`],
/// [`VisibilitySystems::UpdatePerspectiveFrusta`], and
/// [`VisibilitySystems::UpdateOrthographicFrusta`].
pub fn update_frusta<T: Component + CameraProjection + Send + Sync + 'static>(
mut views: Query<(&GlobalTransform, &T, &mut Frustum)>,
) {
Expand Down
7 changes: 7 additions & 0 deletions crates/bevy_sprite/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,13 @@ impl Plugin for SpritePlugin {
}
}

/// System calculating and inserting an [`Aabb`] component to entities with either:
/// - a `Mesh2dHandle` component,
/// - a `Sprite` and `Handle<Image>` components,
/// - a `TextureAtlasSprite` and `Handle<TextureAtlas>` components,
/// and without a [`NoFrustumCulling`] component.
///
/// Used in system set [`VisibilitySystems::CalculateBounds`].
pub fn calculate_bounds_2d(
mut commands: Commands,
meshes: Res<Assets<Mesh>>,
Expand Down