src/core/SkLightingShader.cpp - Issue 2043393002: Refactoring of GPU NormalMap handling out into its own class

Unified Diff: src/core/SkLightingShader.cpp

Issue 2043393002: Refactoring of GPU NormalMap handling out into its own class (Closed) Base URL: https://skia.googlesource.com/skia@master

Patch Set: Rebased, unified headers, NormalSource is now serialized, addressed patch 1 comments Created 4 years, 6 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

Index: src/core/SkLightingShader.cpp

diff --git a/src/core/SkLightingShader.cpp b/src/core/SkLightingShader.cpp

index a2ce52fe28764b2750546faf09ae96194cbc2a85..96a4850dfd0d98eb0dcf9b65a6b3914e725fefcc 100644

--- a/src/core/SkLightingShader.cpp

+++ b/src/core/SkLightingShader.cpp

@@ -52,7 +52,8 @@ public:

SkLightingShaderImpl(const SkBitmap& diffuse, const SkBitmap& normal,

const sk_sp<SkLights> lights,

const SkVector& invNormRotation,

- const SkMatrix* diffLocalM, const SkMatrix* normLocalM)

+ const SkMatrix* diffLocalM, const SkMatrix* normLocalM,

+ sk_sp<const SkLightingShader::NormalSource> normalSource)

egdaniel 2016/06/10 14:23:28 putting a const inside of an sk_sp is generally a

dvonbeck 2016/06/10 15:22:29 Done.

: INHERITED(diffLocalM)

, fDiffuseMap(diffuse)

, fNormalMap(normal)

@@ -67,6 +68,7 @@ public:

// Pre-cache so future calls to fNormLocalMatrix.getType() are threadsafe.

(void)fNormLocalMatrix.getType();

+ fNormalSource = normalSource;

egdaniel 2016/06/10 14:23:28 use std::move(

dvonbeck 2016/06/10 15:22:29 Done.

}

bool isOpaque() const override;

@@ -117,6 +119,8 @@ private:

SkMatrix fNormLocalMatrix;

SkVector fInvNormRotation;

+ sk_sp<const SkLightingShader::NormalSource> fNormalSource;

egdaniel 2016/06/10 14:23:27 again const sk_sp is bad.

dvonbeck 2016/06/10 15:22:29 Done.

friend class SkLightingShader;

typedef SkShader INHERITED;

@@ -139,19 +143,12 @@ private:

class LightingFP : public GrFragmentProcessor {

public:

- LightingFP(GrTexture* diffuse, GrTexture* normal, const SkMatrix& diffMatrix,

- const SkMatrix& normMatrix, const GrTextureParams& diffParams,

- const GrTextureParams& normParams, sk_sp<SkLights> lights,

- const SkVector& invNormRotation)

+ LightingFP(GrTexture* diffuse, const SkMatrix& diffMatrix, const GrTextureParams& diffParams,

+ sk_sp<SkLights> lights, sk_sp<GrFragmentProcessor> normalFP)

: fDiffDeviceTransform(kLocal_GrCoordSet, diffMatrix, diffuse, diffParams.filterMode())

- , fNormDeviceTransform(kLocal_GrCoordSet, normMatrix, normal, normParams.filterMode())

- , fDiffuseTextureAccess(diffuse, diffParams)

- , fNormalTextureAccess(normal, normParams)

- , fInvNormRotation(invNormRotation) {

+ , fDiffuseTextureAccess(diffuse, diffParams) {

this->addCoordTransform(&fDiffDeviceTransform);

- this->addCoordTransform(&fNormDeviceTransform);

this->addTextureAccess(&fDiffuseTextureAccess);

- this->addTextureAccess(&fNormalTextureAccess);

// fuse all ambient lights into a single one

fAmbientColor.set(0.0f, 0.0f, 0.0f);

@@ -165,6 +162,7 @@ public:

}

+ this->registerChildProcessor(normalFP);

egdaniel 2016/06/10 14:23:27 use std::move(noramlFP) here

dvonbeck 2016/06/10 15:22:29 Done.

this->initClassID<LightingFP>();

}

@@ -174,7 +172,6 @@ public:

fLightDir.fX = 10000.0f;

fLightColor.fX = 0.0f;

fAmbientColor.fX = 0.0f;

- fInvNormRotation.set(0.0f, 0.0f);

}

void emitCode(EmitArgs& args) override {

@@ -198,33 +195,16 @@ public:

kVec3f_GrSLType, kDefault_GrSLPrecision,

"AmbientColor", &ambientColorUniName);

- const char* xformUniName = nullptr;

