Bug 31159 - shadow problem in 0ad game
shadow problem in 0ad game
Status: RESOLVED FIXED
Product: Mesa
Classification: Unclassified
Component: Mesa core
git
x86 (IA32) Linux (All)
: medium normal
Assigned To: mesa-dev
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-27 05:07 UTC by Fabio Pedretti
Modified: 2011-03-08 07:41 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
example of wrong shadow in the 0ad editor behind the lion (124.74 KB, image/jpeg)
2010-10-27 05:07 UTC, Fabio Pedretti
Details
clamp in sample_compare (2.89 KB, patch)
2011-03-07 16:00 UTC, Philip Taylor
Details | Splinter Review
swrast patch (3.86 KB, patch)
2011-03-08 06:32 UTC, Philip Taylor
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fabio Pedretti 2010-10-27 05:07:33 UTC
Created attachment 39808 [details]
example of wrong shadow in the 0ad editor behind the lion

Using the 0ad game in some maps or in the editor appear a shadow covering all the screen behind last element that shouldn't be there. The simple way to reproduce this is through the editor:

1. run the game with the -editor command line;
2. click on the third button "Object";
3. select an element (e.g. lion) and move the cursor over the map: the wrong shadow appear behind the lion as in the attached screenshot.

Note that changing sun orientation also moves the shadow. Also, disabling shadows (shadows = false in ~/.config/0ad/config/local.cfg ) made it disappears. Lastly, the wrong shadow is only behind the element that's more far from the sun in the map (indeed the obelisk shadow is ok).

I reproduced this with both r300g and softpipe. There was a discussion on the 0ad forum about this but nobody was able to reproduce the problem (most are using windows or linux with proprietary drivers).
Comment 1 Marek Olšák 2011-03-03 05:22:11 UTC
I was looking into this issue and I wasn't able to find the cause. I guess there is a bug in the texenv program code, because both swrast and gallium fail, and because the only shaders which generate 2DSHADOW texture fetch instructions are texenv programs.

Would it be possible to make a simple test case?
Comment 2 Philip Taylor 2011-03-07 15:14:38 UTC
I see this bug with softpipe, but not with i965 (non-Gallium).

I don't have a simple test case, but I've observed the problem in the game is where parts of the terrain are outside the far plane of the camera used for generating the shadow depth texture.

I think that means the (s,t,r) texcoords used for the depth texture comparison have r > 1.0. The depth texture sampled at (s,t) should have value 1.0, and r should be clamped to [0, 1] (per the GL spec, for non-floating-point depth textures), so the LEQUAL depth test should pass and the terrain should be non-shadowed.

Looking at softpipe's sp_tex_sample.c, the p[] values passed into sample_compare are often larger than 1.0, so it seems to me that something is failing to correctly clamp the texcoords before sampling the texture.
Comment 3 Brian Paul 2011-03-07 15:27:56 UTC
(In reply to comment #2)
[...]
> Looking at softpipe's sp_tex_sample.c, the p[] values passed into
> sample_compare are often larger than 1.0, so it seems to me that something is
> failing to correctly clamp the texcoords before sampling the texture.

Can you try adding clamping there and retest?
Comment 4 Philip Taylor 2011-03-07 16:00:16 UTC
Created attachment 44215 [details] [review]
clamp in sample_compare

With this patch on the 7.10 release, the game's shadows now render correctly in softpipe.
Comment 5 Brian Paul 2011-03-07 18:01:24 UTC
This also fixes the piglit depth-tex-compare test!  Neither softpipe nor llvmpipe were clamping the texture coordinate to [0,1], per the spec.  llvmpipe still doesn't pass depth-tex-compare completely (for ==, != cases), but I believe that's a defect in the test itself.

Softpipe still has other issues with shadow compare w.r.t. linear vs bilinear filtering but that's less important than this fix.

Does this bug report only pertain to the softpipe driver?  If so, please test Mesa/master with this patch (commit 0eef561a5bb10df343837d58d37d5c0d5b708243) and close.
Comment 6 Brian Paul 2011-03-07 18:02:39 UTC
(In reply to comment #1)
> I was looking into this issue and I wasn't able to find the cause. I guess
> there is a bug in the texenv program code, because both swrast and gallium
> fail, and because the only shaders which generate 2DSHADOW texture fetch
> instructions are texenv programs.

The 2DSHADOW fetch will be generated by GLSL shaders that use shadow2D().
Comment 7 Marek Olšák 2011-03-07 18:23:31 UTC
(In reply to comment #6)
> (In reply to comment #1)
> > I was looking into this issue and I wasn't able to find the cause. I guess
> > there is a bug in the texenv program code, because both swrast and gallium
> > fail, and because the only shaders which generate 2DSHADOW texture fetch
> > instructions are texenv programs.
> 
> The 2DSHADOW fetch will be generated by GLSL shaders that use shadow2D().

I only meant 0ad, which doesn't use GLSL.

I'll fix r300 soon..
Comment 8 Philip Taylor 2011-03-07 18:53:48 UTC
(In reply to comment #5)
> Does this bug report only pertain to the softpipe driver?

No - comment #0 reported it on r300g, and comment #1 on swrast, and also it broke on llvmpipe. (I have no idea whether any more drivers are affected similarly.)
Comment 9 Marek Olšák 2011-03-07 20:18:05 UTC
Now that I have fixed r300, I am changing the summary to "swrast: ..."
Comment 10 Philip Taylor 2011-03-08 06:32:23 UTC
Created attachment 44230 [details] [review]
swrast patch

This adds clamping to swrast's sample_depth_texture, but there's another bug: With GL_LEQUAL, shadow_compare correctly returns 1 if coord <= depthSample, whereas shadow_compare4 returns 0 if depthXX <= coord (for all XX). Looks like compare4 is trying to flip the conditional, but flipping <= gives > not >=, so it's wrong when e.g. depthXX == coord == 1. Fixing this (plus the clamping) makes the shadows work for me in swrast.

Also, I noticed the GL_ALWAYS case in shadow_compare4 differs from shadow_compare and seems wrong. Also the default case reports the wrong function name and the return value differs from shadow_compare. So I've tried to fix those as well.

Mesa/master softpipe and llvmpipe both fix the shadow bug for me too.
Comment 11 Brian Paul 2011-03-08 07:32:12 UTC
(In reply to comment #10)
> Created an attachment (id=44230) [details]
> swrast patch
> 
> This adds clamping to swrast's sample_depth_texture, but there's another bug:
> With GL_LEQUAL, shadow_compare correctly returns 1 if coord <= depthSample,
> whereas shadow_compare4 returns 0 if depthXX <= coord (for all XX). Looks like
> compare4 is trying to flip the conditional, but flipping <= gives > not >=, so
> it's wrong when e.g. depthXX == coord == 1. Fixing this (plus the clamping)
> makes the shadows work for me in swrast.
> 
> Also, I noticed the GL_ALWAYS case in shadow_compare4 differs from
> shadow_compare and seems wrong. Also the default case reports the wrong
> function name and the return value differs from shadow_compare. So I've tried
> to fix those as well.
> 
> Mesa/master softpipe and llvmpipe both fix the shadow bug for me too.

Thanks.  The GL_EQUAL and GL_NOTEQUAL cases were wrong too.  I'll fix that bit in the patch.

I think I'll flip the conditionals in shadow_compare4() so it looks more like shadow_compare() and the spec language.

Closing.