DrawPolygon class with Unit Tests

cc/quads/draw_polygon.h

+// Copyright 2014 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.
+
+#ifndef CC_QUADS_DRAW_POLYGON_H_
+#define CC_QUADS_DRAW_POLYGON_H_
+
+#include <vector>
+
+#include "cc/base/math_util.h"
+#include "cc/output/bsp_compare_result.h"
+#include "cc/quads/draw_quad.h"
+#include "ui/gfx/point3_f.h"
+#include "ui/gfx/quad_f.h"
+#include "ui/gfx/vector3d_f.h"
+
+namespace cc {
+
+class DrawPolygon {
+ public:
+  DrawPolygon();
+  ~DrawPolygon();
+
+  DrawPolygon(DrawQuad* original_ref,

[enne (OOO), 2014/07/23 21:19:32]

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".

[troyhildebrandt, 2014/07/24 00:43:21]

Done.

+              gfx::Point3F* in_points,

[enne (OOO), 2014/07/23 21:19:32]

Could this be const std::vector<gfx::Point3F>&, rather than a raw pointer and a
count?

[troyhildebrandt, 2014/07/24 00:43:21]

Done.

+              int num_vertices_in_polygon,
+              int draw_order_index = 0);
+  DrawPolygon(const DrawPolygon& other);

[enne (OOO), 2014/07/23 21:19:32]

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.

+
+  DrawPolygon& operator=(const DrawPolygon& rhs);
+
+  bool Split(const DrawPolygon& splitter,
+             scoped_ptr<DrawPolygon>* front,
+             scoped_ptr<DrawPolygon>* back);
+  float SignedPointDistance(const gfx::Point3F& point) const;
+  void ApplyTransform(const gfx::Transform& transform);

[enne (OOO), 2014/07/23 21:19:32]

Where does this function get used? Is this in GLRenderer somewhere?

[troyhildebrandt, 2014/07/24 00:43:21]

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.

+
+  std::vector<gfx::Point3F> points;

[enne (OOO), 2014/07/23 21:19:32]

Are these mutable after construction? Should they be private with public const
accessors?

[troyhildebrandt, 2014/07/24 00:43:21]

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.

[enne (OOO), 2014/07/24 01:04:42]

Yeah, but CreateCopy is a member function so has access to these.  They can
still be private with public accessors?

[troyhildebrandt, 2014/07/24 18:07:21]

You're right, I made all the member variables private.

+  gfx::Vector3dF normal;

[enne (OOO), 2014/07/23 21:19:32]

You should document with a comment if normal is normalized or not.

[troyhildebrandt, 2014/07/24 00:43:21]

Done.

+  float area;
+  int order_index;

[enne (OOO), 2014/07/23 21:19:32]

This needs a comment to explain what it is.

[troyhildebrandt, 2014/07/24 00:43:21]

Done.

+
+  // Checks polygon a against polygon b and returns which side it lies on, or
+  // whether it crosses (necessitating a split in the BSP tree).
+  static BspCompareResult SideCompare(const DrawPolygon& a,
+                                      const DrawPolygon& b);
+  void ToQuads2D(std::vector<gfx::QuadF>* quads) const;
+  int NumPoints() { return points.size(); }
+  bool GetInverseTransform(gfx::Transform* transform) const;
+
+  // The pointer to the original quad, which gives us all the drawing info
+  // we need.
+  DrawQuad* original_ref;
+
+  // This threshold controls how "thick" a plane is. If a point's distance is
+  // <= compare_threshold, then it is considered on the plane. Only when this
+  // boundary is crossed do we consider doing splitting.
+  static float compare_threshold;

[enne (OOO), 2014/07/23 21:19:32]

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.

[troyhildebrandt, 2014/07/24 00:43:21]

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?

[enne (OOO), 2014/07/24 01:04:42]

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.

[troyhildebrandt, 2014/07/24 18:07:21]

Done.

+  static float split_threshold;
+
+ private:
+  void CopyFrom(const DrawPolygon& rhs);
+};
+
+}  // namespace cc
+
+#endif  // CC_QUADS_DRAW_POLYGON_H_