mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102.81k stars 35.38k forks source link

Cascading shadowmaps broken #4507

Closed jbaicoianu closed 9 years ago

jbaicoianu commented 10 years ago

It looks like cascading shadows are broken, going back at least a couple builds. Take a look at the morphtarget_md2_control example:

http://threejs.org/examples/#webgl_morphtargets_md2_control

I'm no expert in how shadowmapping works, but what it looks like to me is that the lowest-level shadowmap texture is being used for all levels, rather than each level being a separate texture.

mrdoob commented 10 years ago

Hmmm... I see something different. There seems to be some sort of layering working properly. But there is some jumping when zooming in/out...

https://www.youtube.com/watch?v=ZUQ5kXc1E3A

WestLangley commented 10 years ago

Setting

renderer.shadowMapDebug = true;
light.shadowCameraVisible = true;

it may be easier to discern what is happening.

jbaicoianu commented 10 years ago

Ah I see - seems this might be a linux-specific issue, I just noticed this in the debug log:

gl.getProgramInfoLog() warning: Variable sampler array index unsupported. This feature of the language was removed in GLSL 1.20 and is unlikely to be supported for 1.10 in Mesa.

http://youtu.be/2BXPGSNhevU

This is on an Intel gfx card in Ubuntu 14.04, and I saw the same issue with older versions of Ubuntu as well. I believe I saw the same error on nvidia, but I will double check and see if the same issue exists there.

WestLangley commented 10 years ago

@jbaicoianu That looks a lot like #3257 (which was fixed, by the way).

mrdoob commented 10 years ago

Ah I see - seems this might be a linux-specific issue, I just noticed this in the debug log:

gl.getProgramInfoLog() warning: Variable sampler array index unsupported. This feature of the language was removed in GLSL 1.20 and is unlikely to be supported for 1.10 in Mesa.

Man, I'm so happy I exposed all these warnings... :)

jbaicoianu commented 10 years ago

Hmm, definitely looks like the same visual end result, but I'm seeing this on an Intel gfx card in Linux, that bug was traced to an OSX ATI driver bug. Maybe the same bug exists in the Intel driver?

With nvidia in Linux, and I see the same thing as mrdoob. The shadowmap works mostly fine, but it seems there's a frame or more of lag on the lower-detailed shadowmaps when zoomed out. Try zooming out a bit and panning wildly - the shadows lag behind where they should be, but if you zoom in to the highest detail shadowmap, they're smooth.

So, looks like two different issues at play here. I suspect the lagging-a-frame-behind issue exists on all cards, and the only-displaying-the-first-texture bug is driver-specific.

mrdoob commented 10 years ago

With nvidia in Linux, and I see the same thing as mrdoob. The shadowmap works mostly fine, but it seems there's a frame or more of lag on the lower-detailed shadowmaps when zoomed out. Try zooming out a bit and panning wildly - the shadows lag behind where they should be, but if you zoom in to the highest detail shadowmap, they're smooth.

Ah, nice observation. That sounds like an easy to solve problem.

jbaicoianu commented 10 years ago

Here's a snippet from the libmesa code regarding the issue:

     if (index) {
          i = index->value.i[0];
      } else {
          /* GLSL 1.10 and 1.20 allowed variable sampler array indices,
           * while GLSL 1.30 requires that the array indices be
           * constant integer expressions.  We don't expect any driver
           * to actually work with a really variable array index, so
           * all that would work would be an unrolled loop counter that ends
           * up being constant above.
           */
           shader_program->InfoLog =
               talloc_asprintf_append(shader_program->InfoLog,
                    "warning: Variable sampler array index "
                    "unsupported.\nThis feature of the language "
                    "was removed in GLSL 1.20 and is unlikely "
                    "to be supported for 1.10 in Mesa.\n");
                i = 0;
      }

I think the part of the Three.js shadowmap shader which is trigging the error is this:

There's a comment in that code about manually unrolling the inner loop for ATI cards, but that doesn't seem to have fixed the issue on Intel. Is the outer loop causing issues, with the fact that we're accessing shadowMap[i] when i is an int, non-const? Does this loop need unrolling too?

Seems like unrolling that outer loop would make for some messy code, unless it's broken out into functions. Is this the right way to go or is there a simpler fix?

mrdoob commented 10 years ago

