[3/5] Add translated rasterization support for RasterBuffer & below

cc/base/scale_translate2d.h

+// Copyright (c) 2016 The Chromium Authors. All rights reserved.

[danakj, 2017/03/29 16:16:37]

2017

[trchen, 2017/03/29 22:59:56]

Acknowledged.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CC_BASE_SCALE_TRANSLATE2D_H_
+#define CC_BASE_SCALE_TRANSLATE2D_H_
+
+#include "cc/base/base_export.h"
+#include "ui/gfx/geometry/rect_f.h"
+#include "ui/gfx/geometry/vector2d_f.h"
+
+class SkCanvas;
+
+namespace cc {
+
+class CC_BASE_EXPORT ScaleTranslate2d {

[enne (OOO), 2017/03/29 13:06:13]

If you're going to add a geometry type, could you at least add some unit tests
that smoke test all the functions?

[danakj, 2017/03/29 16:16:37]

Please put a class-level comment on it explaining what it is, when it would be
used, any limitations it has etc.

I don't think that we should add geometry types to cc/base/. They should go in
ui/gfx/geometry.

[trchen, 2017/03/29 22:59:56]

Acknowledged.
Acknowledged.
Acknowledged. I was thinking the same. I put it in cc only because I wasn't sure
what people prefer. :)

+ public:
+  constexpr ScaleTranslate2d() : scale_(1.f) {}

[danakj, 2017/03/29 16:16:37]

 = default

[trchen, 2017/03/29 22:59:56]

Acknowledged.

+  constexpr ScaleTranslate2d(float scale, const gfx::Vector2dF& translation)
+      : scale_(scale), translation_(translation) {}
+
+  bool operator==(const ScaleTranslate2d& other) const {
+    return scale_ == other.scale_ && translation_ == other.translation_;
+  }
+  bool operator!=(const ScaleTranslate2d& other) const {
+    return !(*this == other);
+  }
+
+  static ScaleTranslate2d PreScale(const ScaleTranslate2d& t, float scale) {

[danakj, 2017/03/29 16:16:37]

It would be nice to make these static methods into free functions like we do for
Rect/Size/Point/Vector2d/etc..

However then this would be PreScaleScaleTranslate2d, which uh.. doesn't work.

I'm proposing to name this class AxisTransform2d (because its a transform that
keeps things axis-aligned). That would give the class a much more clearly-nouns
name, and we could have PreScaleAxisTransform2d and that'd make sense.

[trchen, 2017/03/29 22:59:56]

Acknowledged.

I will follow the existing convention. But this is one question I always wanted
to ask. Why is this style preferred over the others?

The problem with free function is that it pollutes the namespace. With type
suffix it is mitigated, but then the advantage of not having to type redundant
type name is gone, makes it no better than static class method. And what's so
wrong with member function anyway? Is it because it confuses between in-place
and out-of-place operations?
Yea I saw the discussion on chat. In mathematical notation it would be
D(2)⨯T(2), but I couldn't find a common name for it either. AxisTransform2d
sounds good enough (AxisPreservingTransform2d sounds too verbose). :/

+    return ScaleTranslate2d(t.scale_ * scale, t.translation_);
+  }
+  void PreScale(float scale) { *this = PreScale(*this, scale); }

[danakj, 2017/03/29 16:16:37]

This is maybe a nit, or maybe nicer if the statics are free functions, but I'd
prefer if these modifying methods we implemented directly, and the free/static
methods used them on a variable that they returned. I think these would read
very clearly when they are modifying their own members, and the free functions
would read more clearly by calling them than by constructing a new object and
doing the math on the arguments to the constructor.

[trchen, 2017/03/29 22:59:56]

Acknowledged.

