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

[Paul Berry]

On 2 July 2012 15:12, Vadim Girlin <vadimgirlin at gmail.com> wrote:

> 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.
>

Oh, ok.  I misunderstood.  Sandy Bridge and Ivy Bridge are similar.


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

Yeah, that makes sense.


>
> > 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 :)
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120702/c8f67915/attachment-0001.html>