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

[Roland Scheidegger]

Am 10.12.2015 um 17:06 schrieb Jose Fonseca:
> On 10/12/15 15:53, Roland Scheidegger wrote:
>> 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?
> 
> Ah, if these things are really what they sounds like.
I guess they aren't, both are really clip space coords, just one of them
containing what was in clipVertex instead of position. Albeit I wonder
if we couldn't eliminate one of them. If I see that right, we only need
two if we have clipVertex. But, in this case, I think we could take that
data directly from the "normal" vertex shader clipVertex output. We
can't do that for ordinary pos, since at least for the llvm case we put
the viewport-transformed data there - which is actually a hack causing
problems since all stages which technically come after vs but before
viewport transform need special code to deal with that (though that's
mostly only clip itself and streamout). But, for clipVertex output, we
don't really do anything special with it, so I can't see why we couldn't
pull that data from there. I think that would clean things up.
(Technically, things would actually be a LOT cleaner if viewport
transform had its own stage, but it's of course convenient to place it
at the end of the vs, since you can then do it cheaply in SoA format
without having to really reload the output values, albeit it's a bit
ridiculous if you have things like geometry shaders where it needs to be
done manually afterwards anyway as we can't do that there, and of course
it needs to be redone if things are clipped in any case.)


> 
>>
>> 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).
> 
> Right.  The key point here is to check that those tests don't regress
> with softpipe.

Looks like the non-llvm paths did all the same luckily...

Roland