bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
36.2k stars 3.57k forks source link

`bevy_render::Frustum` should live in `bevy_math` #13878

Open alice-i-cecile opened 4 months ago

alice-i-cecile commented 4 months ago

What problem does this solve or what need does it fill?

Our existing Frustum type is used for computing what cameras should see.

However, as this existed before the addition of our mathematical primitive types, it a) doesn't live in bevy_math like the others and b) is missing all of the standard expected functionality for it.

What solution would you like?

  1. Move HalfSpace into bevy_math unchanged.
  2. Move Frustum into bevy_math unchanged.
  3. Implement gizmos for frustums.

1 and 2 should be done in a single PR. 3 should be done in a follow-up.

We can later add all of the other weird and wonderful things (random sampling, meshability) to these types if people think it's worthwhile, but those steps are the key things.

Once things are moved, we can look into changing naming, representation and so on. Those should be done in tightly scoped PRs, with aggressive benchmarking for frustrum culling performance. This is directly in the rendering hot loop: changes here have a high impact, for both better and worse.

Additional context

Raised on Discord. @superdump gave us rendering's blessing, as long as we don't regress performance.

The remaining bevy_render::primitives types should be moved / consolidated eventually, but gizmos for frustums is the motivating requirement here.

torsteingrindvik commented 4 months ago

Had a quick look at 1. + 2. and I notice that moving HalfSpace and Frustum comes with a couple of stumbling blocks.

I'm assuming there should be no dependency on bevy_render in bevy_math, even behind a feature flag. Should those two structs also be moved to bevy_math?

Lastly e.g. Frustum has an impl of Component, but bevy_math does not yet have a dependency on bevy_ecs. My assumption here is that adding bevy_ecs as an optional dep is ok.

alice-i-cecile commented 4 months ago

I'm assuming there should be no dependency on bevy_render in bevy_math, even behind a feature flag. Should those two structs also be moved to bevy_math?

Yes, but in their own legacy_bevy_render module or something to clearly mark them as weird and something we need to clean up. Definitely no dependency in that direction :)

Lastly e.g. Frustum has an impl of Component, but bevy_math does not yet have a dependency on bevy_ecs. My assumption here is that adding bevy_ecs as an optional dep is ok.

Yep, follow the pattern established for bevy_reflect here.

janhohenheim commented 4 months ago

Would it make sense to move AABB and Sphere into bevy_math as well?

alice-i-cecile commented 4 months ago

Yes! And then probably replace both of them.

torsteingrindvik commented 4 months ago

https://github.com/bevyengine/bevy/issues/13931

The PR mentions a follow up, linking it here since it's relevant.