-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Fix plane model matrix #9112
Fix plane model matrix #9112
Conversation
Thanks for the pull request @baothientran!
Reviewers, don't forget to make sure that:
|
@IanLilleyT can you review? I think you had the same idea when you wrote up #8268 |
@baothientran can you update |
|
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.
Looks good and fixes the problems in this sandcastle
Just one minor change in the comments.
1.0, | ||
CesiumMath.EPSILON8 | ||
) | ||
) { | ||
up = Cartesian3.clone(Cartesian3.UNIT_Z, up); | ||
up = Cartesian3.clone(Cartesian3.UNIT_Y, up); | ||
if ( |
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 know this isn't code you added, but I think we can safely delete this section (lines 347 to 355). There's no way the initial up guess is aligned to both the Z and Y axes, so setting to UNIT_X
will never happen.
@IanLilleyT I updated to remove the unreachable condition that aligns up with X-axis. |
Thanks @baothientran ! |
Fixes #8268
Currently the rotation is constructed manually to align the plane with the surface of the ellipsoid. This works correctly if the plane's normal is not aligned with the main axis. Instead of transforming the plane's origin and normal to the world coordinate first, we can orient the plane in the local coordinate using its original normal and origin. Then, we multiply the local transform with the world transform to convert to the world coordinate. Because the world transform already has the rotation matrix that will align the plane with the ellipsoid's surface, this should work with all of the examples as before.
@lilleyse or @hpinkos Can you please take a look at it? Please let me know if it is a right approach