- fXformUni = uniformHandler->addUniform(kFragment_GrShaderFlag,

- kVec2f_GrSLType, kDefault_GrSLPrecision,

- "Xform", &xformUniName);

fragBuilder->codeAppend("vec4 diffuseColor = ");

fragBuilder->appendTextureLookupAndModulate(args.fInputColor, args.fTexSamplers[0],

args.fCoords[0].c_str(),

args.fCoords[0].getType());

fragBuilder->codeAppend(";");

- fragBuilder->codeAppend("vec4 normalColor = ");

- fragBuilder->appendTextureLookup(args.fTexSamplers[1],

- args.fCoords[1].c_str(),

- args.fCoords[1].getType());

- fragBuilder->codeAppend(";");

- fragBuilder->codeAppend("vec3 normal = normalColor.rgb - vec3(0.5);");

- 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);

- // TODO: inverse map the light direction vectors in the vertex shader rather than

- // transforming all the normals here!

- fragBuilder->codeAppend("normal = normalize(m*normal);");

+ SkString dstNormalName("dstNormal");

+ this->emitChild(0, nullptr, &dstNormalName, args);

+ fragBuilder->codeAppendf("vec3 normal = %s.xyz;", dstNormalName.c_str());

fragBuilder->codeAppendf("float NdotL = clamp(dot(normal, %s), 0.0, 1.0);",

lightDirUniName);

// diffuse light

@@ -262,12 +242,6 @@ public:

pdman.set3fv(fAmbientColorUni, 1, &ambientColor.fX);

fAmbientColor = ambientColor;

}

- const SkVector& invNormRotation = lightingFP.invNormRotation();

- if (invNormRotation != fInvNormRotation) {

- pdman.set2fv(fXformUni, 1, &invNormRotation.fX);

- fInvNormRotation = invNormRotation;

- }

}

private:

@@ -279,9 +253,6 @@ public:

SkColor3f fAmbientColor;

GrGLSLProgramDataManager::UniformHandle fAmbientColorUni;

- SkVector fInvNormRotation;

- GrGLSLProgramDataManager::UniformHandle fXformUni;

};

void onGetGLSLProcessorKey(const GrGLSLCaps& caps, GrProcessorKeyBuilder* b) const override {

@@ -297,32 +268,25 @@ public:

const SkVector3& lightDir() const { return fLightDir; }

const SkColor3f& lightColor() const { return fLightColor; }

const SkColor3f& ambientColor() const { return fAmbientColor; }

- const SkVector& invNormRotation() const { return fInvNormRotation; }

private:

GrGLSLFragmentProcessor* onCreateGLSLInstance() const override { return new LightingGLFP; }

bool onIsEqual(const GrFragmentProcessor& proc) const override {

+ // TODO add comparison of NormalFPs

egdaniel 2016/06/10 14:23:27 The base class of isEquals takes care of calling c

dvonbeck 2016/06/10 15:22:29 Done.

const LightingFP& lightingFP = proc.cast<LightingFP>();

return fDiffDeviceTransform == lightingFP.fDiffDeviceTransform &&

- fNormDeviceTransform == lightingFP.fNormDeviceTransform &&

fDiffuseTextureAccess == lightingFP.fDiffuseTextureAccess &&

- fNormalTextureAccess == lightingFP.fNormalTextureAccess &&

fLightDir == lightingFP.fLightDir &&

fLightColor == lightingFP.fLightColor &&

- fAmbientColor == lightingFP.fAmbientColor &&

- fInvNormRotation == lightingFP.fInvNormRotation;

+ fAmbientColor == lightingFP.fAmbientColor;

}

GrCoordTransform fDiffDeviceTransform;

- GrCoordTransform fNormDeviceTransform;

GrTextureAccess fDiffuseTextureAccess;

- GrTextureAccess fNormalTextureAccess;

SkVector3 fLightDir;

SkColor3f fLightColor;

SkColor3f fAmbientColor;

- SkVector fInvNormRotation;

};

////////////////////////////////////////////////////////////////////////////

