Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1860823003:
Enhance unit tests for normals. (Closed)
|
Created:
4 years, 8 months ago by Peter Mayo Modified:
4 years, 8 months ago Reviewers:
Ian Vollick CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnhance unit tests for normals.
We asserted that changing vertex winding shouldn't change a
polygon's normal when it doesn't have to.
We asserted that inversion on the X axis (or Y-Axis) should change the normal, but it shouldn't.
We also tested the normal calculation of degenerate polygons (colinear vertices) when these don't have to work, and weren't what the test was claiming to check.
This introduces DISABLED tests to fix, separates the set of normal
calculations into separate tests so that failures are reported better, and uses better formed polygons for the touching and splitting test cases.
BUG=595820
TEST=self
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/94858ade402cdf1fec143c154d600ee7b17c3557
Cr-Commit-Position: refs/heads/master@{#386470}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix comments from me. #
Total comments: 2
Patch Set 3 : Remove BackwardsWoundNormal Test #Patch Set 4 : rebased #
Total comments: 2
Patch Set 5 : Fix degenerate polys #Patch Set 6 : Rebase #Patch Set 7 : Rebase #
Total comments: 1
Patch Set 8 : Added a comment for the DISABLED test with a direct bug #Messages
Total messages: 26 (7 generated)
Description was changed from ========== Enhance unit tests for normals. We fail to highlight that changing vertex winding shouldn't change a polygon's normal, nor should inversion on the X axis (or Y-Axis) This isntroduces DISABLED tests to fix and separates the set of normal calculations into separate tests so that failures are reported better. BUG=595820 TEST=self ========== to ========== Enhance unit tests for normals. We fail to highlight that changing vertex winding shouldn't change a polygon's normal, nor should inversion on the X axis (or Y-Axis) This isntroduces DISABLED tests to fix and separates the set of normal calculations into separate tests so that failures are reported better. BUG=595820 TEST=self CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
petermayo@chromium.org changed reviewers: + vollick@chromium.org
Simple setup for future fix.
https://codereview.chromium.org/1860823003/diff/1/cc/quads/draw_polygon_unitt... File cc/quads/draw_polygon_unittest.cc (right): https://codereview.chromium.org/1860823003/diff/1/cc/quads/draw_polygon_unitt... cc/quads/draw_polygon_unittest.cc:140: gfx::Transform tranform_a(0, 1, 0, 0, 1, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1); sp: tran-s-form https://codereview.chromium.org/1860823003/diff/1/cc/quads/draw_polygon_unitt... cc/quads/draw_polygon_unittest.cc:143: EXPECT_NORMAL(polygon_a, 0.0f, 0.0f, -1.0f); Wong - swapping Axes should not invert the normal. It can be accomplised by a invert along the axis and a rotation.
https://codereview.chromium.org/1860823003/diff/20001/cc/quads/draw_polygon_u... File cc/quads/draw_polygon_unittest.cc (right): https://codereview.chromium.org/1860823003/diff/20001/cc/quads/draw_polygon_u... cc/quads/draw_polygon_unittest.cc:85: EXPECT_NORMAL(polygon, 0.0f, 0.0f, 1.0f); This is surprising to me. We want reverse winding to produce the same normal? I know that scaleX(-1) will change winding order and shouldn't change b/f visibility, but in this test case I would have expected to see a different transform in addition to a different winding order.
https://codereview.chromium.org/1860823003/diff/20001/cc/quads/draw_polygon_u... File cc/quads/draw_polygon_unittest.cc (right): https://codereview.chromium.org/1860823003/diff/20001/cc/quads/draw_polygon_u... cc/quads/draw_polygon_unittest.cc:85: EXPECT_NORMAL(polygon, 0.0f, 0.0f, 1.0f); On 2016/04/06 12:32:22, vollick wrote: > This is surprising to me. We want reverse winding to produce the same normal? I > know that scaleX(-1) will change winding order and shouldn't change b/f > visibility, but in this test case I would have expected to see a different > transform in addition to a different winding order. I think we want to be insensitive to the winding order because the CREATE_TEST_DRAW_POLYGON macros supplies a normal to the constructor that is z-positive, as well as the fact that I expect the result of constructing from vertices to be the same if we have pre-transformed them. This isn't the path most of the polygons in chrome come through. They get created from a Rect & a transform, for which we have a better handle on the normal, and I expect the winding order to be constant. Regardless, as you point out the winding order can be changed by a transform that doesn't change the normal, so I think we should not use it even as a hint. Perhaps a comment to indicate this is a strong decision?
On 2016/04/06 14:05:00, Peter Mayo wrote: > https://codereview.chromium.org/1860823003/diff/20001/cc/quads/draw_polygon_u... > File cc/quads/draw_polygon_unittest.cc (right): > > https://codereview.chromium.org/1860823003/diff/20001/cc/quads/draw_polygon_u... > cc/quads/draw_polygon_unittest.cc:85: EXPECT_NORMAL(polygon, 0.0f, 0.0f, 1.0f); > On 2016/04/06 12:32:22, vollick wrote: > > This is surprising to me. We want reverse winding to produce the same normal? > I > > know that scaleX(-1) will change winding order and shouldn't change b/f > > visibility, but in this test case I would have expected to see a different > > transform in addition to a different winding order. > > I think we want to be insensitive to the winding order because the > CREATE_TEST_DRAW_POLYGON macros supplies a normal to the constructor > that is z-positive, as well as the fact that I expect the result of constructing > from vertices to be the same if we have pre-transformed them. This isn't the > path most of the polygons in chrome come through. They get created from a Rect > & a transform, for which we have a better handle on the normal, and I expect the > winding order to be constant. > > Regardless, as you point out the winding order can be changed by a transform > that doesn't change the normal, so I think we should not use it even as a hint. Say local transform is M (and say M = scaleX(-1)). I expected that our normal would be edge1.cross(edge2) * (lefthanded(M) ? 1 : -1) (modulo some business about ensuring that edge1 and edge2 are linearly independent). That is, both our winding order and how we use it to derive normals are dependent on M. Is this not true? > Perhaps a comment to indicate this is a strong decision?
On 2016/04/06 14:11:05, vollick wrote: > On 2016/04/06 14:05:00, Peter Mayo wrote: > > > https://codereview.chromium.org/1860823003/diff/20001/cc/quads/draw_polygon_u... > > File cc/quads/draw_polygon_unittest.cc (right): > > > > > https://codereview.chromium.org/1860823003/diff/20001/cc/quads/draw_polygon_u... > > cc/quads/draw_polygon_unittest.cc:85: EXPECT_NORMAL(polygon, 0.0f, 0.0f, > 1.0f); > > On 2016/04/06 12:32:22, vollick wrote: > > > This is surprising to me. We want reverse winding to produce the same > normal? > > I > > > know that scaleX(-1) will change winding order and shouldn't change b/f > > > visibility, but in this test case I would have expected to see a different > > > transform in addition to a different winding order. > > > > I think we want to be insensitive to the winding order because the > > CREATE_TEST_DRAW_POLYGON macros supplies a normal to the constructor > > that is z-positive, as well as the fact that I expect the result of > constructing > > from vertices to be the same if we have pre-transformed them. This isn't the > > path most of the polygons in chrome come through. They get created from a > Rect > > & a transform, for which we have a better handle on the normal, and I expect > the > > winding order to be constant. > > > > Regardless, as you point out the winding order can be changed by a transform > > that doesn't change the normal, so I think we should not use it even as a > hint. > > Say local transform is M (and say M = scaleX(-1)). I expected that our normal > would be edge1.cross(edge2) * (lefthanded(M) ? 1 : -1) (modulo some business > about ensuring that edge1 and edge2 are linearly independent). > > That is, both our winding order and how we use it to derive normals are > dependent on M. > > Is this not true? > > > Perhaps a comment to indicate this is a strong decision? s/local transform/transform/ (blush).
One of the constructors (the one being exercised here) has points and a
normal, but no transform.
DrawPolygon::DrawPolygon(const DrawQuad* original,
const std::vector<gfx::Point3F>& in_points,
const gfx::Vector3dF& normal,
int draw_order_index)
On 6 April 2016 at 10:11, <vollick@chromium.org> wrote:
> On 2016/04/06 14:05:00, Peter Mayo wrote:
> >
>
>
https://codereview.chromium.org/1860823003/diff/20001/cc/quads/draw_polygon_u...
> > File cc/quads/draw_polygon_unittest.cc (right):
> >
> >
>
>
https://codereview.chromium.org/1860823003/diff/20001/cc/quads/draw_polygon_u...
> > cc/quads/draw_polygon_unittest.cc:85: EXPECT_NORMAL(polygon, 0.0f, 0.0f,
> 1.0f);
> > On 2016/04/06 12:32:22, vollick wrote:
> > > This is surprising to me. We want reverse winding to produce the same
> normal?
> > I
> > > know that scaleX(-1) will change winding order and shouldn't change b/f
> > > visibility, but in this test case I would have expected to see a
> different
> > > transform in addition to a different winding order.
> >
> > I think we want to be insensitive to the winding order because the
> > CREATE_TEST_DRAW_POLYGON macros supplies a normal to the constructor
> > that is z-positive, as well as the fact that I expect the result of
> constructing
> > from vertices to be the same if we have pre-transformed them. This isn't
> the
> > path most of the polygons in chrome come through. They get created from a
> Rect
> > & a transform, for which we have a better handle on the normal, and I
> expect
> the
> > winding order to be constant.
> >
> > Regardless, as you point out the winding order can be changed by a
> transform
> > that doesn't change the normal, so I think we should not use it even as a
> hint.
>
> Say local transform is M (and say M = scaleX(-1)). I expected that our
> normal
> would be edge1.cross(edge2) * (lefthanded(M) ? 1 : -1) (modulo some
> business
> about ensuring that edge1 and edge2 are linearly independent).
>
> That is, both our winding order and how we use it to derive normals are
> dependent on M.
>
> Is this not true?
>
> > Perhaps a comment to indicate this is a strong decision?
>
>
>
> https://codereview.chromium.org/1860823003/
>
--
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
On 2016/04/06 14:13:34, chromium-reviews wrote: > One of the constructors (the one being exercised here) has points and a > normal, but no transform. > > DrawPolygon::DrawPolygon(const DrawQuad* original, > const std::vector<gfx::Point3F>& in_points, > const gfx::Vector3dF& normal, > int draw_order_index) > > Yeah - what they said.
rebased
https://codereview.chromium.org/1860823003/diff/60001/cc/quads/draw_polygon_u... File cc/quads/draw_polygon_unittest.cc (right): https://codereview.chromium.org/1860823003/diff/60001/cc/quads/draw_polygon_u... cc/quads/draw_polygon_unittest.cc:246: vertices_b.push_back(gfx::Point3F(5.0f, 0.0f, 15.0f)); This is a degenerate polygon (line) as written. https://codereview.chromium.org/1860823003/diff/60001/cc/quads/draw_polygon_u... cc/quads/draw_polygon_unittest.cc:268: vertices_b.push_back(gfx::Point3F(5.0f, 0.0f, -10.0f)); This is a degenerate polygon (line) as written.
Fix degenerate polys
Rebase
Rebase
Description was changed from ========== Enhance unit tests for normals. We fail to highlight that changing vertex winding shouldn't change a polygon's normal, nor should inversion on the X axis (or Y-Axis) This isntroduces DISABLED tests to fix and separates the set of normal calculations into separate tests so that failures are reported better. BUG=595820 TEST=self CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Enhance unit tests for normals. We asserted that changing vertex winding shouldn't change a polygon's normal when it doesn't have to. We asserted that inversion on the X axis (or Y-Axis) should change the normal, but it shouldn't. We also tested the normal calculation of degenerate polygons (colinear vertices) when these don't have to work, and weren't what the test was claiming to check. This introduces DISABLED tests to fix, separates the set of normal calculations into separate tests so that failures are reported better, and uses better formed polygons for the touching and splitting test cases. BUG=595820 TEST=self CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Ping.
lgtm. https://codereview.chromium.org/1860823003/diff/120001/cc/quads/draw_polygon_... File cc/quads/draw_polygon_unittest.cc (right): https://codereview.chromium.org/1860823003/diff/120001/cc/quads/draw_polygon_... cc/quads/draw_polygon_unittest.cc:134: Would be nice to add a comment with a link to the bug for enabling these.
Added a comment for the DISABLED test with a direct bug
The CQ bit was checked by petermayo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vollick@chromium.org Link to the patchset: https://codereview.chromium.org/1860823003/#ps140001 (title: "Added a comment for the DISABLED test with a direct bug")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1860823003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1860823003/140001
Message was sent while issue was closed.
Description was changed from ========== Enhance unit tests for normals. We asserted that changing vertex winding shouldn't change a polygon's normal when it doesn't have to. We asserted that inversion on the X axis (or Y-Axis) should change the normal, but it shouldn't. We also tested the normal calculation of degenerate polygons (colinear vertices) when these don't have to work, and weren't what the test was claiming to check. This introduces DISABLED tests to fix, separates the set of normal calculations into separate tests so that failures are reported better, and uses better formed polygons for the touching and splitting test cases. BUG=595820 TEST=self CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Enhance unit tests for normals. We asserted that changing vertex winding shouldn't change a polygon's normal when it doesn't have to. We asserted that inversion on the X axis (or Y-Axis) should change the normal, but it shouldn't. We also tested the normal calculation of degenerate polygons (colinear vertices) when these don't have to work, and weren't what the test was claiming to check. This introduces DISABLED tests to fix, separates the set of normal calculations into separate tests so that failures are reported better, and uses better formed polygons for the touching and splitting test cases. BUG=595820 TEST=self CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Enhance unit tests for normals. We asserted that changing vertex winding shouldn't change a polygon's normal when it doesn't have to. We asserted that inversion on the X axis (or Y-Axis) should change the normal, but it shouldn't. We also tested the normal calculation of degenerate polygons (colinear vertices) when these don't have to work, and weren't what the test was claiming to check. This introduces DISABLED tests to fix, separates the set of normal calculations into separate tests so that failures are reported better, and uses better formed polygons for the touching and splitting test cases. BUG=595820 TEST=self CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Enhance unit tests for normals. We asserted that changing vertex winding shouldn't change a polygon's normal when it doesn't have to. We asserted that inversion on the X axis (or Y-Axis) should change the normal, but it shouldn't. We also tested the normal calculation of degenerate polygons (colinear vertices) when these don't have to work, and weren't what the test was claiming to check. This introduces DISABLED tests to fix, separates the set of normal calculations into separate tests so that failures are reported better, and uses better formed polygons for the touching and splitting test cases. BUG=595820 TEST=self CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/94858ade402cdf1fec143c154d600ee7b17c3557 Cr-Commit-Position: refs/heads/master@{#386470} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/94858ade402cdf1fec143c154d600ee7b17c3557 Cr-Commit-Position: refs/heads/master@{#386470} |
