Move Frustum, HalfSpace from bevy_render to bevy_math by torsteingrindvik · Pull Request #13882 · bevyengine/bevy

torsteingrindvik · 2024-06-16T19:07:15Z

Also move necessary baggage.

Objective

Fixes #13878

Solution

Move Frustum and HalfSpace from bevy_render to bevy_math
Move the Sphere and Aabb structs from bevy_render to a legacy module in bevy_math, to be refactored later

Changelog

Move Frustum and HalfSpace from bevy_render to bevy_math

Migration Guide

Frustum and HalfSpace must now be imported from bevy_math::primitives::{Frustum, HalfSpace} instead of from bevy_render.

torsteingrindvik · 2024-06-16T19:09:45Z

When moving baggage into the legacy module (as suggested in #13878 (comment)) I had to also make docs for those, but since it's a legacy module I just mocked the docs. Let me know what should be done there (if not that).

Also the bevy_render/src/lib.rs registering of types seem kinda weird 🤔

alice-i-cecile · 2024-06-16T20:17:47Z

crates/bevy_math/src/primitives/mod.rs

+pub use frustum::Frustum;
+mod half_space;
+pub use half_space::HalfSpace;
+/// Clean me up!


This needs real docs explaining why this exists, and linking to an issue made to track their clean-up.

alice-i-cecile

Good, just a few notes:

IMO we should remove the legacy notes docs and simply blanket allow missing docs on the legacy_bevy_render module.
We should also remove Sphere from bevy_render during this PR rather than duplicating it, and note that in the migration guide.
We should make an issue specifically for eliminating bevy_render::primitives and link it in the module docs of both the bevy_math and bevy_render modules.

torsteingrindvik · 2024-06-18T18:26:01Z

We should also remove Sphere from bevy_render during this PR rather than duplicating it, and note that in the migration guide.

Perhaps I misunderstood the goal/scope of point 1. and 2. in the issue, but could you elaborate?
Sphere is deleted from bevy_render here.

Linking to changes seems a bit buggy, but it's deleted from bevy_render/src/primitives/mod.rs.

If you mean it's duplicated because it's now in the legacy module and we should remove that definition altogether then I would need to figure out the implications since the Sphere definition from bevy_render has a center field and the one in bevy_math doesn't.
I was probably missing some context from the conversation on Discord- I'm feeling a bit zapped for energy currently so depending on the scope I might put this up for adaption or close. But if I can get a clear picture it might be ok.

alice-i-cecile · 2024-06-18T19:47:34Z

Sphere is deleted from bevy_render here.

I just misread the diff, sorry for the confusion! I can make the follow-up issue, so the only thing I want from you is the missing_docs change.

…lso move necessary baggage. Signed-off-by: Torstein Grindvik <torstein.grindvik@muybridge.com>

Signed-off-by: Torstein Grindvik <torstein.grindvik@muybridge.com>

…flict for rust lints Signed-off-by: Torstein Grindvik <torstein.grindvik@muybridge.com>

torsteingrindvik · 2024-06-19T18:14:39Z

Sphere is deleted from bevy_render here.

I just misread the diff, sorry for the confusion! I can make the follow-up issue, so the only thing I want from you is the missing_docs change.

Was questioning my sanity for a bit there 😅

I made a placeholder issue and linked to it in the PR: #13931 , will you edit it when needed?

alice-i-cecile

Great: I'll merge this as soon as 0.14 is out the door. Merging refactors like this will make cherrypicking fixes harder, so I'm going to hold off for just a bit.

Jondolf · 2024-06-20T00:25:53Z

If you mean it's duplicated because it's now in the legacy module and we should remove that definition altogether then I would need to figure out the implications since the Sphere definition from bevy_render has a center field and the one in bevy_math doesn't.

Sphere is functionally like a bounding sphere, and its equivalent in bevy_math is the aptly named BoundingSphere. The two types have basically identical representations, except BoundingSphere has a bunch more functionality. Afaik the only missing method is intersects_obb.

As for the Aabb, we can't just remove it from bevy_render or deprecate it without a replacement since it's an actual component used during frustum culling, so moving it to a legacy module doesn't feel very useful. It should either (1) be a wrapper over Aabb3d living in bevy_render, or (2) remain roughly the same, but probably renamed to have a more specific name like CullingAabb or just RenderAabb.

The main "issue" with option (1) is that Aabb has a center + half-extents representation while Aabb3d uses min + max (which is usually better), so we need benchmarks if we intend to change it to use Aabb3d. I guess this might be fine to leave to a follow-up though, as long as it's not merged for 0.14.

Jondolf

Personally, I don't really like adding a legacy rendering module like this to bevy_math especially since it's not gated behind feature flags, and I don't really see the utility. It just feels like extra complexity and is not something bevy_math should have at all, even momentarily imo.

Sphere is a type that we should pretty easily be able to just remove, and Aabb is a type we need to keep around in some form in bevy_render anyway. I'd just add an aabb module (or similar) in bevy_render, or even leave it where it was, and leave refactoring/renaming the type to a follow-up. Similarly for Frustum, if it needs to be a component, it just means that we need a newtype in bevy_render.

Jondolf · 2024-06-20T00:27:29Z

crates/bevy_math/src/primitives/half_space.rs

+#[cfg_attr(feature = "bevy_ecs", derive(Component))]
+#[cfg_attr(
+    feature = "bevy_reflect",
+    derive(Reflect),
+    reflect(Debug, PartialEq, Default)
+)]
+#[cfg_attr(
+    all(feature = "serialize", feature = "bevy_reflect"),
+    reflect(Serialize, Deserialize)
+)]
+#[cfg_attr(
+    all(feature = "bevy_ecs", feature = "bevy_reflect"),
+    reflect(Component)
+)]
+pub struct HalfSpace {


This should not be a component. The old one isn't either.

alice-i-cecile · 2024-06-20T01:00:44Z

@Jondolf how do you think we should tackle this broader initiative of "don't have weird primitives in bevy_render" as a whole? I really value your judgement here, and it's not clear what can / should be done first.

Jondolf · 2024-06-20T14:09:57Z

My response became so long that I just wrote it up into its own issue 😅 See #13945 for the full context.

TLDR concerning this PR: The Aabb and Sphere changes aren't as trivial and shouldn't be done here imo, and I think that adding the legacy module and bevy_ecs dependency in bevy_math just isn't a great idea even if they're temporary, especially given that we don't seem to have a clear plan of how we'd move forward afterwards.

My opinion is that this PR should just do what the PR title says:

Move HalfSpace into bevy_math. (done)
(Optional) Move Frustum into bevy_math. But:
- Rename it to something like ViewFrustum to be more accurate and avoid overlap with e.g. ConicalFrustum.
- Add a wrapper component in bevy_render. Not sure what name fits best since both lights and cameras can have the frustum component. Rendering people might be able to give input here. Either way, I don't really want components in bevy_math, even behind feature flags.
- Add intersects_obb as a method on the wrapper component in bevy_render for now. It'd be nice to have a similar method in bevy_math, but again I don't really like adding a duplicate Aabb or a dependency on bevy_render there. And this is just simpler imo :)

