-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Move HalfSpace and some of Frustum (renamed to ViewFrustum) from bevy_camera to bevy_math
#22684
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
base: main
Are you sure you want to change the base?
Conversation
cb93bc3 to
ff89364
Compare
ff89364 to
b34bb28
Compare
| #[reflect(ignore, clone)] | ||
| pub half_spaces: [HalfSpace; 6], | ||
| } | ||
| pub struct Frustum(pub ViewFrustum); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is that even a thing at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t actually know, but I’m not curious/courageous enough to try removing it
Trying to be as minimally invasive for this PR as possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dunno #22693
|
Looks like one of your doc links is broken :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm generally in favor of something like this! I think starting with a direct port like this is fine, though long-term, I'd like to handle things a bit differently. I didn't look at the code too deeply yet, just some general initial thoughts in this area:
- I would personally prefer if we replaced our existing plane types with a representation like this one (normal and signed distance). The existing ones are kinda useless. Edit: specifically infinite planes, I don't really care what we do with finite planes, I'd make them meshing-only or remove them
- Planes are more general-purpose, and a half-space is also defined by a plane, just with the additional semantics that it represents one of the two regions created by the plane, not the plane itself. IMO these semantics can generally be derived from surrounding context (ex: you could have a
half_spaceproperty that is still defined by aPlane3d). - Alternatively, we could also keep
HalfSpace2dandHalfSpace3dfor their special semantics, but as newtypes overPlane2dandPlane3d. - Note that I have my own
Plane2dandPlane3dtypes implemented locally. IMO they have a better and more complete API than the half-space type here. I made a gist for them here.
- Planes are more general-purpose, and a half-space is also defined by a plane, just with the additional semantics that it represents one of the two regions created by the plane, not the plane itself. IMO these semantics can generally be derived from surrounding context (ex: you could have a
- I would like to have both
HalfSpace2dandHalfSpace3d.HalfSpaceshouldn't be a 3D-specific type. - This has been discussed a million times, but we really need to figure out what shapes we include in
bevy_mathand if/how to categorize them. Unlike most other shapes, these probably won't have meshing or some other features.
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
atlv24
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally onboard with this direction. I avoid touch bevy_math because people have a lot of conflicting opinions about it which makes PRs stagnate and die, so while I would have chosen different names for things I dont want to feel strongly enough about it that i contribute to the problem.
Im fine with merging as is, provided CI passes. I have some feelings about the NEAR/FAR constants living in bevy_math, and the more render-y specific stuff being here, but eh. I like seeing any movement at all on this. Agree with Jondolf about 2d/3d variants in the future, maybe good to make space for them now by going to HalfSpace3d immediately.
f79ab7e to
510723b
Compare
alice-i-cecile
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this as an incremental improvement, but unsurprisingly Jondolf has good takes on the longterm plans :)
Objective
bevy_renderprimitives andbevy_mathprimitives #13945bevy_render::Frustumshould live inbevy_math#13878Solution
bevy_camera::primitives::HalfSpacetobevy_math::primitives::HalfSpace(first commit)bevy_camera::primitives::Frustumtobevy_math::primitives::ViewFrustum(second commit)Testing
I ran the
lighting,2d_gizmos, and3d_gizmosexample 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.