With nvidia in Linux, and I see the same thing as mrdoob. The shadowmap works mostly fine, but it seems there's a frame or more of lag on the lower-detailed shadowmaps when zoomed out. Try zooming out a bit and panning wildly - the shadows lag behind where they should be, but if you zoom in to the highest detail shadowmap, they're smooth.

Ah, nice observation. That sounds like an easy to solve problem.

Hmmm, I had a quick look and didn't see anything obvious... :/

Usnul commented 10 years ago

cascaded shadow maps are jumpy, choppy, they do not support more than 3 layers (you can configure more, but you won't get more), they aren't documented and controls for splitting the frustum are anything but intuitive. just my constructive input, i didn't mention this before because i don't have a solution to these. Also, i feel like shadows are somewhat of a weak point in threejs overall. I'm sure this will all be different few months from now, but as it stands shadows could use "something" that would make them:

KillerJim commented 10 years ago

As @jbaicoianu commented, the issue is due to the fact that a variable array index is passed into texture2D, aka:

"depthKernel[0][0] = unpackDepth( texture2D( shadowMap[ i ], shadowCoord.xy + vec2( dx0, dy0 ) ) );

To see if the maps could be easily fixed, without creating a TON of extra code I created a "getShadowMapFragment()" function to handle the variable index, therefore replacing the look-up line to:

"depthKernel[0][0] = unpackDepth( getShadowMapFragment( i, shadowCoord.xy + vec2( dx0, dy0 ) ) );"

and adding the routine as:

        "vec4 getShadowMapFragment( int index, vec2 coords ) {",
            "if (index == 0) {",
                "return texture2D( shadowMap[ 0 ], coords) ; ",
            "}",
            "if (index == 1) {",
                "return texture2D( shadowMap[ 1 ], coords) ; ",
            "}",
            "if (index == 2) {",
                "return texture2D( shadowMap[ 2 ], coords) ; ",
            "}",
            "return texture2D( shadowMap[ 0 ], coords) ;",
        "}",

Of course its not great, switch statements aren't widely supported - plus you will need a few "#if MAX_SHADOWS < X" around each "if statement" in-case there are more/less than 3 shadows. But this fixes the morphtargets_md2_control example on Linux/Intel..

However, I now see the "blurring" and poping effect.

mrdoob commented 10 years ago

I assume this doesn't do anything...?

"sampler2D map = shadowMap[ i ];",
"depthKernel[0][0] = unpackDepth( texture2D( map, shadowCoord.xy + vec2( dx0, dy0 ) ) );"
gero3 commented 10 years ago

it seems like you can't do that in glsl you can't define a sampler2D as variable only as function argument and uniform.

KillerJim commented 10 years ago

As gero3 says, you can't do that; nor can you return a sampler2D from a function. The only optimisation I could see what to use the switch() statement when supported.

ramakarl commented 9 years ago

Why doesnt this work? depthKernel[0][0] = unpackDepth ( texture2D ( shadowMap[ i ], shadowCoord.xy + vec2(dx0,dy0) ) ); You're not passing a variable into texture2D, you're passing a sampler2D, which is selected by index prior to the call.

mrdoob commented 9 years ago

@rchoetzlein could you do a PR with a fix? 😇

ramakarl commented 9 years ago

@mrdoob.. thanks for making 3js btw. There are two issues I see. The Linux/Intel index access issue, for which @KillerJim has a switch-based workaround (try using the '?' operator, usually better), and is most likely driver specific. The other is the 'jumping' issue seen in your youtube video on multiple hardware. @jbaicoianu sees "a frame or two lag", from which it's not clear if he's seeing the same jumping issue, or if this is a third issue. Since the 'jumping' is seen on multiple hardware, it may also be a run-time or app-level issue. In any case, if you can do some experiments (like killerjim did for the other issue), you should be able to determine where this is coming from. If it turns out to be a linux/nvidia specific, I can help file it. But need to isolate the app code that triggers it.

ramakarl commented 9 years ago

Ok, sorry, just found out what PR means on github.. I'm with nvidia, and PR can mean a driver patch for us, so I thought you were asking for a driver fix. The second bug issue may not be a driver issue, and isolating the app condition that triggers the 'jumping' is next step.. I'll take a look if I have time, and/or check back on this thread.

mrdoob commented 9 years ago

@rchoetzlein thanks!