Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
411793002:
DrawPolygon class with Unit Tests (Closed)
|
Created:
6 years, 5 months ago by troyhildebrandt Modified:
6 years, 4 months ago Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionDrawPolygon class with Unit Tests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286316
Patch Set 1 #
Total comments: 5
Patch Set 2 : #Patch Set 3 : Added non quad area test #
Total comments: 29
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #Patch Set 6 : Removed area calcs, transform normal in ApplyTransform, transform normal unittest #Patch Set 7 : Separated ApplyTransform from normal transforms #
Total comments: 22
Patch Set 8 : #Patch Set 9 : #
Total comments: 22
Patch Set 10 : #
Total comments: 9
Patch Set 11 : #
Total comments: 3
Patch Set 12 : #Patch Set 13 : Changed const int return to int #Patch Set 14 : Fixed some MSVC compile issues. #Patch Set 15 : Added CC_EXPORT to DrawPolygon, fixed gyp/build issues. #
Messages
Total messages: 30 (0 generated)
https://codereview.chromium.org/411793002/diff/1/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/411793002/diff/1/cc/quads/draw_polygon.cc#new... cc/quads/draw_polygon.cc:25: static float SignedArea(const DrawPolygon& polygon) { Thinking about simplifying things or removing complexity, you're only using area to do the original sort, right? If so, why not just look at the original draw quad and multiply width by height and then be done. Area can just be "OriginalArea". https://codereview.chromium.org/411793002/diff/1/cc/quads/draw_polygon.cc#new... cc/quads/draw_polygon.cc:199: points[i] = MathUtil::MapPoint(transform, points[i], &clipped); Will this clobber |clipped| every time it is called? Do you even use this return value?
https://codereview.chromium.org/411793002/diff/1/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/411793002/diff/1/cc/quads/draw_polygon.cc#new... cc/quads/draw_polygon.cc:25: static float SignedArea(const DrawPolygon& polygon) { On 2014/07/23 00:22:42, enne wrote: > Thinking about simplifying things or removing complexity, you're only using area > to do the original sort, right? If so, why not just look at the original draw > quad and multiply width by height and then be done. Area can just be > "OriginalArea". The area gets recalculated when things are split. In the case that it is the original quad though, we can definitely speed up the calculation by just doing that. We can also avoid the area calculation on the second piece of a split. https://codereview.chromium.org/411793002/diff/1/cc/quads/draw_polygon.cc#new... cc/quads/draw_polygon.cc:199: points[i] = MathUtil::MapPoint(transform, points[i], &clipped); On 2014/07/23 00:22:42, enne wrote: > Will this clobber |clipped| every time it is called? > > Do you even use this return value? It definitely clobbers clipped every single time, but MapPoint requires us to feed it that boolean so no real choice there. The return value isn't used anywhere though, and would be useless in this case, so that's been changed to void.
https://codereview.chromium.org/411793002/diff/1/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/411793002/diff/1/cc/quads/draw_polygon.cc#new... cc/quads/draw_polygon.cc:25: static float SignedArea(const DrawPolygon& polygon) { On 2014/07/23 19:47:56, troyhildebrandt wrote: > On 2014/07/23 00:22:42, enne wrote: > > Thinking about simplifying things or removing complexity, you're only using > area > > to do the original sort, right? If so, why not just look at the original draw > > quad and multiply width by height and then be done. Area can just be > > "OriginalArea". > > The area gets recalculated when things are split. In the case that it is the > original quad though, we can definitely speed up the calculation by just doing > that. We can also avoid the area calculation on the second piece of a split. Could you test areas of non-quads, then?
https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:159: gfx::Vector3dF vec1 = plane_origin - line_start; Can you name these better than vec1 and vec2? https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:180: v.Scale(1.f / v.Length()); Can you leave a comment about what this is all doing? Pretend like nobody else understands math and will send you angry emails asking you to explain it. https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:285: if (points.size() == 0) Should this be points.size() <= 2? https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:288: // op1 = offset plus 1, op2 = offset plus 2. This comment isn't particularly helpful. Maybe instead explain what this algorithm is doing? https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.h File cc/quads/draw_polygon.h (right): https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:24: DrawPolygon(DrawQuad* original_ref, I'm not super keen on these raw pointers. I know why it's there (i.e. a performance benefit to not have to dupe info from DrawQuads), but I think this needs like a big warning on it. Something like "this DrawQuad is owned by the caller and its lifetime must be preserved as long as this DrawPolygon is alive". https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:25: gfx::Point3F* in_points, Could this be const std::vector<gfx::Point3F>&, rather than a raw pointer and a count? https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:28: DrawPolygon(const DrawPolygon& other); You've got a whole bunch of copying functions here. A copy ctor, an operator=, and a CopyFrom function. It looks like you're only using the copy ctor? I think it'd be clearer in calling code if you made a public scoped_ptr<DrawPolygon> CreateCopy() function and then got rid of the copy constructor and operator= and use the DISALLOW_COPY_AND_ASSIGN macro. https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:36: void ApplyTransform(const gfx::Transform& transform); Where does this function get used? Is this in GLRenderer somewhere? https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:38: std::vector<gfx::Point3F> points; Are these mutable after construction? Should they be private with public const accessors? https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:39: gfx::Vector3dF normal; You should document with a comment if normal is normalized or not. https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:41: int order_index; This needs a comment to explain what it is. https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:58: static float compare_threshold; I don't think you should put these in the header. If these don't get used outside of draw_polygon.cc, then just put them in an anonymous namespace in the .cc file like you do with coplanar_dot_epsilon. https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon_un... File cc/quads/draw_polygon_unittest.cc (right): https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon_un... cc/quads/draw_polygon_unittest.cc:71: EXPECT_EQ(DrawPolygon::SideCompare(polygon_b, polygon_a), BSP_SPLIT); Can you test the values of the points too?
https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:159: gfx::Vector3dF vec1 = plane_origin - line_start; On 2014/07/23 21:19:31, enne wrote: > Can you name these better than vec1 and vec2? Done. https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:180: v.Scale(1.f / v.Length()); On 2014/07/23 21:19:32, enne wrote: > Can you leave a comment about what this is all doing? Pretend like nobody else > understands math and will send you angry emails asking you to explain it. Done. https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:285: if (points.size() == 0) On 2014/07/23 21:19:31, enne wrote: > Should this be points.size() <= 2? Done. https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:288: // op1 = offset plus 1, op2 = offset plus 2. On 2014/07/23 21:19:31, enne wrote: > This comment isn't particularly helpful. Maybe instead explain what this > algorithm is doing? Done. https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.h File cc/quads/draw_polygon.h (right): https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:24: DrawPolygon(DrawQuad* original_ref, On 2014/07/23 21:19:32, enne wrote: > I'm not super keen on these raw pointers. I know why it's there (i.e. a > performance benefit to not have to dupe info from DrawQuads), but I think this > needs like a big warning on it. > > Something like "this DrawQuad is owned by the caller and its lifetime must be > preserved as long as this DrawPolygon is alive". Done. https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:25: gfx::Point3F* in_points, On 2014/07/23 21:19:32, enne wrote: > Could this be const std::vector<gfx::Point3F>&, rather than a raw pointer and a > count? Done. https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:36: void ApplyTransform(const gfx::Transform& transform); On 2014/07/23 21:19:32, enne wrote: > Where does this function get used? Is this in GLRenderer somewhere? Yea, this is used in the grander scheme of things to transform to 3D space for sorting and then back to 2D so they can be sent to GL as usual with all the current GL transformations. https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:38: std::vector<gfx::Point3F> points; On 2014/07/23 21:19:32, enne wrote: > Are these mutable after construction? Should they be private with public const > accessors? In order to implement the CreateCopy() function efficiently (without recalculating area, normal, etc.) it is necessary to create an empty DrawPolygon and then assign to the points afterwards. https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:39: gfx::Vector3dF normal; On 2014/07/23 21:19:32, enne wrote: > You should document with a comment if normal is normalized or not. Done. https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:41: int order_index; On 2014/07/23 21:19:32, enne wrote: > This needs a comment to explain what it is. Done. https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:58: static float compare_threshold; On 2014/07/23 21:19:32, enne wrote: > I don't think you should put these in the header. If these don't get used > outside of draw_polygon.cc, then just put them in an anonymous namespace in the > .cc file like you do with coplanar_dot_epsilon. At least one, compare_threshold, needs to visible in the BspTree unit tests. Not sure if we should find another way to do this, or maybe move just split_threshold into the .cc? https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon_un... File cc/quads/draw_polygon_unittest.cc (right): https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon_un... cc/quads/draw_polygon_unittest.cc:71: EXPECT_EQ(DrawPolygon::SideCompare(polygon_b, polygon_a), BSP_SPLIT); On 2014/07/23 21:19:32, enne wrote: > Can you test the values of the points too? Done.
https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.h File cc/quads/draw_polygon.h (right): https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:38: std::vector<gfx::Point3F> points; On 2014/07/24 00:43:21, troyhildebrandt wrote: > On 2014/07/23 21:19:32, enne wrote: > > Are these mutable after construction? Should they be private with public const > > accessors? > > In order to implement the CreateCopy() function efficiently (without > recalculating area, normal, etc.) it is necessary to create an empty DrawPolygon > and then assign to the points afterwards. Yeah, but CreateCopy is a member function so has access to these. They can still be private with public accessors? https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:58: static float compare_threshold; On 2014/07/24 00:43:21, troyhildebrandt wrote: > On 2014/07/23 21:19:32, enne wrote: > > I don't think you should put these in the header. If these don't get used > > outside of draw_polygon.cc, then just put them in an anonymous namespace in > the > > .cc file like you do with coplanar_dot_epsilon. > > At least one, compare_threshold, needs to visible in the BspTree unit tests. Not > sure if we should find another way to do this, or maybe move just > split_threshold into the .cc? I looked at that code, and I think you should just test SideCompare inside of DrawPolygon unit tests (which you're already doing) and then just reuse SideCompare in the BSP tests to verify that things got put on the side they should.
https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.h File cc/quads/draw_polygon.h (right): https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:38: std::vector<gfx::Point3F> points; On 2014/07/24 01:04:42, enne wrote: > On 2014/07/24 00:43:21, troyhildebrandt wrote: > > On 2014/07/23 21:19:32, enne wrote: > > > Are these mutable after construction? Should they be private with public > const > > > accessors? > > > > In order to implement the CreateCopy() function efficiently (without > > recalculating area, normal, etc.) it is necessary to create an empty > DrawPolygon > > and then assign to the points afterwards. > > Yeah, but CreateCopy is a member function so has access to these. They can > still be private with public accessors? You're right, I made all the member variables private. https://codereview.chromium.org/411793002/diff/40001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:58: static float compare_threshold; On 2014/07/24 01:04:42, enne wrote: > On 2014/07/24 00:43:21, troyhildebrandt wrote: > > On 2014/07/23 21:19:32, enne wrote: > > > I don't think you should put these in the header. If these don't get used > > > outside of draw_polygon.cc, then just put them in an anonymous namespace in > > the > > > .cc file like you do with coplanar_dot_epsilon. > > > > At least one, compare_threshold, needs to visible in the BspTree unit tests. > Not > > sure if we should find another way to do this, or maybe move just > > split_threshold into the .cc? > > I looked at that code, and I think you should just test SideCompare inside of > DrawPolygon unit tests (which you're already doing) and then just reuse > SideCompare in the BSP tests to verify that things got put on the side they > should. Done.
https://codereview.chromium.org/411793002/diff/60001/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/411793002/diff/60001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:36: total = total + cross_prod; How does this do with large quads? Seems like there's risk of overflow? I think that cases like star shaped polygons with the inner points very close to one another or triangles with zero height will give catastrophic cancellation here? Can you please test cases like this and show that the function does something reasonable? It would put my mind at ease :) https://codereview.chromium.org/411793002/diff/60001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:55: gfx::Vector3dF c13 = in_points[2] - in_points[0]; Couldn't these two vectors be collinear? I think they could also be the zero vector if we have a doubled point. I don't think we need to resort to PCA/eigenvectors for this, but I think we could do something a bit more clever. Perhaps we could walk around the perimeter of the polygon keeping a running sum of the cross product of adjacent edge vectors? They should all be pointing in the same direction, so summing them shouldn't hurt. I think we're always convex, too, so we shouldn't need to worry about cancellation. We could also abort the walk if the length of the summed normal exceeds some threshold. dneto pointed out that this scheme is worst if we've got a zillion points in a circle and every cross product is tiny. Perhaps we could traverse the points in a pseudo random order? wdyt? This can still fall apart when the points are all collinear or coincident. We should have code for dealing with this and tests that stress this case. https://codereview.chromium.org/411793002/diff/60001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:57: normal.Scale(1.0f / normal.Length()); Please check for div-by-zero. If |in_points| is filled with coincident points, this'll def be zero. Should have a test for this.
https://codereview.chromium.org/411793002/diff/60001/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/411793002/diff/60001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:36: total = total + cross_prod; On 2014/07/24 18:11:27, Ian Vollick wrote: > How does this do with large quads? Seems like there's risk of overflow? I think > that cases like star shaped polygons with the inner points very close to one > another or triangles with zero height will give catastrophic cancellation here? > Can you please test cases like this and show that the function does something > reasonable? It would put my mind at ease :) All of this removed.
https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:67: double dot = gfx::DotProduct(a.normal_, b.normal_); Do we need to ensure that apply normal has been called so that these normals are valid? It occurs that we only need to transform the normal once for the original quad and all the split polygons can refer to that normal. Maybe we should grab the normal off the original polygon here? https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:71: if (std::abs(dot) >= coplanar_dot_epsilon) { It's a little odd to have epsilon be .99 rather than some number close to zero. Can you make eps = 0.01f and then make your check here be against 1.0f - eps? https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:74: sign = gfx::DotProduct(a.points_[0] - b.points_[0], b.normal_); What if a.points_[0] == b.points_[0] ? https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:147: end_distance > distance_threshold)) { Dumb question, if you know start and end distance, can't you lerp the endpoints of the line segment? https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:228: while (current_intersection < 2) { A comment explaining that we can only have 2 points of intersection b/c the poly is convex would be helpful. Is the case where poly A slices through an edge and a vertex of poly B handled by the thick plane stuff in LineIntersectPlane? https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:258: for (unsigned int i = start1; i <= vertex_before[1]; i++) { Confused about this. Isn't it possible that we'd have to wrap around to get back to vertex_before[1]? Why don't we have to keep doing i = (i + 1) % size and stop when i == vertex_before[1]? https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:327: bool DrawPolygon::GetInverseTransform(gfx::Transform* transform) const { Does anyone use this?
https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:67: double dot = gfx::DotProduct(a.normal_, b.normal_); On 2014/07/25 15:52:13, Ian Vollick wrote: > Do we need to ensure that apply normal has been called so that these normals are > valid? It occurs that we only need to transform the normal once for the original > quad and all the split polygons can refer to that normal. Maybe we should grab > the normal off the original polygon here? Applying the transformation to the normal happens right before going into the BSP tree, and this is the only place the normal matters. When anything gets split, the normal is simply copied to the two split pieces, so the normals should always be valid in this case. https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:71: if (std::abs(dot) >= coplanar_dot_epsilon) { On 2014/07/25 15:52:13, Ian Vollick wrote: > It's a little odd to have epsilon be .99 rather than some number close to zero. > Can you make eps = 0.01f and then make your check here be against 1.0f - eps? Done. https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:74: sign = gfx::DotProduct(a.points_[0] - b.points_[0], b.normal_); On 2014/07/25 15:52:13, Ian Vollick wrote: > What if a.points_[0] == b.points_[0] ? We end up with the dot product resulting in 0, which is totally okay with us. This just means that the two surfaces are coplanar and they happen to share a point which is totally acceptable. These cases are handled below. https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:147: end_distance > distance_threshold)) { On 2014/07/25 15:52:13, Ian Vollick wrote: > Dumb question, if you know start and end distance, can't you lerp the endpoints > of the line segment? You're right, much simpler, done. https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:228: while (current_intersection < 2) { On 2014/07/25 15:52:13, Ian Vollick wrote: > A comment explaining that we can only have 2 points of intersection b/c the poly > is convex would be helpful. Is the case where poly A slices through an edge and > a vertex of poly B handled by the thick plane stuff in LineIntersectPlane? If I'm envisioning the case correctly, then it should be handled just fine by LineIntersectPlane. When figuring out the 2 intersection points, one of the edges will have the intersected vertex as its start point, and since that is within |distance_threshold|, then this point will be an intersection point. https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:258: for (unsigned int i = start1; i <= vertex_before[1]; i++) { On 2014/07/25 15:52:12, Ian Vollick wrote: > Confused about this. Isn't it possible that we'd have to wrap around to get back > to vertex_before[1]? Why don't we have to keep doing i = (i + 1) % size and stop > when i == vertex_before[1]? If we had to wrap around to reach vertex_before[1], we would have hit it on the first run through, and it would instead be vertex_before[0], unless I'm misunderstanding something? The worst case scenario is that the very last edge we traverse (the one from vertex n-1 to vertex 0) is our second intersecting edge, and so vertex_before[1] will be n-1. It's possible I'm still missing something or misunderstanding you though. https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:280: This is where the normals are set for the splits so we don't need to reapply the transform, if this is what you were talking about above. https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:327: bool DrawPolygon::GetInverseTransform(gfx::Transform* transform) const { On 2014/07/25 15:52:13, Ian Vollick wrote: > Does anyone use this? This is used when rendering, since we have to go back into layer space before sending to OpenGL.
https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:67: double dot = gfx::DotProduct(a.normal_, b.normal_); On 2014/07/25 20:37:47, troyhildebrandt wrote: > On 2014/07/25 15:52:13, Ian Vollick wrote: > > Do we need to ensure that apply normal has been called so that these normals > are > > valid? It occurs that we only need to transform the normal once for the > original > > quad and all the split polygons can refer to that normal. Maybe we should grab > > the normal off the original polygon here? > > Applying the transformation to the normal happens right before going into the > BSP tree, and this is the only place the normal matters. When anything gets > split, the normal is simply copied to the two split pieces, so the normals > should always be valid in this case. K. Thanks for the explanation. https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:74: sign = gfx::DotProduct(a.points_[0] - b.points_[0], b.normal_); On 2014/07/25 20:37:47, troyhildebrandt wrote: > On 2014/07/25 15:52:13, Ian Vollick wrote: > > What if a.points_[0] == b.points_[0] ? > > We end up with the dot product resulting in 0, which is totally okay with us. > This just means that the two surfaces are coplanar and they happen to share a > point which is totally acceptable. These cases are handled below. I'm not convinced that this is always ok. Two polygons that are not coplanar can share points, right? One example that comes to mind are two quads that share a line segment, opening like a book. https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:228: while (current_intersection < 2) { On 2014/07/25 20:37:47, troyhildebrandt wrote: > On 2014/07/25 15:52:13, Ian Vollick wrote: > > A comment explaining that we can only have 2 points of intersection b/c the > poly > > is convex would be helpful. Is the case where poly A slices through an edge > and > > a vertex of poly B handled by the thick plane stuff in LineIntersectPlane? > > If I'm envisioning the case correctly, then it should be handled just fine by > LineIntersectPlane. When figuring out the 2 intersection points, one of the > edges will have the intersected vertex as its start point, and since that is > within |distance_threshold|, then this point will be an intersection point. kk. https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:327: bool DrawPolygon::GetInverseTransform(gfx::Transform* transform) const { On 2014/07/25 20:37:47, troyhildebrandt wrote: > On 2014/07/25 15:52:13, Ian Vollick wrote: > > Does anyone use this? > > This is used when rendering, since we have to go back into layer space before > sending to OpenGL. Perhaps you could use it in ApplyTransformToNormal?
https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:74: sign = gfx::DotProduct(a.points_[0] - b.points_[0], b.normal_); On 2014/07/26 01:24:37, Ian Vollick - OOO until Aug 5 wrote: > On 2014/07/25 20:37:47, troyhildebrandt wrote: > > On 2014/07/25 15:52:13, Ian Vollick wrote: > > > What if a.points_[0] == b.points_[0] ? > > > > We end up with the dot product resulting in 0, which is totally okay with us. > > This just means that the two surfaces are coplanar and they happen to share a > > point which is totally acceptable. These cases are handled below. > > I'm not convinced that this is always ok. Two polygons that are not coplanar can > share points, right? One example that comes to mind are two quads that share a > line segment, opening like a book. The std::abs(dot) >= coplanar_dot_epsilon dot product check above ensures that the two surfaces have normals that are either pointing "exactly" the same or opposite directions, so if it shares one point that just means that the rest of the points on it are all in the same plane as well. https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:327: bool DrawPolygon::GetInverseTransform(gfx::Transform* transform) const { On 2014/07/26 01:24:37, Ian Vollick - OOO until Aug 5 wrote: > On 2014/07/25 20:37:47, troyhildebrandt wrote: > > On 2014/07/25 15:52:13, Ian Vollick wrote: > > > Does anyone use this? > > > > This is used when rendering, since we have to go back into layer space before > > sending to OpenGL. > > Perhaps you could use it in ApplyTransformToNormal? I just removed it instead for now.
Hey, I'm not able to do a proper review for a few hours, but I just wanted to send this note to say that my concerns have been addressed (lest I hold up the show) :) https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/411793002/diff/120001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:74: sign = gfx::DotProduct(a.points_[0] - b.points_[0], b.normal_); On 2014/07/28 17:39:37, troyhildebrandt wrote: > On 2014/07/26 01:24:37, Ian Vollick - OOO until Aug 5 wrote: > > On 2014/07/25 20:37:47, troyhildebrandt wrote: > > > On 2014/07/25 15:52:13, Ian Vollick wrote: > > > > What if a.points_[0] == b.points_[0] ? > > > > > > We end up with the dot product resulting in 0, which is totally okay with > us. > > > This just means that the two surfaces are coplanar and they happen to share > a > > > point which is totally acceptable. These cases are handled below. > > > > I'm not convinced that this is always ok. Two polygons that are not coplanar > can > > share points, right? One example that comes to mind are two quads that share a > > line segment, opening like a book. > > The std::abs(dot) >= coplanar_dot_epsilon dot product check above ensures that > the two surfaces have normals that are either pointing "exactly" the same or > opposite directions, so if it shares one point that just means that the rest of > the points on it are all in the same plane as well. Oh right (facepalm). Thanks for catching me up :)
I have a few more small things, but this seems mostly ready to go. https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:31: for (unsigned int i = 0; i < in_points.size(); i++) { unsigned int => size_t https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:166: void DrawPolygon::ApplyTransformToNormal(const gfx::Transform& transform) { I'm not sure I understand ApplyTransform vs ApplyTransformToNormal. This looks like a weird optimization and a confusing set of functions to me. Can you help me understand why there are two functions and why one gets used instead of the other? https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:200: unsigned int vertex_before[2]; unsigned int => size_t here and elsewhere in this function https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:225: if (current_intersection < 2) { Can you DCHECK here for boundary cases that shouldn't happen, like finding 1 intersection? (I think that's not right, if I'm reading this correctly?) https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:231: unsigned int start1 = (vertex_before[0] + 1) % points_size; If you mean "the size of a vector" or "an index into a vector" then use size_t, here and elsewhere in this function. https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:254: // Send false as last parameter for is_original because they're split. Comment doesn't make sense here. Add this in a future patch if you need it? https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:260: poly1->SetNormal(normal_); Should normal be a constructor parameter? https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:289: unsigned int offset = 1; unsigned int => size_t, here and elsewhere in this function https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.h File cc/quads/draw_polygon.h (right): https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.h... cc/quads/draw_polygon.h:28: bool Split(const DrawPolygon& splitter, Can you leave a comment about this function here about what this does? In particular what the boolean return value means and when front/back will be set (aka only if it returns true?) https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon_u... File cc/quads/draw_polygon_unittest.cc (right): https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon_u... cc/quads/draw_polygon_unittest.cc:113: EXPECT_EQ(front_polygon->points().size(), static_cast<unsigned int>(4)); static_cast<unsigned int>(4) => 4u, here and elsewhere
https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:19: static const float split_threshold = 0.5f; Can you leave a comment about this number too?
https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:19: static const float split_threshold = 0.5f; On 2014/07/28 21:01:13, enne wrote: > Can you leave a comment about this number too? Done. https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:31: for (unsigned int i = 0; i < in_points.size(); i++) { On 2014/07/28 20:46:40, enne wrote: > unsigned int => size_t Done. https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:166: void DrawPolygon::ApplyTransformToNormal(const gfx::Transform& transform) { On 2014/07/28 20:46:40, enne wrote: > I'm not sure I understand ApplyTransform vs ApplyTransformToNormal. This looks > like a weird optimization and a confusing set of functions to me. Can you help > me understand why there are two functions and why one gets used instead of the > other? When we transform the geometry for BSP splitting/sorting, we need both the geometry of the quad to be transformed along with its normal. Since the normal is only necessary during the splitting/sorting phase, and not when we transform back to layer space, it seems totally unnecessary and quite costly to also calculate the inverse transpose matrix and transform the normal back to its original value where it won't be used. https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:200: unsigned int vertex_before[2]; On 2014/07/28 20:46:40, enne wrote: > unsigned int => size_t here and elsewhere in this function Done. https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:225: if (current_intersection < 2) { On 2014/07/28 20:46:40, enne wrote: > Can you DCHECK here for boundary cases that shouldn't happen, like finding 1 > intersection? (I think that's not right, if I'm reading this correctly?) Done. https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:231: unsigned int start1 = (vertex_before[0] + 1) % points_size; On 2014/07/28 20:46:40, enne wrote: > If you mean "the size of a vector" or "an index into a vector" then use size_t, > here and elsewhere in this function. Done. https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:254: // Send false as last parameter for is_original because they're split. On 2014/07/28 20:46:40, enne wrote: > Comment doesn't make sense here. Add this in a future patch if you need it? Done. https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:260: poly1->SetNormal(normal_); On 2014/07/28 20:46:40, enne wrote: > Should normal be a constructor parameter? It could be. I've changed it so it is. https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:289: unsigned int offset = 1; On 2014/07/28 20:46:40, enne wrote: > unsigned int => size_t, here and elsewhere in this function Done.
https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:166: void DrawPolygon::ApplyTransformToNormal(const gfx::Transform& transform) { On 2014/07/28 21:24:55, troyhildebrandt wrote: > On 2014/07/28 20:46:40, enne wrote: > > I'm not sure I understand ApplyTransform vs ApplyTransformToNormal. This > looks > > like a weird optimization and a confusing set of functions to me. Can you > help > > me understand why there are two functions and why one gets used instead of the > > other? > > When we transform the geometry for BSP splitting/sorting, we need both the > geometry of the quad to be transformed along with its normal. Since the normal > is only necessary during the splitting/sorting phase, and not when we transform > back to layer space, it seems totally unnecessary and quite costly to also > calculate the inverse transpose matrix and transform the normal back to its > original value where it won't be used. Recapping in person discussion: change this to TransformToScreenSpace and TransformToLayerSpace. In both cases, the normal will be correct after the call. In the second case, the normal is just clobbered and set to the original. It's wasted work (but simpler) but it keeps DrawPolygon in a consistent state at all times. https://codereview.chromium.org/411793002/diff/200001/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/411793002/diff/200001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:15: // This threshold controls how "thick" a plane is. If a point's distance is What are these units in? (both compare and split thresholds?) Distance in...device pixels? https://codereview.chromium.org/411793002/diff/200001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:106: unsigned int pos_count = 0; unsigned int => int https://codereview.chromium.org/411793002/diff/200001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:251: for (unsigned int i = 0; i < points_remaining; i++) { size_t https://codereview.chromium.org/411793002/diff/200001/cc/quads/draw_polygon_u... File cc/quads/draw_polygon_unittest.cc (right): https://codereview.chromium.org/411793002/diff/200001/cc/quads/draw_polygon_u... cc/quads/draw_polygon_unittest.cc:26: for (unsigned int i = 0; i < points.size(); i++) { size_t https://codereview.chromium.org/411793002/diff/200001/cc/quads/draw_polygon_u... cc/quads/draw_polygon_unittest.cc:113: EXPECT_EQ(front_polygon->points().size(), static_cast<unsigned int>(4)); 4u, here and elsewhere :( If it's helpful, this patch should not contain "unsigned int" anywhere.
https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.h File cc/quads/draw_polygon.h (right): https://codereview.chromium.org/411793002/diff/160001/cc/quads/draw_polygon.h... cc/quads/draw_polygon.h:28: bool Split(const DrawPolygon& splitter, On 2014/07/28 20:46:40, enne wrote: > Can you leave a comment about this function here about what this does? In > particular what the boolean return value means and when front/back will be set > (aka only if it returns true?) Done. https://codereview.chromium.org/411793002/diff/200001/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/411793002/diff/200001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:15: // This threshold controls how "thick" a plane is. If a point's distance is On 2014/07/28 23:11:34, enne wrote: > What are these units in? (both compare and split thresholds?) Distance > in...device pixels? I believe they're in device pixels, if that's what the 2D layers we receive before transforming into 3D are. https://codereview.chromium.org/411793002/diff/200001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:106: unsigned int pos_count = 0; On 2014/07/28 23:11:34, enne wrote: > unsigned int => int Done. https://codereview.chromium.org/411793002/diff/200001/cc/quads/draw_polygon.c... cc/quads/draw_polygon.cc:251: for (unsigned int i = 0; i < points_remaining; i++) { On 2014/07/28 23:11:34, enne wrote: > size_t Done. https://codereview.chromium.org/411793002/diff/200001/cc/quads/draw_polygon_u... File cc/quads/draw_polygon_unittest.cc (right): https://codereview.chromium.org/411793002/diff/200001/cc/quads/draw_polygon_u... cc/quads/draw_polygon_unittest.cc:26: for (unsigned int i = 0; i < points.size(); i++) { On 2014/07/28 23:11:34, enne wrote: > size_t Done. https://codereview.chromium.org/411793002/diff/260001/cc/quads/draw_polygon_u... File cc/quads/draw_polygon_unittest.cc (right): https://codereview.chromium.org/411793002/diff/260001/cc/quads/draw_polygon_u... cc/quads/draw_polygon_unittest.cc:174: polygon_a.TransformToScreenSpace(transform); This will cause the points in polygon_a to be completely wrong for the normal, but I wanted to leave the raw ApplyTransform functions private in DrawPolygon, and TransformToScreenSpace will do the normal transformation for us. Since we're not checking the resulting points of the polygon, just the normal, I feel like this is okay but correct me if I'm wrong.
https://codereview.chromium.org/411793002/diff/260001/cc/quads/draw_polygon_u... File cc/quads/draw_polygon_unittest.cc (right): https://codereview.chromium.org/411793002/diff/260001/cc/quads/draw_polygon_u... cc/quads/draw_polygon_unittest.cc:174: polygon_a.TransformToScreenSpace(transform); On 2014/07/28 23:48:45, troyhildebrandt wrote: > This will cause the points in polygon_a to be completely wrong for the normal, > but I wanted to leave the raw ApplyTransform functions private in DrawPolygon, > and TransformToScreenSpace will do the normal transformation for us. Since we're > not checking the resulting points of the polygon, just the normal, I feel like > this is okay but correct me if I'm wrong. If the points don't matter, then maybe just make them all gfx::Point3F() with a comment that they're not used in this test?
https://codereview.chromium.org/411793002/diff/260001/cc/quads/draw_polygon_u... File cc/quads/draw_polygon_unittest.cc (right): https://codereview.chromium.org/411793002/diff/260001/cc/quads/draw_polygon_u... cc/quads/draw_polygon_unittest.cc:174: polygon_a.TransformToScreenSpace(transform); On 2014/07/29 00:05:40, enne wrote: > On 2014/07/28 23:48:45, troyhildebrandt wrote: > > This will cause the points in polygon_a to be completely wrong for the normal, > > but I wanted to leave the raw ApplyTransform functions private in DrawPolygon, > > and TransformToScreenSpace will do the normal transformation for us. Since > we're > > not checking the resulting points of the polygon, just the normal, I feel like > > this is okay but correct me if I'm wrong. > > If the points don't matter, then maybe just make them all gfx::Point3F() with a > comment that they're not used in this test? Done, simply didn't add any points at all and all it will do is transform the normal. The test still works properly regardless.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thildebr@chromium.org/411793002/320001
The CQ bit was checked by thildebr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thildebr@chromium.org/411793002/340001
The CQ bit was checked by thildebr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thildebr@chromium.org/411793002/370001
Message was sent while issue was closed.
Change committed as 286316 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
