Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(150)

Unified Diff: src/core/SkNormalSource.cpp

Issue 2063793002: API change to allow for NormalSource selection at the user level. (Closed) Base URL: https://skia.googlesource.com/skia@dvonbeck-normal-factor-out
Patch Set: Fixed CPU behavior when normal.Z=-1 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/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
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 divi
robertphillips 2016/07/06 19:11:14 I would say leave the guard for now. I just wanted
dvonbeck 2016/07/06 19:34:36 Alright! I think it should be fine here considerin
- 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
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
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
dvonbeck 2016/07/06 18:07:48 Needs to handle (0, 0, -1) as well. I could set it
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);
}
////////////////////////////////////////////////////////////////////////////

Powered by Google App Engine
This is Rietveld 408576698