Improve documentation relating to `Frustum` and `HalfSpace` #9136

Selene-Amanita · 2023-07-12T16:43:30Z

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:

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)
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
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 Add Aabb calculation for Sprite, TextureAtlasSprite and Mesh2d #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?

SpecificProtagonist

Precise documentation is great to see!

Can't answer your questions except to say that I agree that visibility_propagate should be pub (which isn't a breaking change). I actually have a question of my own now: Why are UpdateOrthographicFrusta, UpdatePerspectiveFrusta and UpdateProjectionFrusta separate system sets?

Made some minor suggestions, feel free to ignore.

crates/bevy_render/src/primitives/mod.rs

crates/bevy_render/src/view/visibility/mod.rs

Co-authored-by: SpecificProtagonist <vincentjunge@posteo.net>

Selene-Amanita · 2023-07-16T19:54:21Z

Thanks for reviewing!

Why are UpdateOrthographicFrusta, UpdatePerspectiveFrusta and UpdateProjectionFrusta separate system sets?

I... don't know actually, they're used for ordering but I think this could be made with the systems themselves inside the same SystemSet?

I'll make another PR to make visibility_propagate public later (except if someone else wants to do it)

nicopap · 2023-07-16T20:06:51Z

You'll get my review tomorrow.

crates/bevy_render/src/camera/projection.rs

mockersf · 2023-07-17T17:10:36Z

crates/bevy_render/src/primitives/mod.rs

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


saying it's used for frustum culling is very limiting to Aabb, which is why it's in the primitive module and not in view. Aabb are often used for collisions, for debug, for sorting in space, ...

Is it really? By third party crates or Bevy itself?

Because it's not inserted to entities with NoFrustumCulling, so if it is used for something else, shouldn't NoFrustumCulling be handled differently?

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

I agree, but since it's linked to NoFrustumCulling (and not updated in some cases), I don't see it being used for something else, and I would not advise people to use it (as a component and without tweaking how Bevy inserts, uses and updates it) for something else either for now.

Addressed partially by 936a912

mockersf · 2023-07-17T17:12:13Z

crates/bevy_render/src/primitives/mod.rs

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


among other things

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?

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

Is that okay?

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

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

…d add HalfSpace usage

nicopap

I think the phrasing could be streamlined.

nicopap · 2023-07-16T19:44:42Z

crates/bevy_render/src/primitives/mod.rs

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


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

An axis-aligned bounding box: cube aligned to axis

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.

Adressed by: 936a912

nicopap · 2023-07-16T19:47:36Z

crates/bevy_render/src/primitives/mod.rs

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


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.

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

crates/bevy_render/src/primitives/mod.rs

nicopap · 2023-07-16T19:51:58Z

crates/bevy_render/src/primitives/mod.rs

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


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

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

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

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

nicopap · 2023-07-16T19:57:12Z

crates/bevy_render/src/primitives/mod.rs

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


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

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.

nicopap · 2023-07-17T17:33:21Z

crates/bevy_render/src/primitives/mod.rs

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


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

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.

crates/bevy_render/src/primitives/mod.rs

nicopap · 2023-07-17T17:34:45Z

crates/bevy_render/src/primitives/mod.rs

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


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

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.

nicopap · 2023-07-17T17:46:30Z

crates/bevy_render/src/view/visibility/mod.rs

@@ -253,6 +259,10 @@ impl Plugin for VisibilityPlugin {
    }
 }

+/// System calculating and inserting an [`Aabb`] component to entities with a


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.

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

nicopap · 2023-07-17T17:50:01Z

crates/bevy_render/src/view/visibility/mod.rs

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


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

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.

…ly, and how it is not used only for frustum culling

Selene-Amanita · 2023-07-17T19:59:57Z

@nicopap I addressed most of the stuff in 19c3c8c and put an edit to my comments to your reviews for how I handled it. If you're okay with the changes you can put a 👍 "reaction" and I'll resolve the conversation.

Thanks for your review @nicopap and @mockersf

Selene-Amanita · 2023-07-21T08:05:04Z

Is HalfSpace supposed to be an open or closed set, meaning: should the bisecting plane be in it or out of it? From my tests (and reading of intersects_obb and intersects_sphere), I think it is supposed to be in, hence why I changed the documentation to clarify that and changed the formula to n.p + d >= 0, but that could be because of rounding errors, and I think it doesn't really matter anyway, but I wanted to clarify.