No legacy modules or other unrelated changes, just these two (or just (1)). The Aabb and Sphere refactor can be done in a separate PR imo.

Feel free to disagree, these are just my thoughts on this :)

torsteingrindvik · 2024-08-06T11:05:06Z

My response became so long that I just wrote it up into its own issue 😅 See #13945 for the full context.

TLDR concerning this PR: The Aabb and Sphere changes aren't as trivial and shouldn't be done here imo, and I think that adding the legacy module and bevy_ecs dependency in bevy_math just isn't a great idea even if they're temporary, especially given that we don't seem to have a clear plan of how we'd move forward afterwards.

My opinion is that this PR should just do what the PR title says:
1. Move `HalfSpace` into `bevy_math`. (done)

2. (Optional) Move `Frustum` into `bevy_math`. But:
   
   * Rename it to something like `ViewFrustum` to be more accurate and avoid overlap with e.g. `ConicalFrustum`.
   * Add a wrapper component in `bevy_render`. Not sure what name fits best since both lights and cameras can have the frustum component. Rendering people might be able to give input here. Either way, I don't really want components in `bevy_math`, even behind feature flags.
   * Add `intersects_obb` as a method on the wrapper component in `bevy_render` for now. It'd be nice to have a similar method in `bevy_math`, but again I don't really like adding a duplicate `Aabb` or a dependency on `bevy_render` there. And this is just simpler imo :)
