[Mesa-dev] [PATCH] draw: fix clipping with linear interpolated values and gl_ClipVertex

[Roland Scheidegger]

Am 10.12.2015 um 15:44 schrieb Jose Fonseca:
> On 10/12/15 08:09, Dave Airlie wrote:
>> On 10 December 2015 at 14:31,  <sroland at vmware.com> wrote:
>>> From: Roland Scheidegger <sroland at vmware.com>
>>>
>>> Discovered this when working on other clip code, apparently didn't work
>>> correctly - the combination of linear interpolated values and using
>>> gl_ClipVertex produced wrong values (failing all such combinations
>>> in piglits glsl-1.30 interpolation tests).
>>> Use the pre-clip-pos values when determining the interpolation factor to
>>> fix this.
>>> Unfortunately I have no idea what I'm doing here really, but it fixes
>>> all
>>> these failures in piglit (all interpolation-noperspective-XXX-vertex, 10
>>> tests in total). Albeit piglit coverage of clipping isn't great, so
>>> hopefully
>>> someone can confirm this actually makes sense, and wouldn't cause
>>> failures
>>> elsewhere...
>>
>> This makes sense to me, in that interpolating should definitely happen
>> on pre-clipped coordinates.
> 
> The code was added in
> 
> 
> http://cgit.freedesktop.org/mesa/mesa/commit/?id=4625a9b1adf7a30c56e2bbeb41573fbba4465851
> 
> 
> then revised in
> 
> 
> http://cgit.freedesktop.org/mesa/mesa/commit/?id=ab74fee5e1a3fc3323b7238278637b232c2d0d95
> 
> 
> and
> 
> 
> http://cgit.freedesktop.org/mesa/mesa/commit/?id=5da967aff5adb3e27954488206fb885ea1ede0fd
> 
> 
> The 2nd change of Brian seems (if I read correctly) to do precisely the
> opposite.
> 
> I wonder if "clip" and "pre_clip_pos".

What do you wonder?

I'm thinking there could be a mismatch what softpipe and llvmpipe do
here (rather, the llvm vs. non-llvm path). In particular, draw_llvm will
always write to both clip pos and pre-clip-pos, if there wasn't a a
clip_user it will just write the same (look at invocation of store_clip).


> 
> 
> The relevant tests seem to be:
> 
>   GALLIUM_DRIVER=softpipe ./bin/shader_runner
> generated_tests/spec/glsl-1.30/execution/interpolation/interpolation-noperspective-gl_BackColor-flat-fixed.shader_test
> -auto
> 
>   GALLIUM_DRIVER=softpipe ./bin/fbo-blit-stretch -auto
> 
They still seem to pass even with softpipe (and this patch also fixes the
same bug as it did with llvmpipe, i.e.
generated_tests/spec/glsl-1.30/execution/interpolation/interpolation-noperspective-gl_BackColor-flat-vertex.shader_test
and similar.
So maybe non-llvm and llvm path are doing the same these days.
And I suppose using the pos data out of the pos_attr output vs. the one
from clip vs. pre-clip-pos isn't quite doing the same.
I'll do a full piglit run with softpipe too...

Roland



> Jose
> 
> 
>> Though like you my knowledge of this code is debug only.
>>
>> So,
>> Reviewed-by: Dave Airlie <airlied at redhat.com>
>>> ---
>>>   src/gallium/auxiliary/draw/draw_pipe_clip.c | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/gallium/auxiliary/draw/draw_pipe_clip.c
>>> b/src/gallium/auxiliary/draw/draw_pipe_clip.c
>>> index f2b56b0..7f22eef 100644
>>> --- a/src/gallium/auxiliary/draw/draw_pipe_clip.c
>>> +++ b/src/gallium/auxiliary/draw/draw_pipe_clip.c
>>> @@ -192,11 +192,11 @@ static void interp(const struct clip_stage *clip,
>>>         t_nopersp = t;
>>>         /* find either in.x != out.x or in.y != out.y */
>>>         for (k = 0; k < 2; k++) {
>>> -         if (in->clip[k] != out->clip[k]) {
>>> +         if (in->pre_clip_pos[k] != out->pre_clip_pos[k]) {
>>>               /* do divide by W, then compute linear interpolation
>>> factor */
>>> -            float in_coord = in->clip[k] / in->clip[3];
>>> -            float out_coord = out->clip[k] / out->clip[3];
>>> -            float dst_coord = dst->clip[k] / dst->clip[3];
>>> +            float in_coord = in->pre_clip_pos[k] / in->pre_clip_pos[3];
>>> +            float out_coord = out->pre_clip_pos[k] /
>>> out->pre_clip_pos[3];
>>> +            float dst_coord = dst->pre_clip_pos[k] /
>>> dst->pre_clip_pos[3];
>>>               t_nopersp = (dst_coord - out_coord) / (in_coord -
>>> out_coord);
>>>               break;
>>>            }
>>> -- 
>>> 2.1.4
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>