@@ -360,16 +324,12 @@ sk_sp<GrFragmentProcessor> SkLightingShaderImpl::asFragmentProcessor(

// TODO: support different sizes

SkASSERT(fDiffuseMap.width() == fNormalMap.width() &&

fDiffuseMap.height() == fNormalMap.height());

- SkMatrix diffM, normM;

+ SkMatrix diffM;

if (!make_mat(fDiffuseMap, this->getLocalMatrix(), localMatrix, &diffM)) {

return nullptr;

}

- if (!make_mat(fNormalMap, fNormLocalMatrix, localMatrix, &normM)) {

- return nullptr;

- }

bool doBicubic;

GrTextureParams::FilterMode diffFilterMode = GrSkFilterQualityToGrFilterMode(

SkTMin(filterQuality, kMedium_SkFilterQuality),

@@ -378,13 +338,6 @@ sk_sp<GrFragmentProcessor> SkLightingShaderImpl::asFragmentProcessor(

&doBicubic);

SkASSERT(!doBicubic);

- GrTextureParams::FilterMode normFilterMode = GrSkFilterQualityToGrFilterMode(

- SkTMin(filterQuality, kMedium_SkFilterQuality),

- viewM,

- fNormLocalMatrix,

- &doBicubic);

- SkASSERT(!doBicubic);

// TODO: support other tile modes

GrTextureParams diffParams(kClamp_TileMode, diffFilterMode);

SkAutoTUnref<GrTexture> diffuseTexture(GrRefCachedBitmapTexture(context,

@@ -395,18 +348,15 @@ sk_sp<GrFragmentProcessor> SkLightingShaderImpl::asFragmentProcessor(

return nullptr;

}

- GrTextureParams normParams(kClamp_TileMode, normFilterMode);

- SkAutoTUnref<GrTexture> normalTexture(GrRefCachedBitmapTexture(context,

- fNormalMap, normParams,

- gammaTreatment));

- if (!normalTexture) {

- SkErrorInternals::SetError(kInternalError_SkError, "Couldn't convert bitmap to texture.");

- return nullptr;

- }

+ /* TODO is this correct memory handling? FPs were changed to use sk_sp so it is different from

egdaniel 2016/06/10 14:23:28 This should still be fine still

dvonbeck 2016/06/10 15:22:29 Done.

+ * last patch

+ */

+ sk_sp<GrFragmentProcessor> normalFP(

+ fNormalSource->asFragmentProcessor(context, viewM, localMatrix, filterQuality,

+ gammaTreatment));

sk_sp<GrFragmentProcessor> inner (

- new LightingFP(diffuseTexture, normalTexture, diffM, normM, diffParams, normParams, fLights,

- fInvNormRotation));

+ new LightingFP(diffuseTexture, diffM, diffParams, fLights, normalFP));

egdaniel 2016/06/10 14:23:27 I believe you want to use std::move here around no

dvonbeck 2016/06/10 15:22:29 Done.

return GrFragmentProcessor::MulOutputByInputAlpha(std::move(inner));

}

@@ -615,8 +565,11 @@ sk_sp<SkFlattenable> SkLightingShaderImpl::CreateProc(SkReadBuffer& buf) {

invNormRotation = buf.readPoint();

}

+ sk_sp<SkLightingShader::NormalSource> normalSource(

+ static_cast<SkLightingShader::NormalSource*>(buf.readFlattenable(kNormalSource_Type)));

return sk_make_sp<SkLightingShaderImpl>(diffuse, normal, std::move(lights), invNormRotation,

- &diffLocalM, &normLocalM);

+ &diffLocalM, &normLocalM, normalSource);

egdaniel 2016/06/10 14:23:27 wrap normalSouce in std::move

dvonbeck 2016/06/10 15:22:29 Done.

}

void SkLightingShaderImpl::flatten(SkWriteBuffer& buf) const {

@@ -644,6 +597,8 @@ void SkLightingShaderImpl::flatten(SkWriteBuffer& buf) const {

}

buf.writePoint(fInvNormRotation);

+ buf.writeFlattenable(fNormalSource.get());

}

bool SkLightingShaderImpl::computeNormTotalInverse(const ContextRec& rec,

@@ -721,9 +676,10 @@ sk_sp<SkShader> SkLightingShader::Make(const SkBitmap& diffuse, const SkBitmap&

}

SkASSERT(SkScalarNearlyEqual(invNormRotation.lengthSqd(), SK_Scalar1));

+ sk_sp<SkLightingShader::NormalSource> normalSource =

+ SkLightingShader::NormalMapSource::Make(normal, invNormRotation, normLocalM);

return sk_make_sp<SkLightingShaderImpl>(diffuse, normal, std::move(lights),

- invNormRotation, diffLocalM, normLocalM);

+ invNormRotation, diffLocalM, normLocalM, normalSource);

}

///////////////////////////////////////////////////////////////////////////////

« src/core/SkLightingShader.h ('K') | « src/core/SkLightingShader.h ('k') | src/core/SkNormalSourceImpl.cpp » ('j') | tests/SerializationTest.cpp » ('J')