[Mesa-dev] [PATCH] st_program.c: gl_ClipDistance must be interpolated in 3d space.

[Vadim Girlin]

On Mon, 2012-07-02 at 14:19 -0700, Paul Berry wrote:
> On 2 July 2012 13:45, Vadim Girlin <vadimgirlin at gmail.com> wrote:
> 
> > On Mon, 2012-07-02 at 10:08 -0700, Paul Berry wrote:
> > > On 2 July 2012 08:04, Vadim Girlin <vadimgirlin at gmail.com> wrote:
> > >         On Sun, 2012-06-24 at 11:18 +0200, Olivier Galibert wrote:
> > >         > That old bug was hidden but the clipper always interpolating
> > >         in 3d
> > >         > space no matter what it should have been doing.  Now that
> > >         the
> > >         > interpolation has been fixed, the bug shows up.
> > >         >
> > >         > Fixes bugzilla 51364.
> > >         >
> > >         > Signed-off-by: Olivier Galibert <galibert at pobox.com>
> > >         >
> > >         > diff --git a/src/mesa/state_tracker/st_program.c
> > >         b/src/mesa/state_tracker/st_program.c
> > >         > index e6664fb..9f98298 100644
> > >         > --- a/src/mesa/state_tracker/st_program.c
> > >         > +++ b/src/mesa/state_tracker/st_program.c
> > >         > @@ -569,12 +569,12 @@ st_translate_fragment_program(struct
> > >         st_context *st,
> > >         >           case FRAG_ATTRIB_CLIP_DIST0:
> > >         >              input_semantic_name[slot] =
> > >         TGSI_SEMANTIC_CLIPDIST;
> > >         >              input_semantic_index[slot] = 0;
> > >         > -            interpMode[slot] = TGSI_INTERPOLATE_LINEAR;
> > >         > +            interpMode[slot] =
> > >         TGSI_INTERPOLATE_PERSPECTIVE;
> > >         >              break;
> > >         >           case FRAG_ATTRIB_CLIP_DIST1:
> > >         >              input_semantic_name[slot] =
> > >         TGSI_SEMANTIC_CLIPDIST;
> > >         >              input_semantic_index[slot] = 1;
> > >         > -            interpMode[slot] = TGSI_INTERPOLATE_LINEAR;
> > >         > +            interpMode[slot] =
> > >         TGSI_INTERPOLATE_PERSPECTIVE;
> > >
> > >
> > >         At first glance this change doesn't seem correct, GLSL 1.30
> > >         spec says
> > >         that clip distances are interpolated linearly. Am I missing
> > >         something?
> > >
> > > I think when the spec says "The clip distances will be linearly
> > > interpolated across the primitive...", they mean "linear in 3D space".
> > > Interpolating clip distances in screen space would produce really bad
> > > artifacts, IMHO.
> >
> > Looking into it, I think you are right, sorry for the noise.
> >
> > Though then I have some doubts about the "fs-clip-distance-interpolated"
> > test. AFAICS r600g in fact uses different interpolation for ClipDistance
> > inputs with and without the patch, but the test does pass in both
> > cases.
> >
> 
> Yeah, fs-clip-distance-interpolated only checks that the fragment shader
> sees properly interpolated values for gl_ClipDistance.  It doesn't check
> that the implementation uses the correct form of interpolation to decide
> where to clip the primitive.  I suspect that's the reason the test passes
> in both cases.  I wasn't thinking carefully enough when I wrote the test to
> realize that these two things ought to be tested separately.

AFAIK on r600 we have no control over the interpolation of the clip
distances for clipping, so it should be always correct, this patch only
affects the interpolation of the values that are passed to the fragment
shader. 

I think the test should draw something with perspective to check whether
the interpolation is perspective-correct.

> If you would like to add a piglit test to cover the case I missed, I would
> be delighted :)

I'll try to find the time :)