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

src/core/SkLightingShader.cpp

 /*
  * Copyright 2015 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkBitmapProcShader.h"
 #include "SkBitmapProcState.h"
 #include "SkColor.h"
@@ skipping 27 matching lines @@
 
 /** \class SkLightingShaderImpl
     This subclass of shader applies lighting.
 */
 class SkLightingShaderImpl : public SkShader {
 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);
     }
 
     bool isOpaque() const override;
 
 #if SK_SUPPORT_GPU
     sk_sp<GrFragmentProcessor> asFragmentProcessor(GrContext*,
                                                    const SkMatrix& viewM,
                                                    const SkMatrix* localMatrix,
                                                    SkFilterQuality,
@@ skipping 23 matching lines @@
     SK_TO_STRING_OVERRIDE()
     SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS(SkLightingShaderImpl)
 
 protected:
     void flatten(SkWriteBuffer&) const override;
     size_t onContextSize(const ContextRec&) const override;
     Context* onCreateContext(const ContextRec&, void*) const override;
 
 private:
     SkBitmap        fDiffuseMap;
-    SkBitmap        fNormalMap;
-
     sk_sp<SkLights> fLights;
 
-    SkMatrix        fNormLocalMatrix;
-    SkVector        fInvNormRotation;
-
     sk_sp<SkNormalSource> fNormalSource;
 
     friend class SkLightingShader;
 
     typedef SkShader INHERITED;
 };
 
 ////////////////////////////////////////////////////////////////////////////
 
 #if SK_SUPPORT_GPU
@@ skipping 180 matching lines @@
 
     return true;
 }
 
 sk_sp<GrFragmentProcessor> SkLightingShaderImpl::asFragmentProcessor(
                                                      GrContext* context,
                                                      const SkMatrix& viewM,
                                                      const SkMatrix* localMatrix,
                                                      SkFilterQuality filterQuality,
                                                      SkSourceGammaTreatment gammaTreatment) const {
     // 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)) {
         return nullptr;
     }
 
     bool doBicubic;
     GrTextureParams::FilterMode diffFilterMode = GrSkFilterQualityToGrFilterMode(
                                                     SkTMin(filterQuality, kMedium_SkFilterQuality),
                                                     viewM,
@@ skipping 148 matching lines @@
 
 sk_sp<SkFlattenable> SkLightingShaderImpl::CreateProc(SkReadBuffer& buf) {
     SkMatrix diffLocalM;
     bool hasDiffLocalM = buf.readBool();
     if (hasDiffLocalM) {
         buf.readMatrix(&diffLocalM);
     } else {
         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;
 
     for (int l = 0; l < numLights; ++l) {
         bool isAmbient = buf.readBool();
 
         SkColor3f color;
         if (!buf.readScalarArray(&color.fX, 3)) {
             return nullptr;
         }
 
         if (isAmbient) {
             builder.add(SkLights::Light(color));
         } else {
             SkVector3 dir;
             if (!buf.readScalarArray(&dir.fX, 3)) {
                 return nullptr;
             }
             builder.add(SkLights::Light(color, dir));
         }
     }
 
     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) {
         const SkLights::Light& light = fLights->light(l);
 
         bool isAmbient = SkLights::Light::kAmbient_LightType == light.type();
 
         buf.writeBool(isAmbient);
         buf.writeScalarArray(&light.color().fX, 3);
         if (!isAmbient) {
             buf.writeScalarArray(&light.dir().fX, 3);
         }
     }
-    buf.writePoint(fInvNormRotation);
 
     buf.writeFlattenable(fNormalSource.get());
 }
 
 size_t SkLightingShaderImpl::onContextSize(const ContextRec& rec) const {
     return sizeof(LightingShaderContext) +
            sizeof(SkBitmapProcState)     +
            fNormalSource->providerSize(rec);
 }
 
@@ skipping 22 matching lines @@
     if (!normalProvider) {
         diffuseState->~SkBitmapProcState();
         return nullptr;
     }
 
     return new (storage) LightingShaderContext(*this, rec, diffuseState, normalProvider);
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 
-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);
+    if (!normalSource) {
+        // TODO: Use a default implementation of normalSource instead
+        return nullptr;
+    }
 
-    sk_sp<SkNormalSource> normalSource = SkNormalSource::MakeFromNormalMap(mapShader,
-                                                                           invNormRotation);
-
-    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));
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 
 SK_DEFINE_FLATTENABLE_REGISTRAR_GROUP_START(SkLightingShader)
     SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkLightingShaderImpl)
 SK_DEFINE_FLATTENABLE_REGISTRAR_GROUP_END
 
 ///////////////////////////////////////////////////////////////////////////////