Skip to content

Add information about hit side to Ray3d::intersect_plane #20166

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Mati18505
Copy link

@Mati18505 Mati18505 commented Jul 16, 2025

Objective

Currently, Ray3d::intersect_plane always returns intersections on both faces of the plane, including backface hits.
This behavior:

  • isn’t documented clearly,
  • cannot be changed by the user without duplicating the method.

Fixes #20152

Solution

  • Introduced HitSide enum to allow the user to retrieve information about intersected plane side.
  • Changed return type of Ray3d::intersect_plane from Option<f32> to Option<(f32, HitSide)>.

Testing

Added unit tests.


Showcase

if let Some((distance, hit_side)) = ray.intersect_plane(Vec3::Z, InfinitePlane3d::new(Vec3::Z)) {
  match hit_side {
    HitSide::Front => (),
    HitSide::Back => (),
  }
}

Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile requested review from Jondolf and atlv24 July 16, 2025 20:42
@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Math Fundamental domain-agnostic mathematical operations M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 16, 2025
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

@Mati18505 Mati18505 force-pushed the Ray3d_add_backface branch from 36242b7 to 52d50d2 Compare July 16, 2025 20:44
@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Jul 16, 2025
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jul 16, 2025

To me the big question here is if we solidify the fact that a plane has a "front" (the current approach), or if we should be creating a new Halfspace type. I am currently undecided, but we should debate it a bit.

Copy link
Contributor

@IQuick143 IQuick143 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I'm not a huge fan of putting everything everything into one universal method. Though the code diff is actually nicer than what I expected. (ex: All the backface-relevant data is already computed anyway and the method is already full of branches.) So I'm not against merging this.

Given that the user has to supply an argument (whose documentation explains the details), so they won't be surprised by what the method does.

@atlv24
Copy link
Contributor

atlv24 commented Jul 20, 2025

How about making the function params stay the same but changing the output to Option<(f32, HitSide)> where enum HitSide { Front, Back } or something like that, then have users match on it or deconstruct-discard it?

@IQuick143
Copy link
Contributor

That would work by me too yeah.

@alice-i-cecile
Copy link
Member

I like changing the return type!

@Mati18505
Copy link
Author

Thanks for all the suggestions! I agree that using the return type instead of parameter makes everything cleaner.

I've updated Ray3d::intersect_plane to return Option<(f32, HitSide)>, where HitSide is an enum with Front and Back variants.

I'm wondering if Ray2d should also have HitSide for consistency.

@Mati18505 Mati18505 changed the title Add optional backface/frontface culling to Ray3d::intersect_plane Add information about hitted side to Ray3d::intersect_plane Jul 25, 2025
@Mati18505 Mati18505 changed the title Add information about hitted side to Ray3d::intersect_plane Add information about hitted side to Ray3d::intersect_plane Jul 25, 2025
@Mati18505 Mati18505 changed the title Add information about hitted side to Ray3d::intersect_plane Add information about hit side to Ray3d::intersect_plane Jul 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Math Fundamental domain-agnostic mathematical operations C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ray3d::intersect_plane's behavior with respect to backface hits is unclear
4 participants