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

src/core/SkLightingShader.cpp

Index: src/core/SkLightingShader.cpp
diff --git a/src/core/SkLightingShader.cpp b/src/core/SkLightingShader.cpp
index 52b208f8df6f2ff848513f8ea1cedc246c7c7bea..303c4648546809902274c9946a0403686f2c6c23 100644
--- a/src/core/SkLightingShader.cpp
+++ b/src/core/SkLightingShader.cpp
@@ -45,31 +45,17 @@ public:
     /** Create a new lighting shader that uses the provided normal map and
         lights to light the diffuse bitmap.
         @param diffuse           the diffuse bitmap
-        @param normal            the normal map
         @param lights            the lights applied to the normal map
-        @param invNormRotation   rotation applied to the normal map's normals
         @param diffLocalM        the local matrix for the diffuse coordinates
-        @param normLocalM        the local matrix for the normal coordinates
-        @param normalSource      the normal source for GPU computations
+        @param normalSource      the source of normals for lighting computation
     */
-    SkLightingShaderImpl(const SkBitmap& diffuse, const SkBitmap& normal,
+    SkLightingShaderImpl(const SkBitmap& diffuse,
                          const sk_sp<SkLights> lights,
-                         const SkVector& invNormRotation,
-                         const SkMatrix* diffLocalM, const SkMatrix* normLocalM,
+                         const SkMatrix* diffLocalM,
                          sk_sp<SkNormalSource> normalSource)
         : INHERITED(diffLocalM)
         , fDiffuseMap(diffuse)
-        , fNormalMap(normal)
-        , fLights(std::move(lights))
-        , fInvNormRotation(invNormRotation) {
-
-        if (normLocalM) {
-            fNormLocalMatrix = *normLocalM;
-        } else {
-            fNormLocalMatrix.reset();
-        }
-        // Pre-cache so future calls to fNormLocalMatrix.getType() are threadsafe.
-        (void)fNormLocalMatrix.getType();
+        , fLights(std::move(lights)) {
 

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

Can we not do this in the ctor list with the others ?

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

Done. I seem to have realized it in a future CL and changed it there, but got
left as-is here by accident.

         fNormalSource = std::move(normalSource);
     }
@@ -114,13 +100,8 @@ protected:
 
 private:
     SkBitmap        fDiffuseMap;
-    SkBitmap        fNormalMap;
-
     sk_sp<SkLights> fLights;
 
-    SkMatrix        fNormLocalMatrix;
-    SkVector        fInvNormRotation;
-
     sk_sp<SkNormalSource> fNormalSource;
 
     friend class SkLightingShader;
@@ -324,8 +305,6 @@ sk_sp<GrFragmentProcessor> SkLightingShaderImpl::asFragmentProcessor(
     // we assume diffuse and normal maps have same width and height

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

Do we test different sized diffuse maps & light maps ?
If not, we should but in a different CL.

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

This assumption disappears once both normals and diffuse colors are sourced from
arbitrary shaders. I agree, we should add a GM or something.

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

Do you have a task list (or something) to add it to?

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

Yes, I added it

     // TODO: support different sizes, will be addressed when diffuse maps are factored out of
     //       SkLightingShader in a future CL
-    SkASSERT(fDiffuseMap.width() == fNormalMap.width() &&
-             fDiffuseMap.height() == fNormalMap.height());
     SkMatrix diffM;
 
     if (!make_mat(fDiffuseMap, this->getLocalMatrix(), localMatrix, &diffM)) {
@@ -494,26 +473,12 @@ sk_sp<SkFlattenable> SkLightingShaderImpl::CreateProc(SkReadBuffer& buf) {
         diffLocalM.reset();
     }
 
-    SkMatrix normLocalM;
-    bool hasNormLocalM = buf.readBool();
-    if (hasNormLocalM) {
-        buf.readMatrix(&normLocalM);
-    } else {
-        normLocalM.reset();
-    }
-
     SkBitmap diffuse;
     if (!buf.readBitmap(&diffuse)) {
         return nullptr;
     }
     diffuse.setImmutable();
 
-    SkBitmap normal;
-    if (!buf.readBitmap(&normal)) {
-        return nullptr;
-    }
-    normal.setImmutable();
-
     int numLights = buf.readInt();
 
     SkLights::Builder builder;
@@ -539,28 +504,16 @@ sk_sp<SkFlattenable> SkLightingShaderImpl::CreateProc(SkReadBuffer& buf) {
 
     sk_sp<SkLights> lights(builder.finish());
 
-    SkVector invNormRotation = {1,0};
-    if (!buf.isVersionLT(SkReadBuffer::kLightingShaderWritesInvNormRotation)) {
-        invNormRotation = buf.readPoint();
-    }
-
     sk_sp<SkNormalSource> normalSource(buf.readFlattenable<SkNormalSource>());
 
-    return sk_make_sp<SkLightingShaderImpl>(diffuse, normal, std::move(lights), invNormRotation,
-                                            &diffLocalM, &normLocalM, std::move(normalSource));
+    return sk_make_sp<SkLightingShaderImpl>(diffuse, std::move(lights), &diffLocalM,
+                                            std::move(normalSource));
 }
 
 void SkLightingShaderImpl::flatten(SkWriteBuffer& buf) const {
     this->INHERITED::flatten(buf);
 
-    bool hasNormLocalM = !fNormLocalMatrix.isIdentity();
-    buf.writeBool(hasNormLocalM);
-    if (hasNormLocalM) {
-        buf.writeMatrix(fNormLocalMatrix);
-    }
-
     buf.writeBitmap(fDiffuseMap);
-    buf.writeBitmap(fNormalMap);
 
     buf.writeInt(fLights->numLights());
     for (int l = 0; l < fLights->numLights(); ++l) {
@@ -574,7 +527,6 @@ void SkLightingShaderImpl::flatten(SkWriteBuffer& buf) const {
             buf.writeScalarArray(&light.dir().fX, 3);
         }
     }
-    buf.writePoint(fInvNormRotation);
 
     buf.writeFlattenable(fNormalSource.get());
 }
@@ -617,27 +569,19 @@ SkShader::Context* SkLightingShaderImpl::onCreateContext(const ContextRec& rec,
 
 ///////////////////////////////////////////////////////////////////////////////
 
-sk_sp<SkShader> SkLightingShader::Make(const SkBitmap& diffuse, const SkBitmap& normal,
-                                       sk_sp<SkLights> lights,
-                                       const SkVector& invNormRotation,
-                                       const SkMatrix* diffLocalM, const SkMatrix* normLocalM) {
-    if (diffuse.isNull() || SkBitmapProcShader::BitmapIsTooBig(diffuse) ||
-        normal.isNull()  || SkBitmapProcShader::BitmapIsTooBig(normal) ||
-        diffuse.width()  != normal.width() ||
-        diffuse.height() != normal.height()) {
+sk_sp<SkShader> SkLightingShader::Make(const SkBitmap& diffuse, sk_sp<SkLights> lights,

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

tab the following line over to match up with the '('

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

If I do, then the line is over 100 columns. Is this still not according to the
preferred style? What's the preferred style here? Should I have each argument in
its own line?

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

Right, tab it over and add newlines as necessary.

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

Done.

+            const SkMatrix* diffLocalM, sk_sp<SkNormalSource> normalSource) {
+    if (diffuse.isNull() || SkBitmapProcShader::BitmapIsTooBig(diffuse)) {
         return nullptr;
     }
-    SkASSERT(SkScalarNearlyEqual(invNormRotation.lengthSqd(), SK_Scalar1));
 
-    // TODO: support other tile modes
-    sk_sp<SkShader> mapShader = SkMakeBitmapShader(normal, SkShader::kClamp_TileMode,
-                                                   SkShader::kClamp_TileMode, normLocalM, nullptr);
-
-    sk_sp<SkNormalSource> normalSource = SkNormalSource::MakeFromNormalMap(mapShader,
-                                                                           invNormRotation);
+    if (!normalSource) {
+        // TODO: Use a default implementation of normalSource instead
+        return nullptr;
+    }
 
-    return sk_make_sp<SkLightingShaderImpl>(diffuse, normal, std::move(lights),
-            invNormRotation, diffLocalM, normLocalM, std::move(normalSource));
+    return sk_make_sp<SkLightingShaderImpl>(diffuse, std::move(lights), diffLocalM,
+                                            std::move(normalSource));
 }
 
 ///////////////////////////////////////////////////////////////////////////////