Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
PerspectiveCamera: New helper methods
getFrustumBounds()
,getFrustumSize()
. #27574PerspectiveCamera: New helper methods
getFrustumBounds()
,getFrustumSize()
. #27574Changes from 17 commits
16563ef
2c3d816
e8a73c0
99bf0c7
8351897
253fb2a
98886c9
537e1ea
7162815
8376562
8e019c3
a8e2add
fc2867d
27a49c6
ab65b6e
84b0c11
fcab13f
ef8ba73
e81542d
601461c
d66410e
2432011
03bdbe2
7ae93f5
c1024ad
c9f5641
45f8fae
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 would return values in camera space - not world space.
topRight
,topLeft
, have no meaning once the camera is rotated.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.
It seems the tests developers usually perform with these coordinates happen in world space. If the vectors are now in camera space, users have to convert their data into camera space as well OR convert the frustum corner points to world space.
I wonder if this makes the workflow unnecessarily complicated.
We often compute bounding volumes in world space knowing they are only valid as long as the object does not move. I think we can make the same assumptions for the corner points as well.
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.
As I said,
topRight
,topLeft
, have no meaning in world space. Return the values in camera space. Any user using this method knows how to convert a point from camera space into world space -- if that is what they want.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.
See it as a stretch to say
topLeft
fully looses all meaning if the camera is rotated? Agnostic of camera rotation or reference coordinate space, the topLeft vector will still be the point that is rendered to the topLeft of the screen.Agree that if we return the results in camera-space it blunts the utility meant to be added here. Think world-space is the right play and that the nature of the top/bottom/left/right is still easily apparent.
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.
In addition to my previous comments, I do not think
.getFrustumBounds()
and.getFrustumCorners()
should return results in different coordinate frames.And I agree,
.getFrustumCorners()
provides little added utility.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.
@Bug-Reaper To avoid an impasse here, I suggest you remove
getFrustumCorners()
from this PR and create a separate one. In this way, we can at least get the changes toPerspectiveCamera
merged now.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.
Okay guys, gonna remove
getFrustumCorners()
from this PR so we can get the goods merged ⭐️I do believe there's tangible utility added from a method that returns the corners in world-space. Perhaps the name conventions can be improved to clearly indicate the functionality. Will take the feedback from this discussion to heart and try to keep it in mind when I get a chance to do a seperate PR for this method.