Don't add duplicate points when clipping

cc/base/math_util.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/base/math_util.h"
 
 #include <algorithm>
 #include <cmath>
 #include <limits>
 #ifdef __SSE__
@@ skipping 137 matching lines @@
                                               float* xmax,
                                               float* ymin,
                                               float* ymax,
                                               const gfx::PointF& p) {
   *xmin = std::min(p.x(), *xmin);
   *xmax = std::max(p.x(), *xmax);
   *ymin = std::min(p.y(), *ymin);
   *ymax = std::max(p.y(), *ymax);
 }
 
+static inline bool isNearlyTheSame(const float f, const float g) {

[danakj, 2017/01/11 23:12:06]

fix name style

[Peter Mayo, 2017/01/11 23:47:04]

Just the capital, or do you have a specific suggestion on a change?

[danakj, 2017/01/11 23:51:54]

just the capital.

but also consting a float parameter is a bit unusual now that i read this again.

+  // The idea behind this is to use this fraction of the larger of the
+  // tow numbers as the limit of the difference.  This breaks down near
+  // zero, so we reuse this ase the minimum asolute size we will use
+  // for the base of the scale too.

[flackr, 2017/01/11 22:48:28]

spelling nit: s/tow/two s/ase/as s/asolute/absolute

[Peter Mayo, 2017/01/11 23:47:04]

enoD

+  static const float epsilon_scale = 0.00001f;
+  return std::abs(f - g) <
+         epsilon_scale *
+             std::max(std::max(std::abs(f), std::abs(g)), epsilon_scale);
+}
+
+static inline bool isNearlyTheSame(const gfx::PointF& lhs,
+                                   const gfx::PointF& rhs) {
+  return isNearlyTheSame(lhs.x(), rhs.x()) && isNearlyTheSame(lhs.y(), rhs.y());
+}
+
+static inline bool isNearlyTheSame(const gfx::Point3F& lhs,
+                                   const gfx::Point3F& rhs) {
+  return isNearlyTheSame(lhs.x(), rhs.x()) &&
+         isNearlyTheSame(lhs.y(), rhs.y()) && isNearlyTheSame(lhs.z(), rhs.z());
+}
+
 static inline void AddVertexToClippedQuad(const gfx::PointF& new_vertex,
                                           gfx::PointF clipped_quad[8],
                                           int* num_vertices_in_clipped_quad) {
+  if (*num_vertices_in_clipped_quad > 0) {
+    if (isNearlyTheSame(clipped_quad[*num_vertices_in_clipped_quad - 1],
+                        new_vertex))
+      return;
+  }
   clipped_quad[*num_vertices_in_clipped_quad] = new_vertex;
   (*num_vertices_in_clipped_quad)++;
 }
 
 static inline void AddVertexToClippedQuad3d(const gfx::Point3F& new_vertex,
                                             gfx::Point3F clipped_quad[8],
                                             int* num_vertices_in_clipped_quad) {
+  if (*num_vertices_in_clipped_quad > 0) {
+    if (isNearlyTheSame(clipped_quad[*num_vertices_in_clipped_quad - 1],
+                        new_vertex))
+      return;
+  }
   clipped_quad[*num_vertices_in_clipped_quad] = new_vertex;
   (*num_vertices_in_clipped_quad)++;
 }
 
 gfx::Rect MathUtil::MapEnclosingClippedRect(const gfx::Transform& transform,
                                             const gfx::Rect& src_rect) {
   if (transform.IsIdentityOrIntegerTranslation()) {
     gfx::Vector2d offset(static_cast<int>(transform.matrix().getFloat(0, 3)),
                          static_cast<int>(transform.matrix().getFloat(1, 3)));
     return src_rect + offset;
@@ skipping 160 matching lines @@
   if (!h4.ShouldBeClipped()) {
     AddVertexToClippedQuad(
         h4.CartesianPoint2d(), clipped_quad, num_vertices_in_clipped_quad);
   }
 
   if (h4.ShouldBeClipped() ^ h1.ShouldBeClipped()) {
     AddVertexToClippedQuad(ComputeClippedCartesianPoint2dForEdge(h4, h1),
                            clipped_quad, num_vertices_in_clipped_quad);
   }
 
+  if (*num_vertices_in_clipped_quad > 2) {
+    if (isNearlyTheSame(clipped_quad[0],

[flackr, 2017/01/11 22:48:28]

I think canonical style is to use && rather than nest dependent conditions.

[danakj, 2017/01/11 23:12:06]

yes, this throughout.

[Peter Mayo, 2017/01/11 23:47:04]

Throughout the CL, done.

+                        clipped_quad[*num_vertices_in_clipped_quad - 1])
+    {
+      *num_vertices_in_clipped_quad -= 1;
+    }
+  }
+
   DCHECK_LE(*num_vertices_in_clipped_quad, 8);
 }
 
 bool MathUtil::MapClippedQuad3d(const gfx::Transform& transform,
                                 const gfx::QuadF& src_quad,
                                 gfx::Point3F clipped_quad[8],
                                 int* num_vertices_in_clipped_quad) {
   HomogeneousCoordinate h1 =
       MapHomogeneousPoint(transform, gfx::Point3F(src_quad.p1()));
   HomogeneousCoordinate h2 =
@@ skipping 41 matching lines @@
   if (!h4.ShouldBeClipped()) {
     AddVertexToClippedQuad3d(
         h4.CartesianPoint3d(), clipped_quad, num_vertices_in_clipped_quad);
   }
 
   if (h4.ShouldBeClipped() ^ h1.ShouldBeClipped()) {
     AddVertexToClippedQuad3d(ComputeClippedCartesianPoint3dForEdge(h4, h1),
                              clipped_quad, num_vertices_in_clipped_quad);
   }
 
+  if (*num_vertices_in_clipped_quad > 2) {
+    if (isNearlyTheSame(clipped_quad[0],
+                        clipped_quad[*num_vertices_in_clipped_quad - 1])
+    {
+      *num_vertices_in_clipped_quad -= 1;
+    }
+  }
+
   DCHECK_LE(*num_vertices_in_clipped_quad, 8);
   return (*num_vertices_in_clipped_quad >= 4);
 }
 
 gfx::RectF MathUtil::ComputeEnclosingRectOfVertices(
     const gfx::PointF vertices[],
     int num_vertices) {
   if (num_vertices < 2)
     return gfx::RectF();
 
@@ skipping 509 matching lines @@
   _mm_setcsr(orig_state_ | 0x8040);
 #endif
 }
 
 ScopedSubnormalFloatDisabler::~ScopedSubnormalFloatDisabler() {
 #ifdef __SSE__
   _mm_setcsr(orig_state_);
 #endif
 }
 
+bool IsNearlyTheSameForUnitTesting(const float left, const float right) {

[danakj, 2017/01/11 23:12:06]

ForTesting is the suffix that presubmits look for. Why are these only for
testing, would they be bad to use for other cases?

There's a bunch of epsilon float comparison stuff sprinkled around the place
should we use this more widely?

[Peter Mayo, 2017/01/11 23:47:04]

Yes, because it would stress the definitions.  Didn't know about the presubmits
looking for the name - are they doing something for them we want done here?

What I see immediately is them giving an incorrect warning:
" ** Presubmit Warnings **
" You might be calling functions intended only for testing from
" production code.  It is OK to ignore this warning if you know what
" you are doing, as the heuristics used to detect the situation are
" not perfect.  The commit queue will not block on this warning.
"   cc/base/math_util.cc:984
"     bool IsNearlyTheSameForTesting(const gfx::PointF& left, \
"   cc/base/math_util.cc:989
"     bool IsNearlyTheSameForTesting(const gfx::Point3F& left,
Near enough is a context specific concept, so sharing the values is not
necessarily a long-term benefit.  

[danakj, 2017/01/11 23:53:36]

They look for non-test code calling the function and create an error when it
happens. So you should use that syntax here instead of ForUnitTesting. Are you
saying your patch generates errors when these are named ForTesting?

+  return isNearlyTheSame(left, right);
+}
+
+bool IsNearlyTheSameForUnitTesting(const gfx::PointF& left,
+                                   const gfx::PointF& right) {
+  return isNearlyTheSame(left, right);
+}
+
+bool IsNearlyTheSameForUnitTesting(const gfx::Point3F& left,
+                                   const gfx::Point3F& right) {
+  return isNearlyTheSame(left, right);
+}
+
 }  // namespace cc