SkValue: SkXfermode

src/effects/SkToFromValue.cpp

+/*
+ * Copyright 2016 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "SkArithmeticMode.h"
+#include "SkLerpXfermode.h"
+#include "SkMatrix.h"
+#include "SkPixelXorXfermode.h"
+#include "SkToFromValue.h"
+#include "SkValueKeys.h"
+#include "SkXfermode.h"
+
+////////////////////////////////////////////////////////////////////////////////
+
+#define REQUIRE(cond) do { if (!(cond)) { SkASSERT(false); return false; } } while (false)
+
+#define OPTIONAL_KEY(VAL, KEY, PTR)        \

[mtklein, 2016/01/21 19:01:23]

How far do you think we can get without having more macros than REQUIRE?

template <typename T>
bool getT(const SkValue& obj, SkValue::Key key, T* ptr) {
    auto v = obj.get(key);
    return v ? SkFromValue(*v, ptr) : false;
}

I think that makes the equivalent of OPTIONAL_KEY just
    int32_t i = 0;
    getT(obj, key, &i);
and the equivalent of MANDATORY_KEY just
    int32_t i;
    REQUIRE(getT(obj, key, &i));

Seem right?

[hal.canary, 2016/01/21 19:33:40]

almost.  OPTIONAL_KEY should still return false if the key type is a mismatch.

Is what I changed it to better or worse?

[mtklein, 2016/01/21 20:01:33]

Why?  We have a default value already, so it's not super-fatal if we find the
wrong type there.  
   int32_t i = 0;
   getT(obj, key, &i); 
would assert in Debug mode (catches our bugs) and carry on with i = 0 without
error in Release mode (safe) if the key doesn't point to an S32.

+    do {                                   \
+        if (auto v = (VAL).get(KEY)) {     \
+            REQUIRE(SkFromValue(*v, PTR)); \
+        }                                  \
+    } while (false)
+
+#define MANDATORY_KEY(VAL, KEY, PTR)        \
+    do {                                    \
+        auto v = (VAL).get(KEY);            \
+        REQUIRE(v && SkFromValue(*v, PTR)); \
+    } while (false)
+
+template<> bool SkFromValue<float>(const SkValue& val, float* f) {
+    REQUIRE(val.type() == SkValue::F32);
+    *f = val.f32();
+    return true;
+}
+
+template<> bool SkFromValue<int32_t>(const SkValue& val, int32_t* x) {
+    REQUIRE(val.type() == SkValue::S32);
+    *x = val.s32();
+    return true;
+}
+
+template<> bool SkFromValue<uint32_t>(const SkValue& val, uint32_t* x) {
+    REQUIRE(val.type() == SkValue::U32);
+    *x = val.u32();
+    return true;
+}
+
+////////////////////////////////////////////////////////////////////////////////
+
+template<> SkValue SkToValue<SkMatrix>(const SkMatrix& mat) {
+    auto val = SkValue::Object(SkValue::Matrix);
+    for (int i = 0; i < 9; ++i) {
+        if (mat[i] != SkMatrix::I()[i]) {
+            val.set(i, SkValue::FromF32(mat[i]));
+        }
+    }
+    return val;
+}
+
+template<> bool SkFromValue<SkMatrix>(const SkValue& val, SkMatrix* m){
+    REQUIRE(val.type() == SkValue::Matrix);
+    *m = SkMatrix::I();
+    for (int i = 0; i < 9; i++) {
+        OPTIONAL_KEY(val, i, &(*m)[i]);
+    }
+    return true;
+}
+
+////////////////////////////////////////////////////////////////////////////////
+
+template<> SkValue SkToValue<SkXfermode>(const SkXfermode* x) {
+    return x ? x->asValue() : SkValue::Object(SkValue::DefaultXfermode);
+}
+
+static bool defaultxfermode_from_value(const SkValue& val,

[mtklein, 2016/01/21 19:01:23]

I'd slightly prefer from_value_DefaultXfermode, etc, so these look static but
also look exactly like the Type / type they're working with.

[hal.canary, 2016/01/21 19:33:40]

done.

+                                       SkAutoTUnref<SkXfermode>* dst) {
+    dst->reset(nullptr);
+    return true;
+}
+
+static bool arithmeticxfermode_from_value(const SkValue& val,
+                                          SkAutoTUnref<SkXfermode>* dst) {
+    float k[4];
+    int32_t enforce = true;

[mtklein, 2016/01/21 19:15:31]

One of my goals in reducing the number of macros we have is to make us feel good
about writing things like this field naturally the way we want them expressed,
rather than how the macros make us do it.  E.g.

bool enforcePMColor = true;
if (auto v = val.get(kEnforcePMColor)) {
    enforcePMColor = SkToBool(v->s32());
}

some more fewer-macro ideas below:

+    using namespace SkValueKeys::ArithmeticXfermode;
+    MANDATORY_KEY(val, kK0, &k[0]);
+    MANDATORY_KEY(val, kK1, &k[1]);
+    MANDATORY_KEY(val, kK2, &k[2]);
+    MANDATORY_KEY(val, kK3, &k[3]);
+    OPTIONAL_KEY(val, kEnforcePMColor, &enforce);
+    dst->reset(SkArithmeticMode::Create(
+                       k[0], k[1], k[2], k[3], SkToBool(enforce)));
+    return true;
+}
+
+static bool lerpxfermode_from_value(const SkValue& val,
+                                    SkAutoTUnref<SkXfermode>* dst) {
+    float scale = 0;
+    MANDATORY_KEY(val, SkValueKeys::LerpXfermode::kScale, &scale);
+    dst->reset(SkLerpXfermode::Create(scale));
+    return true;
+}
+
+static bool pixelxorxfermode_from_value(const SkValue& val,
+                                        SkAutoTUnref<SkXfermode>* dst) {
+    uint32_t opColor = 0;

[mtklein, 2016/01/21 19:15:31]

Same deal here:

uint32_t color;
REQUIRE(getT(val, kOpColor, &color));
dst->reset(SkPixelXorXfermode::Create(color));
return true;

OR

auto color = val.get(kOpColor);
REQUIRE(color);
dst->reset(SkPixelXorXfermode::Create(color->u32()));
return true;

+    MANDATORY_KEY(val, SkValueKeys::PixelXorXfermode::kOpColor, &opColor);
+    dst->reset(SkPixelXorXfermode::Create(opColor));
+    return true;
+}
+
+static bool proccoeffxfermode_from_value(const SkValue& val,
+                                         SkAutoTUnref<SkXfermode>* dst) {
+    uint32_t mode = 0;

[mtklein, 2016/01/21 19:15:31]

Same deal here:

auto mode = val.get(kMode);
REQUIRE(mode);
dst->reset(SkXfermode::Create((SkXfermode::Mode)mode->u32()));
return true;

+    MANDATORY_KEY(val, SkValueKeys::ProcCoeffXfermode::kMode, &mode);
+    dst->reset(SkXfermode::Create((SkXfermode::Mode)mode));
+    return true;
+}
+
+template<> bool SkFromValue< SkAutoTUnref<SkXfermode> >(
+        const SkValue& val, SkAutoTUnref<SkXfermode>* dst) {
+    switch (val.type()) {
+        case SkValue::DefaultXfermode:
+            return defaultxfermode_from_value(val, dst);

[mtklein, 2016/01/21 19:01:23]

This would be easier to read if they were each one line.  Does it run over 100
columns?  If so, can we shorten names so it doesn't?

[hal.canary, 2016/01/21 19:33:40]

done.

+        case SkValue::ArithmeticXfermode:
+            return arithmeticxfermode_from_value(val, dst);
+        case SkValue::LerpXfermode:
+            return lerpxfermode_from_value(val, dst);
+        case SkValue::PixelXorXfermode:
+            return pixelxorxfermode_from_value(val, dst);
+        case SkValue::ProcCoeffXfermode:
+            return proccoeffxfermode_from_value(val, dst);
+        default:
+            REQUIRE(false);
+    }
+}
+
+////////////////////////////////////////////////////////////////////////////////
+
+#undef REQUIRE
+#undef OPTIONAL_KEY
+#undef MANDATORY_KEY
+