No legacy modules or other unrelated changes, just these two (or just (1)). The Aabb and Sphere refactor can be done in a separate PR imo.

Feel free to disagree, these are just my thoughts on this :)

Thanks for the explanation. This PR was originally just to address a chore when I had some extra time but there are more moving parts involved- so I think it's simplest for me to close this out for now.
The issue you created I think will serve the goals better.

…m `bevy_camera` to `bevy_math` (#22684) # Objective - Does the latter half of #13945 - Does the first two thirds of #13878 - Finish some of what #13882 started ## Solution - Moves `bevy_camera::primitives::HalfSpace` to `bevy_math::primitives::HalfSpace` (first commit) - Moves some of `bevy_camera::primitives::Frustum` to `bevy_math::primitives::ViewFrustum` (second commit) - I basically followed Jondolf’s directions on the two refactorings. aabb, obb, and sphere stuff stays in bevy_camera (for now? If sphere is refactored, it has been stressed to be done as a follow up ) ## Testing I ran the `lighting`, `2d_gizmos`, and `3d_gizmos` example just to make sure the lights… and camera… (and action!) were still working. Everything seems to be fine there compared to main. If there are any other examples I should run to make sure things look good, let me know.

torsteingrindvik requested a review from alice-i-cecile June 16, 2024 19:07

torsteingrindvik changed the title ~~Initial pass move Frustum, HalfSpace from bevy_render to bevy_math. A…~~ Move Frustum, HalfSpace from bevy_render to bevy_math Jun 16, 2024

alice-i-cecile reviewed Jun 16, 2024

View reviewed changes

alice-i-cecile requested changes Jun 16, 2024

View reviewed changes

alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 16, 2024

BD103 approved these changes Jun 18, 2024

View reviewed changes

Torstein Grindvik added 3 commits June 19, 2024 19:58

Initial pass move Frustum, HalfSpace from bevy_render to bevy_math. A…

cfc34ec

…lso move necessary baggage. Signed-off-by: Torstein Grindvik <torstein.grindvik@muybridge.com>

Fix typo by just removing that comment altogether

10dea9d

Signed-off-by: Torstein Grindvik <torstein.grindvik@muybridge.com>

CI

01d357a

Signed-off-by: Torstein Grindvik <torstein.grindvik@muybridge.com>

torsteingrindvik mentioned this pull request Jun 19, 2024

Placeholder: Clean up remaining primitives in bevy_render #13931

Closed

Use allow missing docs, link to follow up, use indent from recent con…

be45e64

…flict for rust lints Signed-off-by: Torstein Grindvik <torstein.grindvik@muybridge.com>

torsteingrindvik force-pushed the move-math-stuff branch from 591e4ee to be45e64 Compare June 19, 2024 18:11

alice-i-cecile approved these changes Jun 19, 2024

View reviewed changes

alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jun 19, 2024

Jondolf reviewed Jun 20, 2024

View reviewed changes

alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Jun 20, 2024

Jondolf mentioned this pull request Jun 20, 2024

Resolve overlap between bevy_render primitives and bevy_math primitives #13945

Open

torsteingrindvik closed this Aug 6, 2024

kfc35 mentioned this pull request Jan 24, 2026

Move HalfSpace and some of Frustum (renamed to ViewFrustum) from bevy_camera to bevy_math #22684

Merged

Uh oh!

Conversation

torsteingrindvik commented Jun 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Changelog

Migration Guide

Uh oh!

torsteingrindvik commented Jun 16, 2024

Uh oh!

alice-i-cecile Jun 16, 2024

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

torsteingrindvik commented Jun 18, 2024

Uh oh!

alice-i-cecile commented Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

torsteingrindvik commented Jun 19, 2024

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

Jondolf commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jondolf left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jondolf Jun 20, 2024

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Jun 20, 2024

Uh oh!

Jondolf commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

torsteingrindvik commented Aug 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

torsteingrindvik commented Jun 16, 2024 •

edited

Loading

alice-i-cecile commented Jun 18, 2024 •

edited

Loading

Jondolf commented Jun 20, 2024 •

edited

Loading

Jondolf left a comment •

edited

Loading

Jondolf commented Jun 20, 2024 •

edited

Loading