Skip to content

Conversation

atlv24
Copy link
Contributor

@atlv24 atlv24 commented Aug 22, 2025

Objective

  • Fixes a bug where Frustums constructed by from_clip_from_world_no_far will consider all Aabbs and all zero-radius spheres to not be in the frustum if far plane culling is enabled

Solution

  • initialize to a correct default

Testing

  • bug has not been observed in the wild, only discovered by audit. example. from intersects_obb, in the halfspace loop:
let relative_radius = aabb.relative_radius(&p_normal, &world_from_local.matrix3);
if half_space.normal_d().dot(aabb_center_world) + relative_radius <= 0.0 {
    return false;
}

relative radius is a bunch of dot products with the frustum plane (which is 0) so it is 0.
normal dot aabb center again is 0, 0 + 0 is 0, which is <= 0. The if gets hit, and intersects returns false, wrongly.

Changing d to f32::MAX instead makes the normal_d dot center extended by 1 result in f32::MAX, which is not <= 0, and thus intersect can succeed

@atlv24 atlv24 added A-Rendering Drawing game state to the screen S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 22, 2025
@atlv24 atlv24 added the C-Bug An unexpected or incorrect behavior label Aug 22, 2025
@alice-i-cecile 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 S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Straightforward Simple bug fixes and API improvements, docs, test and examples and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Aug 24, 2025
@atlv24
Copy link
Contributor Author

atlv24 commented Aug 25, 2025

Something to note I didnt tackle here is that HalfSpace is a pub type and impl's Default. I would say the current Default doesnt make sense, but there's actually no .contains method on it, its just a thin wrapper over Vec4. We may want to pull this out into bevy_math and make it more broadly useful (with an appropriate Default impl) at some point. This is just a minimal bugfix.

@atlv24 atlv24 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-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 25, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 26, 2025
Merged via the queue into bevyengine:main with commit cf31b18 Aug 26, 2025
47 checks passed
@atlv24 atlv24 deleted the ad/frustum-no-far branch September 9, 2025 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants