Fixing shadow map crash on Linux. [do not merge]

[bhouston]

There is this error:

0:201(39): error: sampler arrays indexed with non-constant expressions is forbidden in GLSL 1.30 and later

It occurs on Linux machines. Intel driver at least.
It occurs if shadow maps are enabled.

It should be solvable because the access patterns are similar between lights and shadow maps.

[bhouston]

@gero3 btw I am pretty sure that PRs do not always update. For example this one, I changed the light color to blue in this example and while I see it locally, getting it from the CI machine, the light is still white: http://ci.threejs.org/api/pullrequests/7426/examples/webgl_animation_skinning_morph.html

On my machine the same scene looks like this:

[tschw]

Interestingly, that driver really has a point there (though shouldn't be crashing, of course)...

See GLSL ES 1 Appendix A 4 & 5
See GLSL ES 3 12.14 / 12.29 / 12.30.

Conclusion: The current code is illegal(!) in WebGL 2.

[bhouston]

What construct is illegal exactly? I have had a hard time figuring out
what is wrong.

Best regards,
Ben Houston
http://Clara.io Online 3d modeling and rendering
On Oct 24, 2015 10:19 AM, "tschw" notifications@github.com wrote:

Interesting, that driver really has a point there (though shouldn't be
crashing, of course)...

See GLSL ES 1
https://www.khronos.org/registry/gles/specs/2.0/GLSL_ES_Specification_1.0.17.pdf
Appendix A 4 & 5
See GLSL ES 3
https://www.khronos.org/registry/gles/specs/3.0/GLSL_ES_Specification_3.00.3.pdf
12.14 / 12.29 / 12.30.

Conclusion: The current code is illegal(!) in WebGL 2.

—
Reply to this email directly or view it on GitHub
#7426 (comment).

[tschw]

What construct is illegal exactly?

Indexing an array of samplers with a loop index, most surprisingly. GLSL ES 3, 12.30 reads

Indexing of arrays of samplers by constant-index-expressions is supported in GLSL ES 1.00. A constant-
index-expression is an expression formed from constant-expressions and certain loop indices, defined for
a subset of loop constructs. Should this functionality be included in GLSL ES 3.00?

RESOLUTION: No. Arrays of samplers may only be indexed by constant-integral-expressions

That obviously excludes the index variable (even) of (finite & simple) loops.

[bhouston]

It should be possible to fix this bug as I've looked through the shadow map code in the basic/simple case and it seems to be nearly identical in structure to lighting. The only difference is some of the shadowMap uniforms are conditionally read based on whether or not a shadowMap test passes. I wonder if we decide to always read those uniforms before the test, it will not cause this error? Less efficient yes, but if it works on more platforms, I'm all for it.

I've made that change in this PR but I haven't yet tested it on Intel drivers on Linux. But if anyone wants to the fix is live here:

http://ci.threejs.org/api/pullrequests/7426/examples/webgl_animation_skinning_morph.html

[tschw]

http://ci.threejs.org/api/pullrequests/7426/examples/webgl_animation_skinning_morph.html

We may be using different versions or code paths because of different hardware of that driver. That example was never crashing on mine and I still get the warning (it might just be a coincidence).

However, looking at the problematic ones that were crashing after running OK for a few frames before (e.g. skin / bumpmap), it seems you really did it. No crash! 👍

[bhouston]

It didn't fix it on our Intel GPUs 4600/4000 unfortunately.

It appears that the line in question is this one:

https://github.com/bhouston/three.js/blob/shadow_linux_fix/src/renderers/shaders/ShaderChunk/shadowmap_fragment.glsl#L101

And basically on Mesa you can not access an element of an array of samplers inside of a loop using the index.

I wonder if that restriction applies to accessing a sampler in an array of structures. I will investigate this because the sampler would no longer be an array, rather it would be the structures that are the array construct. Having shadow maps use structures is a good thing anyhow.

Alternatively, one could write a macro that can manually unroll loops.

[bhouston]

@tschw Maybe the driver is fixed in newer versions? That would be amazing. What version of the driver you are using? What is the Intel CPU model number (so I can figure out the GPU version)? What OS are you using? We are using Ubuntu 14 and 15 and we have Intel Core i7 3770 and Intel Core i7 4970 CPUs. We are using the default Intel GPU drivers included in Ubuntu.

[WestLangley]

@bhouston

you can not access an element of an array of samplers inside of a loop using the index.

This has come up before: #5232 (comment)

[bhouston]

I wonder if it possible to use macros in a tricky way to unroll loops for us.

Something like:

#define FOR_LOOP_1PARAM( NumLoops, FuncName, Param1 ) UNROLL_LOOP_1PARAM_##NumLoops( FuncName, Param1 )

#define UNROLL_LOOP_1PARAM_0( FuncName, Param1 ) // do nothing
#define UNROLL_LOOP_1PARAM_1( FuncName, Param1 ) FuncName( 0, Param1 );
#define UNROLL_LOOP_1PARAM_2( FuncName, Param1 ) FuncName( 0, Param1 ); FuncName( 1, Param1 );
.,.

You'd have to put everything into a struct to pass into the function, or have multiple different versions for different number of parameters (does WebGL's preprocessor support variable number of macro parameters?)

I'm making up the above, but I think something like that could technically work, even though it is super messy.

[bhouston]

Or we can write our own preprocess that can unroll loops. I think if we use a Macro-like interface, it shouldn't be that hard. Probably half a day's work.

[tschw]

With the preprocessor, I'd do it like this:

#define REPEAT0() // zero repetitions means empty
#define REPEAT1() repetitive_code(0)
#define REPEAT2() REPEAT1() repetitive_code(1)
#define REPEAT3() REPEAT2() repetitive_code(2)
#define REPEAT4() REPEAT3() repetitive_code(3)
#define REPEAT5() REPEAT4() repetitive_code(4)
#define REPEAT6() REPEAT5() repetitive_code(5)
#define REPEAT7() REPEAT6() repetitive_code(6)
#define REPEAT8() REPEAT7() repetitive_code(7)
// ...

// ---------------- Test starts here:

#define N_LIGHTS REPEAT4 // passed in via .defines

#define repetitive_code( index ) evaluate_light( foo, bar, baz, index );

    N_LIGHTS()

#undef repetitive_code

Note: No need for args - can curry. No need for explosion, can chain.

[mrdoob]

r74dev no longed indexes sampler arrays with non-constant expressions 😀

[bhouston]

Sweet!