-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Improve documentation relating to Frustum
and HalfSpace
#9136
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
Changes from 7 commits
c9bcff3
d3d39f4
0377c42
3c948cb
a291534
10751c1
9714243
dee7a95
19c3c8c
936a912
4d506ca
aeaef2d
1a3e4f0
c425511
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -4,6 +4,18 @@ use bevy_reflect::Reflect; | |||
use bevy_utils::HashMap; | ||||
|
||||
/// An axis-aligned bounding box. | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This probably needs to mention There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It isn't applied only to 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 local axis. It is used as a component on an entity to determine | ||||
/// if it should be rendered by a [`Camera`](crate::camera::Camera) entity if it | ||||
/// intersects with its [`Frustum`], in a process called frustum culling. | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. saying it's used for frustum culling is very limiting to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it really? By third party crates or Bevy itself? Because it's not inserted to entities with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In Bevy it's used just for frustum culling, but an aabb is a general type that have many uses. It's a component that users can use however they want, we shouldn't restrict it to frustum culling in its doc There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, but since it's linked to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed partially by 936a912 |
||||
/// | ||||
/// It will be added automatically to entities that could be subject to frustum | ||||
/// culling, and don't have the [`NoFrustumCulling`](crate::view::visibility::NoFrustumCulling) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unless this is describe in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #4971. Also in situations related to global lighting/reflection There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). | ||||
Selene-Amanita marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
/// | ||||
/// It won't be updated automatically if the space occupied by the entity changes. | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 { | ||||
|
@@ -81,11 +93,28 @@ 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. | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the "origin of 3D space along the normal" here? The global zero point There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "along the normal" applies to the distance, not the origin of 3D space. I can move it earlier in the sentence. It was kinda worded like this before just the "bisecting plane" and "origin" were reversed and there was no "3D space" Edit: Changed it to: /// 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.
But I don't think it's clearer. |
||||
/// | ||||
/// 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. | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think these are necessary. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 `normal.dot(p) + distance >= 0.` is satisfied. | ||||
/// | ||||
/// 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`]. | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. among other things There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed to Is that okay? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||||
|
@@ -94,7 +123,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 | ||||
Selene-Amanita marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
/// 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 { | ||||
|
@@ -109,8 +138,10 @@ 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. | ||||
/// This distance helps determine the position of a point `p` relative to the | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 This sentence was written for Edit: I just removed it. |
||||
/// bisecting plane, as per the equation `normal.dot(p) + distance >= 0.`. | ||||
#[inline] | ||||
pub fn d(&self) -> f32 { | ||||
self.normal_d.w | ||||
|
@@ -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`]. | ||||
Selene-Amanita marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
/// Usually a six sided polyhedron with sides inside the bisecting planes of the half-spaces. | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
It kinda piles up math jargon on top and I don't think it helps much. I prefer "pyramid without the top" personally There was a problem hiding this comment. Choose a reason for hiding this commentThe 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` | ||||
Selene-Amanita marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
/// 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. | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 { | ||||||||||
|
@@ -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, | ||||||||||
} | ||||||||||
|
@@ -253,6 +259,10 @@ impl Plugin for VisibilityPlugin { | |||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
/// System calculating and inserting an [`Aabb`] component to entities with a | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: |
||||||||||
/// [`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>>, | ||||||||||
|
@@ -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. | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I should still mention I guess the context is in the doc of |
||||||||||
/// | ||||||||||
/// 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)>, | ||||||||||
) { | ||||||||||
|
Uh oh!
There was an error while loading. Please reload this page.