+
+  static ScaleTranslate2d PostScale(const ScaleTranslate2d& t, float scale) {
+    return ScaleTranslate2d(t.scale_ * scale,
+                            gfx::ScaleVector2d(t.translation_, scale));
+  }
+  void PostScale(float scale) { *this = PostScale(*this, scale); }
+
+  static ScaleTranslate2d PreTranslate(const ScaleTranslate2d& t,
+                                       const gfx::Vector2dF& translation) {
+    return ScaleTranslate2d(
+        t.scale_, gfx::ScaleVector2d(translation, t.scale_) + t.translation_);
+  }
+  void PreTranslate(const gfx::Vector2dF& translation) {
+    *this = PreTranslate(*this, translation);
+  }
+
+  static ScaleTranslate2d PostTranslate(const ScaleTranslate2d& t,
+                                        const gfx::Vector2dF& translation) {
+    return ScaleTranslate2d(t.scale_, t.translation_ + translation);
+  }
+  void PostTranslate(const gfx::Vector2dF& translation) {
+    *this = PostTranslate(*this, translation);
+  }
+
+  static ScaleTranslate2d Concat(const ScaleTranslate2d& post,
+                                 const ScaleTranslate2d& pre) {
+    return ScaleTranslate2d(
+        pre.scale_ * post.scale_,
+        gfx::ScaleVector2d(pre.translation_, post.scale_) + post.translation_);
+  }
+  void PreConcat(const ScaleTranslate2d& other) {
+    *this = Concat(*this, other);
+  }
+  void PostConcat(const ScaleTranslate2d& other) {
+    *this = Concat(other, *this);
+  }
+
+  static ScaleTranslate2d Inverse(const ScaleTranslate2d& t) {
+    return ScaleTranslate2d(1 / t.scale_,
+                            gfx::ScaleVector2d(-t.translation_, 1 / t.scale_));
+  }
+  void Invert() { *this = Inverse(*this); }
+
+  gfx::PointF TransformPoint(const gfx::PointF& p) const {

[danakj, 2017/03/29 16:16:37]

I'd say everything should be s/Transform/Map/g cuz that's how we refer to things
in MathUtil

[trchen, 2017/03/29 22:59:56]

Acknowledged. YYYYES! I was following gfx::Transform, but MapPoint sounds way
better!

+    return gfx::ScalePoint(p, scale_) + translation_;
+  }
+  gfx::PointF TransformPointReverse(const gfx::PointF& p) const {
+    return gfx::ScalePoint(p - translation_, 1.f / scale_);
+  }
+
+  gfx::RectF TransformRect(const gfx::RectF& r) const {
+    return gfx::ScaleRect(r, scale_) + translation_;
+  }
+  gfx::RectF TransformRectReverse(const gfx::RectF& r) const {

[enne (OOO), 2017/03/29 13:06:13]

naming nit: Maybe InverseTransformRect, as it really just seems to be a helper
for Inverse(this).TransformRect?

[trchen, 2017/03/29 22:59:56]

Acknowledged. Was following gfx::Transform. Perhaps InverseMapRect? I personally
preferred to omit the type suffix but oh well.

+    return gfx::ScaleRect(r - translation_, 1.f / scale_);
+  }
+
+  void ApplyToCanvas(SkCanvas* canvas) const;

[danakj, 2017/03/29 16:16:37]

This seems like something that should not be part of this class. It could be a
free function somewhere else.

[trchen, 2017/03/29 22:59:56]

Agreed. Somewhere in ui/gfx? cc/base? skia/ext?

[trchen, 2017/03/30 20:56:05]

It's only called once. Unrolled & removed this helper function for now.

+
+  float scale() const { return scale_; }
+  gfx::Vector2dF translation() const { return translation_; }
+
+  std::string ToString() const { return "todo"; }
+
+ private:
+  // Scale is applied before translation, i.e.

[danakj, 2017/03/29 16:16:37]

This type of thing seems like it should be part of the class-level comment,
rather than on the private variables.

[trchen, 2017/03/29 22:59:56]

I think the opposite. At the class level all we know is that it is a D(2)⨯T(2)
group. The internal representation shouldn't be a concern of the caller. It
could well be pre-translation post-scale while keeping all behavior the same.

[trchen, 2017/03/30 20:56:05]

Anyway, also added comments at the top about pre-scale post-translate. No harm
to keep both right?

+  // this->Transform(p) == scale_ * p + translation_
+  float scale_;

[danakj, 2017/03/29 16:16:37]

= 1.f

[trchen, 2017/03/29 22:59:56]

Acknowledged.

+  gfx::Vector2dF translation_;
+};
+
+}  // namespace cc
+
+#endif  // CC_BASE_SCALE_TRANSLATE2D_H_