Fixed gpu perlin noise on Windows

Fixed gpu perlin noise on Windows

I just cleaned up and re-ordered some instructions to reduce the number of registers used under the register limit. Instead of looping over channels, the function now only loops over octaves and computes all 4 channels separately within each octave loop. I made a noise function to make it simpler to make the changes and I fixed the code's alignment so that it's more readable.

R=bsalomon@google.com, senorblanco@chromium.org

Committed: https://code.google.com/p/skia/source/detail?r=9486

[Stephen White, 2013-06-07 18:25:01]

This LGTM, but I'll leave for Brian to take a look.

https://codereview.chromium.org/15753005/diff/1/src/effects/SkPerlinNoiseShad...
File src/effects/SkPerlinNoiseShader.cpp (right):

https://codereview.chromium.org/15753005/diff/1/src/effects/SkPerlinNoiseShad...
src/effects/SkPerlinNoiseShader.cpp:1139: builder->fsCodeAppendf("\n\t\tvec2 %s
= floor((%s*vec3(%s, 1.0)).xy) * %s;",
Nit: spaces around the *

https://codereview.chromium.org/15753005/diff/1/src/effects/SkPerlinNoiseShad...
src/effects/SkPerlinNoiseShader.cpp:1161: "vec4(\n\t\t\t\t%s(%s, %s,
%s),\n\t\t\t\t%s(%s, %s, %s),"
This might be more readable (albeit more Appendfs) as four separate appends,
with one GLSL line for each.

https://codereview.chromium.org/15753005/diff/1/src/effects/SkPerlinNoiseShad...
src/effects/SkPerlinNoiseShader.cpp:1169: "vec4(\n\t\t\t\t%s(%s,
%s),\n\t\t\t\t%s(%s, %s),"
Same here.

https://codereview.chromium.org/15753005/diff/1/src/effects/SkPerlinNoiseShad...
src/effects/SkPerlinNoiseShader.cpp:1179: builder->fsCodeAppendf(" / %s;",
ratio);
Perf nit: consider a reciprocal-and-multiply here.

https://codereview.chromium.org/15753005/diff/1/src/effects/SkPerlinNoiseShad...
src/effects/SkPerlinNoiseShader.cpp:1181: builder->fsCodeAppendf("\n\t\t\t%s *=
vec2(2.0);", noiseVec);
Then multiply by 0.5 here.

[sugoi1, 2013-06-07 18:42:23]

I made the small changes requested. Note that trybots are running on the first
uploaded patch and I'm going to let them continue since the changes here are
mostly aesthetic.

https://codereview.chromium.org/15753005/diff/1/src/effects/SkPerlinNoiseShad...
File src/effects/SkPerlinNoiseShader.cpp (right):

https://codereview.chromium.org/15753005/diff/1/src/effects/SkPerlinNoiseShad...
src/effects/SkPerlinNoiseShader.cpp:1139: builder->fsCodeAppendf("\n\t\tvec2 %s
= floor((%s*vec3(%s, 1.0)).xy) * %s;",
On 2013/06/07 18:25:02, Stephen White wrote:
> Nit: spaces around the *

Done.

https://codereview.chromium.org/15753005/diff/1/src/effects/SkPerlinNoiseShad...
src/effects/SkPerlinNoiseShader.cpp:1161: "vec4(\n\t\t\t\t%s(%s, %s,
%s),\n\t\t\t\t%s(%s, %s, %s),"
On 2013/06/07 18:25:02, Stephen White wrote:
> This might be more readable (albeit more Appendfs) as four separate appends,
with one GLSL line for each.

As discussed, because of the 100 character limit, I'm going to leave it this way
as I find it more readable that way.

https://codereview.chromium.org/15753005/diff/1/src/effects/SkPerlinNoiseShad...
src/effects/SkPerlinNoiseShader.cpp:1169: "vec4(\n\t\t\t\t%s(%s,
%s),\n\t\t\t\t%s(%s, %s),"
On 2013/06/07 18:25:02, Stephen White wrote:
> Same here.

Same here.

https://codereview.chromium.org/15753005/diff/1/src/effects/SkPerlinNoiseShad...
src/effects/SkPerlinNoiseShader.cpp:1179: builder->fsCodeAppendf(" / %s;",
ratio);
On 2013/06/07 18:25:02, Stephen White wrote:
> Perf nit: consider a reciprocal-and-multiply here.

Done.

https://codereview.chromium.org/15753005/diff/1/src/effects/SkPerlinNoiseShad...
src/effects/SkPerlinNoiseShader.cpp:1181: builder->fsCodeAppendf("\n\t\t\t%s *=
vec2(2.0);", noiseVec);
On 2013/06/07 18:25:02, Stephen White wrote:
> Then multiply by 0.5 here.

Done.

[bsalomon, 2013-06-10 13:39:25]

lgtm with some style nits.

Maybe you've already done this, but given how many limitations in drivers and
such this effect revealed on its way into the code base the first time, can you
run some try bots on a variety of platforms before committing?

https://codereview.chromium.org/15753005/diff/9001/src/effects/SkPerlinNoiseS...
File src/effects/SkPerlinNoiseShader.cpp (right):

https://codereview.chromium.org/15753005/diff/9001/src/effects/SkPerlinNoiseS...
src/effects/SkPerlinNoiseShader.cpp:1012: static const GrGLShaderVar
perlinnoise_Args[] =  {
We already have two conventions for static const globals, I don't think we need
a third :)

In some places kWeNameThemWithALowerCaseKRegardlessOfTheType
gAndInOthersWithAGIfTheyreNotIntegral.

https://codereview.chromium.org/15753005/diff/9001/src/effects/SkPerlinNoiseS...
src/effects/SkPerlinNoiseShader.cpp:1023: SkString noise_code;
localVarsAreLikeThis.

https://codereview.chromium.org/15753005/diff/9001/src/effects/SkPerlinNoiseS...
src/effects/SkPerlinNoiseShader.cpp:1132:
builder->emitFunction(GrGLShaderBuilder::kFragment_ShaderType, kFloat_GrSLType,
"perlinnoise",
Would this be easier to read if the there was just a single if?

if (fStitchTiles) {
 builder->emitFunction()
} else
 builder->emitFunction()
}

[bsalomon, 2013-06-10 13:40:13]

On 2013/06/10 13:39:25, bsalomon wrote:
> lgtm with some style nits.
> 
> Maybe you've already done this, but given how many limitations in drivers and
> such this effect revealed on its way into the code base the first time, can
you
> run some try bots on a variety of platforms before committing?
> 
Just saw in your prev comment that you already ran bots on patchset 1. Good
enough for me.

[sugoi1, 2013-06-10 13:59:27]

Committed patchset #2 manually as r9486 (presubmit successful).