API change to allow for NormalSource selection at the user level.

src/core/SkNormalSource.cpp

Index: src/core/SkNormalSource.cpp
diff --git a/src/core/SkNormalSource.cpp b/src/core/SkNormalSource.cpp
index 2f525303828229b32704ca1635311fd9610a0592..55781baae10e150f225d1af2dd4bb22d25c7e44f 100644
--- a/src/core/SkNormalSource.cpp
+++ b/src/core/SkNormalSource.cpp
@@ -8,6 +8,7 @@
 #include "SkError.h"
 #include "SkErrorInternals.h"
 #include "SkLightingShader.h"
+#include "SkMatrix.h"
 #include "SkNormalSource.h"
 #include "SkReadBuffer.h"
 #include "SkWriteBuffer.h"
@@ -19,9 +20,9 @@ SkNormalSource::~SkNormalSource() {}
 
 class NormalMapSourceImpl : public SkNormalSource {
 public:
-    NormalMapSourceImpl(sk_sp<SkShader> mapShader, const SkVector &normRotation)
+    NormalMapSourceImpl(sk_sp<SkShader> mapShader, const SkMatrix& invCTM)
         : fMapShader(std::move(mapShader))
-        , fNormRotation(normRotation) {}
+        , fInvCTM(invCTM) {}
 
 #if SK_SUPPORT_GPU
     sk_sp<GrFragmentProcessor> asFragmentProcessor(GrContext*,
@@ -58,7 +59,7 @@ private:
     };
 
     sk_sp<SkShader> fMapShader;
-    SkVector        fNormRotation;
+    SkMatrix        fInvCTM; // Inverse of the canvas total matrix, used for rotating normals.
 
     friend class SkNormalSource;
 
@@ -78,8 +79,8 @@ private:
 
 class NormalMapFP : public GrFragmentProcessor {
 public:
-    NormalMapFP(sk_sp<GrFragmentProcessor> mapFP, const SkVector& normRotation)
-        : fNormRotation(normRotation) {
+    NormalMapFP(sk_sp<GrFragmentProcessor> mapFP, const SkMatrix& invCTM)
+        : fInvCTM(invCTM) {
         this->registerChildProcessor(mapFP);
 
         this->initClassID<NormalMapFP>();
@@ -88,33 +89,45 @@ public:
     class GLSLNormalMapFP : public GrGLSLFragmentProcessor {
     public:
         GLSLNormalMapFP() {
-            fNormRotation.set(0.0f, 0.0f);
+            fInvCTM.reset();
         }
 
         void emitCode(EmitArgs& args) override {
-
             GrGLSLFragmentBuilder* fragBuilder = args.fFragBuilder;
             GrGLSLUniformHandler* uniformHandler = args.fUniformHandler;
 
             // add uniform
             const char* xformUniName = nullptr;

[robertphillips, 2016/07/06 16:36:33]

It looks like we only use the upper left 2x2 - why upload the entire thing ?

[dvonbeck, 2016/07/06 18:07:48]

Done.

-            fXformUni = uniformHandler->addUniform(kFragment_GrShaderFlag,
-                                                   kVec2f_GrSLType, kDefault_GrSLPrecision,
-                                                   "Xform", &xformUniName);
+            fXformUni = uniformHandler->addUniform(kFragment_GrShaderFlag, kMat33f_GrSLType,
+                                                   kDefault_GrSLPrecision, "Xform", &xformUniName);
 
             SkString dstNormalColorName("dstNormalColor");
             this->emitChild(0, nullptr, &dstNormalColorName, args);
-            fragBuilder->codeAppendf("vec3 normal = %s.rgb - vec3(0.5);",
+            fragBuilder->codeAppendf("vec3 normal = normalize(%s.rgb - vec3(0.5));",
                                      dstNormalColorName.c_str());
 
             // TODO: inverse map the light direction vectors in the vertex shader rather than
             // transforming all the normals here!
-            fragBuilder->codeAppendf(
-                    "mat3 m = mat3(%s.x, -%s.y, 0.0, %s.y, %s.x, 0.0, 0.0, 0.0, 1.0);",
-                    xformUniName, xformUniName, xformUniName, xformUniName);
 

[robertphillips, 2016/07/06 16:36:34]

A branch in the shader is expensive !

[dvonbeck, 2016/07/06 18:07:48]

The code seemed to run just fine ignoring the division by 0 problem, at least on
my computer, even when the normals were all (0, 0, 1). Should I get rid of the
branching, and then see when triaging if it causes problems in other types of
hardware?

[robertphillips, 2016/07/06 19:11:14]

I would say leave the guard for now. I just wanted you to be aware of the perf
cost.

[dvonbeck, 2016/07/06 19:34:36]

Alright! I think it should be fine here considering the size of the code blocks.

-            fragBuilder->codeAppend("normal = normalize(m*normal);");
-            fragBuilder->codeAppendf("%s = vec4(normal, 0);", args.fOutputColor);
+            // If there's no x & y components, return (0, 0, +/- 1) instead to avoid division by 0
+            fragBuilder->codeAppend( "if (abs(normal.z) > 0.9999) {");

[robertphillips, 2016/07/06 16:36:33]

Might as well just set it to (0, 0, 1, 0).

[dvonbeck, 2016/07/06 18:07:48]

Needs to handle (0, 0, -1, 0) as well. I could set it to normalize(normal) if
that makes more sense.

[robertphillips, 2016/07/06 19:11:14]

Okay - leave it as is.

+            fragBuilder->codeAppendf("    %s = normalize(vec4(0.0, 0.0, normal.z, 0.0));",
+                    args.fOutputColor);
+            // Else, Normalizing the transformed X and Y, while keeping constant both Z and the
+            // vector's angle in the XY plane. This maintains the "slope" for the surface while
+            // appropriately rotating the normal for any anisotropic scaling that occurs.
+            // Here, we call scaling factor the number that must divide the transformed X and Y so
+            // that the normal's length remains equal to 1.
+            fragBuilder->codeAppend( "} else {");
+            fragBuilder->codeAppendf("    vec2 transformed = mat2(%s) * normal.xy;",
+                    xformUniName);
+            fragBuilder->codeAppend( "    float scalingFactorSquared = "

[robertphillips, 2016/07/06 16:36:34]

Using pows for squares seems a bit overkill.

[dvonbeck, 2016/07/06 18:07:48]

Should I be using *?

[robertphillips, 2016/07/06 19:11:14]

Right, I would just have multiplies. The compiler should convert the pow(*, 2)
calls to multiplies for you but, who knows.

[dvonbeck, 2016/07/06 19:34:36]

Done.

+                                                 "(pow(transformed.x, 2) + pow(transformed.y, 2))"
+                                                 "/(1.0 - pow(normal.z, 2));");
+            fragBuilder->codeAppendf("    %s = vec4(transformed*inversesqrt(scalingFactorSquared),"
+                                                   "normal.z, 0.0);",
+                    args.fOutputColor);
+            fragBuilder->codeAppend( "}");
         }
 
         static void GenKey(const GrProcessor& proc, const GrGLSLCaps&,
@@ -126,15 +139,13 @@ public:
         void onSetData(const GrGLSLProgramDataManager& pdman, const GrProcessor& proc) override {
             const NormalMapFP& normalMapFP = proc.cast<NormalMapFP>();
 
-            const SkVector& normRotation = normalMapFP.normRotation();
-            if (normRotation != fNormRotation) {
-                pdman.set2fv(fXformUni, 1, &normRotation.fX);
-                fNormRotation = normRotation;
-            }
+            const SkMatrix& invCTM = normalMapFP.invCTM();
+            fInvCTM = invCTM;
+            pdman.setSkMatrix(fXformUni, fInvCTM);
         }
 
     private:
-        SkVector fNormRotation;
+        SkMatrix fInvCTM;
         GrGLSLProgramDataManager::UniformHandle fXformUni;
     };
 
@@ -148,17 +159,17 @@ public:
         inout->setToUnknown(GrInvariantOutput::ReadInput::kWillNot_ReadInput);
     }
 
-    const SkVector& normRotation() const { return fNormRotation; }
+    const SkMatrix& invCTM() const { return fInvCTM; }
 
 private:
     GrGLSLFragmentProcessor* onCreateGLSLInstance() const override { return new GLSLNormalMapFP; }
 
     bool onIsEqual(const GrFragmentProcessor& proc) const override {
         const NormalMapFP& normalMapFP = proc.cast<NormalMapFP>();
-        return fNormRotation == normalMapFP.fNormRotation;
+        return fInvCTM == normalMapFP.fInvCTM;
     }
 
-    SkVector fNormRotation;
+    SkMatrix fInvCTM;
 };
 
 sk_sp<GrFragmentProcessor> NormalMapSourceImpl::asFragmentProcessor(
@@ -171,7 +182,7 @@ sk_sp<GrFragmentProcessor> NormalMapSourceImpl::asFragmentProcessor(
     sk_sp<GrFragmentProcessor> mapFP = fMapShader->asFragmentProcessor(context, viewM,
             localMatrix, filterQuality, gammaTreatment);
 
-    return sk_make_sp<NormalMapFP>(std::move(mapFP), fNormRotation);
+    return sk_make_sp<NormalMapFP>(std::move(mapFP), fInvCTM);
 }
 
 #endif // SK_SUPPORT_GPU
@@ -239,11 +250,28 @@ void NormalMapSourceImpl::Provider::fillScanLine(int x, int y, SkPoint3 output[]
                          SkIntToScalar(SkGetPackedB32(tmpNormalColors[i])) - 127.0f);
             tempNorm.normalize();
 

[robertphillips, 2016/07/06 16:36:33]

There 3 overlength lines here.

[dvonbeck, 2016/07/06 18:07:48]

Done.

-            output[i].fX = fSource.fNormRotation.fX * tempNorm.fX +
-                           fSource.fNormRotation.fY * tempNorm.fY;
-            output[i].fY = -fSource.fNormRotation.fY * tempNorm.fX +
-                           fSource.fNormRotation.fX * tempNorm.fY;
-            output[i].fZ = tempNorm.fZ;
+            if (!SkScalarNearlyEqual(SkScalarAbs(tempNorm.fZ), 1.0f)) {
+                SkVector transformed = fSource.fInvCTM.mapVector(tempNorm.fX, tempNorm.fY);
+
+                // Normalizing the transformed X and Y, while keeping constant both Z and the vector's
+                // angle in the XY plane. This maintains the "slope" for the surface while appropriately
+                // rotating the normal for any anisotropic scaling that occurs.
+                // Here, we call scaling factor the number that must divide the transformed X and Y so
+                // that the normal's length remains equal to 1.
+                SkScalar scalingFactorSquared =
+                        (SkScalarSquare(transformed.fX) + SkScalarSquare(transformed.fY))
+                        / (1.0f - SkScalarSquare(tempNorm.fZ));
+                SkScalar invScalingFactor = SkScalarInvert(SkScalarSqrt(scalingFactorSquared));
+
+                output[i].fX = transformed.fX * invScalingFactor;
+                output[i].fY = transformed.fY * invScalingFactor;
+                output[i].fZ = tempNorm.fZ;
+            } else {

[robertphillips, 2016/07/06 16:36:34]

Might as well just set this to (0, 0, 1) and skip the normalization.

[dvonbeck, 2016/07/06 18:07:48]

Needs to handle (0, 0, -1) as well. I could set it to tempNorm if that makes
more sense.

[robertphillips, 2016/07/06 19:11:14]

Nope, just leave it as is.

+                output[i] = {0.0f, 0.0f, tempNorm.fZ};
+                output[i].normalize();
+            }
+
+            SkASSERT(SkScalarNearlyEqual(output[i].length(), 1.0f))
         }
 
         output += n;
@@ -258,31 +286,29 @@ sk_sp<SkFlattenable> NormalMapSourceImpl::CreateProc(SkReadBuffer& buf) {
 
     sk_sp<SkShader> mapShader = buf.readFlattenable<SkShader>();
 
-    SkVector normRotation = {1,0};
-    if (!buf.isVersionLT(SkReadBuffer::kLightingShaderWritesInvNormRotation)) {
-        normRotation = buf.readPoint();
-    }
+    SkMatrix invCTM;
+    buf.readMatrix(&invCTM);
 
-    return sk_make_sp<NormalMapSourceImpl>(std::move(mapShader), normRotation);
+    return sk_make_sp<NormalMapSourceImpl>(std::move(mapShader), invCTM);
 }
 
 void NormalMapSourceImpl::flatten(SkWriteBuffer& buf) const {
     this->INHERITED::flatten(buf);
 
     buf.writeFlattenable(fMapShader.get());
-    buf.writePoint(fNormRotation);
+    buf.writeMatrix(fInvCTM);
 }
 
 ////////////////////////////////////////////////////////////////////////////
 
-sk_sp<SkNormalSource> SkNormalSource::MakeFromNormalMap(sk_sp<SkShader> map,
-                                                        const SkVector &normRotation) {
-    SkASSERT(SkScalarNearlyEqual(normRotation.lengthSqd(), SK_Scalar1));
-    if (!map) {
+sk_sp<SkNormalSource> SkNormalSource::MakeFromNormalMap(sk_sp<SkShader> map, const SkMatrix& ctm) {
+    SkMatrix invCTM;
+
+    if (!ctm.invert(&invCTM) || !map) {
         return nullptr;
     }
 
-    return sk_make_sp<NormalMapSourceImpl>(std::move(map), normRotation);
+    return sk_make_sp<NormalMapSourceImpl>(std::move(map), invCTM);
 }
 
 ////////////////////////////////////////////////////////////////////////////