After testing and analysis of the Frustum code (which excludes stuff with n.p + d <= 0, so includes >0), HalfSpace seems to be an open set instead (at least from the perspective of Frustum, there is no actual method in `HalfSpace to say if a vertex is in or not). Fixed in aeaef2d .

crates/bevy_render/src/primitives/mod.rs

alice-i-cecile

Needs a bit more work, but definitely a marked improvement.

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

github-actions · 2023-08-18T16:49:44Z

Example alien_cake_addict failed to run, please try running it locally and check the result.

github-actions · 2023-08-18T16:55:06Z

Example alien_cake_addict failed to run, please try running it locally and check the result.

…ne#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 bevyengine#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 <vincentjunge@posteo.net> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

Enhance documentation relating to and

c9bcff3

Selene-Amanita added C-Docs An addition or correction to our documentation A-Rendering Drawing game state to the screen labels Jul 12, 2023

Selene-Amanita added this to the 0.11.1 milestone Jul 12, 2023

Selene-Amanita changed the title ~~Enhance documentation relating to Frustum and HalfSpace~~ Improve documentation relating to Frustum and HalfSpace Jul 12, 2023

Selene-Amanita added 2 commits July 13, 2023 01:03

Add infos about 2d frustum culling

d3d39f4

rustfmt

0377c42

nicopap self-requested a review July 13, 2023 11:59

Selene-Amanita added 2 commits July 14, 2023 10:16

Enhance Aabb documentation

3c948cb

rustfmt

a291534

SpecificProtagonist approved these changes Jul 16, 2023

View reviewed changes

Selene-Amanita and others added 2 commits July 16, 2023 20:36

Apply suggestions from code review

10751c1

Co-authored-by: SpecificProtagonist <vincentjunge@posteo.net>

Proposed changes from SpecificProtagonist

9714243

mockersf reviewed Jul 17, 2023

View reviewed changes

crates/bevy_render/src/camera/projection.rs Outdated Show resolved Hide resolved

mockersf reviewed Jul 17, 2023

View reviewed changes

Proposed changes by mockersf: remove CameraProjection implementors an…

dee7a95

…d add HalfSpace usage

nicopap reviewed Jul 17, 2023

View reviewed changes

Selene-Amanita added 2 commits July 17, 2023 20:43

Proposed changes by nicopap and mockersf

19c3c8c

Clarify Aabb, can be used not as a component, what is it more general…

936a912

…ly, and how it is not used only for frustum culling

Fix broken sentence

4d506ca

Selene-Amanita added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 18, 2023

HalfSpace is actually an open set

aeaef2d

nicopap self-requested a review July 21, 2023 08:15

alice-i-cecile reviewed Jul 31, 2023

View reviewed changes

crates/bevy_render/src/primitives/mod.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Jul 31, 2023

View reviewed changes

crates/bevy_render/src/primitives/mod.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Jul 31, 2023

View reviewed changes

crates/bevy_render/src/primitives/mod.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Jul 31, 2023

View reviewed changes

crates/bevy_render/src/primitives/mod.rs Outdated Show resolved Hide resolved

alice-i-cecile requested changes Jul 31, 2023

View reviewed changes

Selene-Amanita removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 31, 2023

cart removed this from the 0.11.1 milestone Aug 9, 2023

Selene-Amanita and others added 2 commits August 18, 2023 17:37

Apply suggestions from code review

1a3e4f0

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

Suggested change by SpecificProtagonist

c425511

alice-i-cecile approved these changes Aug 18, 2023

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Aug 18, 2023

alice-i-cecile added this pull request to the merge queue Aug 28, 2023

Merged via the queue into bevyengine:main with commit 44f677a Aug 28, 2023
20 of 21 checks passed

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

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

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

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

Improve documentation relating to Frustum and HalfSpace #9136

Improve documentation relating to Frustum and HalfSpace #9136

Conversation

Selene-Amanita commented Jul 12, 2023 • edited Loading

Objective

Remarks and questions

SpecificProtagonist left a comment

Choose a reason for hiding this comment

Selene-Amanita commented Jul 16, 2023

nicopap commented Jul 16, 2023

Choose a reason for hiding this comment

Selene-Amanita Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Selene-Amanita Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Selene-Amanita Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Selene-Amanita Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Selene-Amanita Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

nicopap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Selene-Amanita Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicopap Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Selene-Amanita Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

nicopap Jul 16, 2023 • edited Loading

Choose a reason for hiding this comment

Selene-Amanita Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Selene-Amanita Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Selene-Amanita Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Selene-Amanita commented Jul 17, 2023 • edited Loading

Selene-Amanita commented Jul 21, 2023

alice-i-cecile left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 18, 2023

github-actions bot commented Aug 18, 2023

Improve documentation relating to `Frustum` and `HalfSpace` #9136

Improve documentation relating to `Frustum` and `HalfSpace` #9136

Selene-Amanita commented Jul 12, 2023 •

edited

Loading

Selene-Amanita Jul 17, 2023 •

edited

Loading

Selene-Amanita Jul 17, 2023 •

edited

Loading

Selene-Amanita Jul 17, 2023 •

edited

Loading

Selene-Amanita Jul 17, 2023 •

edited

Loading

Selene-Amanita Jul 17, 2023 •

edited

Loading

Selene-Amanita Jul 17, 2023 •

edited

Loading

nicopap Jul 17, 2023 •

edited

Loading

Selene-Amanita Jul 17, 2023 •

edited

Loading

nicopap Jul 16, 2023 •

edited

Loading

Selene-Amanita Jul 17, 2023 •

edited

Loading

Selene-Amanita Jul 17, 2023 •

edited

Loading

Selene-Amanita Jul 17, 2023 •

edited

Loading

Selene-Amanita commented Jul 17, 2023 •

edited

Loading