Three.js: Pragma unroll_loop does not work with nested code blocks

[gkjohnson]

When unrolling a loop that has a block statement in it the shader breaks because it considers the first } encountered to be closing the for loop. For example this works:

#pragma unroll_loop
for ( int i = 0; i < 6; i ++ ) {

    if ( i < NUM_DIR_LIGHT_SHADOWS ) directLight.color *= getShadow( /* ... */ );

}

But this breaks:

#pragma unroll_loop
for ( int i = 0; i < 6; i ++ ) {

    if ( i < NUM_DIR_LIGHT_SHADOWS ) {

        directLight.color *= getShadow( /* ... */ );

    }

}

The problematic regex is here:

https://github.com/mrdoob/three.js/blob/dev/src/renderers/webgl/WebGLProgram.js#L224

[mrdoob]

Yes, the unroll_loop regex was not done in a general way for simplicity.

Also, as far as I remember, it was mainly for early iPads but I wonder if iOS handles this better these days...

[gkjohnson]

Also, as far as I remember, it was mainly for early iPads but I wonder if iOS handles this better these days...

I see so is the consensus that it's not generally needed, then? It be great to get rid of it. For context I ran into this when looking into adding fade transitions for cascaded shadow maps. It's likely possible to work around this by using functions if #pragma unroll_loop is still desired but it would be cleaner without it.

[mrdoob]

I see so is the consensus that it's not generally needed, then?

Not at all. I bet it's still needed, but maaaaaybe it's no longer needed.

It's likely possible to work around this by using functions if #pragma unroll_loop is still desired but it would be cleaner without it.

Yeah... Either the regex is "clean" or the code is clean... 😵

[Mugen87]

Also, as far as I remember, it was mainly for early iPads but I wonder if iOS handles this better these days...

According to #13090, certain Macbooks with radeon cards required the loop unrool, too.

I see so is the consensus that it's not generally needed, then?

From my point of view, it is required until the opposite can be demonstrated with a few tests. I would not blindly remove it and risk shadow breakage on user devices.

[gkjohnson]

According to #13090, certain Macbooks with radeon cards required the loop unrool, too.

I'm running into that now when trying to use fade with CSM in my project (work computer is a mac). Specifically shadows show up in the wrong spot _only_ when not using pragma unroll_loop and using a texture on the receiving materials.

So it looks like we need to add unroll_loop to the CSM fade branch. I just had a quick try at moving the loop body to a function to avoid the nested block pitfall but the way that RE_Direct function is replaced with implementation-specific ones means I can't write a single fade csm function that takes the correct material parameter to pass through.

@mrdoob How do you feel about something like the unroll loop approach from running in CSM material onBeforeCompile considering this is the only case that needs it? Another option might be to explicitly denote the end of the loop to unroll, which should be just as fast as the current approach:

```glsl

unroll_loop

for ( int i = 0; i < 5; i ++ ) {

// nested blocks for days

}

end_unroll_loop

[mrdoob]

which should be just as fast as the current approach:

#unroll_loop
for ( int i = 0; i < 5; i ++ ) {

 // nested blocks for days

}
#end_unroll_loop

That seems interesting!

I prefer this syntax:

#unroll_loop_start
for ( int i = 0; i < 5; i ++ ) {

  // nested blocks for days

}
#unroll_loop_end