Don't add duplicate points when clipping

cc/base/math_util.cc

Index: cc/base/math_util.cc
diff --git a/cc/base/math_util.cc b/cc/base/math_util.cc
index d28c195971f6cd14eb753b6d9eddd51d1dd082fb..1fe96e60f75de0cc25d153a351c7b62600100336 100644
--- a/cc/base/math_util.cc
+++ b/cc/base/math_util.cc
@@ -155,9 +155,36 @@ static inline void ExpandBoundsToIncludePoint(float* xmin,
   *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)++;
 }
@@ -165,6 +192,11 @@ static inline void AddVertexToClippedQuad(const gfx::PointF& new_vertex,
 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)++;
 }
@@ -345,6 +377,14 @@ void MathUtil::MapClippedQuad(const gfx::Transform& transform,
                            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);
 }
 
@@ -406,6 +446,14 @@ bool MathUtil::MapClippedQuad3d(const gfx::Transform& transform,
                              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);
 }
@@ -935,4 +983,18 @@ ScopedSubnormalFloatDisabler::~ScopedSubnormalFloatDisabler() {
 #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