Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd is_backface_visible. #223
Conversation
|
It would be good to have a few tests for this. |
| @@ -177,6 +177,17 @@ where T: Copy + Clone + | |||
| ) | |||
| } | |||
|
|
|||
| /// Checks if we're facing backface | |||
This comment has been minimized.
This comment has been minimized.
nical
Aug 24, 2017
Collaborator
The comment is a bit confusing because the matrix itself does not face any direction. Could you replace it with something like "Check whether shapes on the XY plane with Z pointing towards the screen transformed by this matrix would be facing back."
|
Thanks for comment. I've replaced the comment and add some tests. |
| @@ -177,6 +177,18 @@ where T: Copy + Clone + | |||
| ) | |||
| } | |||
|
|
|||
| /// Check whether shapes on the XY plane with Z pointing towards the | |||
| /// screen transformed by this matrix would be facing back. | |||
This comment has been minimized.
This comment has been minimized.
kvark
Aug 24, 2017
Member
I think the comment needs to be fixed: "Z pointing towards the screen" implies that it's front facing already
This comment has been minimized.
This comment has been minimized.
nical
Aug 24, 2017
Collaborator
It implies "something front-facing being transformed will become back-facing" which is maybe a bit convoluted but I believe it conveys the correct information.
This comment has been minimized.
This comment has been minimized.
mephisto41
Aug 24, 2017
Author
Contributor
I think the comment is right. It says "XY plane with Z pointing towards the screen" transformed by "this matrix" would be facing back. Which means a plane facing the screen become facing back after transformed by this matrix.
| /// Check whether shapes on the XY plane with Z pointing towards the | ||
| /// screen transformed by this matrix would be facing back. | ||
| pub fn is_backface_visible(&self) -> bool { | ||
| // inverse().m33 < 0; |
This comment has been minimized.
This comment has been minimized.
kvark
Aug 24, 2017
Member
could you provide the links/sources onto why you think inverse().m33 < 0 is our target condition for backface visibility?
This comment has been minimized.
This comment has been minimized.
nical
Aug 24, 2017
Collaborator
Not terribly useful to understand the underlying math but this is ported from gecko's code apparently: https://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/gfx/2d/Matrix.h#1392
This comment has been minimized.
This comment has been minimized.
mephisto41
Aug 24, 2017
Author
Contributor
I refer to gecko's implementation. https://searchfox.org/mozilla-central/source/gfx/2d/Matrix.h#1392-1400
| @@ -662,6 +674,7 @@ mod tests { | |||
| use super::*; | |||
|
|
|||
| use std::f32::consts::FRAC_PI_2; | |||
| use std::f32::consts::PI; | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Alright, thanks for explanation! |
|
@bors-servo r+ |
|
|
Add is_backface_visible. We need this utility function for servo/webrender#1419 r? @kvark <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/euclid/223) <!-- Reviewable:end -->
|
|
mephisto41 commentedAug 24, 2017
•
edited by larsbergstrom
We need this utility function for servo/webrender#1419
r? @kvark
This change is