Re: [Mesa-dev] [PATCH] softpipe: Fix everything that is wrong with clipping and interpolation.

[Brian Paul]

On 05/30/2012 03:31 PM, Olivier Galibert wrote:

This includes:
- picking up correctly which attributes are flatshaded and which are
   noperspective

- copying the flatshaded attributes when needed, including the
   non-built-in ones

- correctly interpolating the noperspective attributes in screen-space
   instead than in a 3d-correct fashion.

Signed-off-by: Olivier Galibertgalib...@pobox.com
---
  src/gallium/auxiliary/draw/draw_pipe_clip.c |  144 +--
  1 file changed, 113 insertions(+), 31 deletions(-)

I've kicked the f_nopersp computation up so that it's always
evaluated, and I've added a bunch of comments.

Every generated interpolation test in piglit pass for both softpipe
and llvmpipe at that point (after forcing llvmpipe to GLSL 1.30 of
course).

diff --git a/src/gallium/auxiliary/draw/draw_pipe_clip.c 
b/src/gallium/auxiliary/draw/draw_pipe_clip.c
index 4da4d65..2d36eb3 100644
--- a/src/gallium/auxiliary/draw/draw_pipe_clip.c
+++ b/src/gallium/auxiliary/draw/draw_pipe_clip.c
@@ -39,6 +39,7 @@

  #include draw_vs.h
  #include draw_pipe.h
+#include draw_fs.h


  #ifndef IS_NEGATIVE
@@ -56,11 +57,12 @@
  struct clip_stage {
 struct draw_stage stage;  /**  base class */

-   /* Basically duplicate some of the flatshading logic here:
-*/
-   boolean flat;
-   uint num_color_attribs;
-   uint color_attribs[4];  /* front/back primary/secondary colors */
+   /* List of the attributes to be flatshaded. */
+   uint num_flat_attribs;
+   uint flat_attribs[PIPE_MAX_SHADER_OUTPUTS];
+
+   /* Mask of attributes in noperspective mode */
+   boolean noperspective_attribs[PIPE_MAX_SHADER_OUTPUTS];

 float (*plane)[4];
  };
@@ -91,17 +93,16 @@ static void interp_attr( float dst[4],


  /**
- * Copy front/back, primary/secondary colors from src vertex to dst vertex.
- * Used when flat shading.
+ * Copy flat shaded attributes src vertex to dst vertex.
   */
-static void copy_colors( struct draw_stage *stage,
-struct vertex_header *dst,
-const struct vertex_header *src )
+static void copy_flat( struct draw_stage *stage,
+   struct vertex_header *dst,
+   const struct vertex_header *src )
  {
 const struct clip_stage *clipper = clip_stage(stage);
 uint i;
-   for (i = 0; i  clipper-num_color_attribs; i++) {
-  const uint attr = clipper-color_attribs[i];
+   for (i = 0; i  clipper-num_flat_attribs; i++) {
+  const uint attr = clipper-flat_attribs[i];
COPY_4FV(dst-data[attr], src-data[attr]);
 }
  }
@@ -120,6 +121,7 @@ static void interp( const struct clip_stage *clip,
 const unsigned pos_attr = 
draw_current_shader_position_output(clip-stage.draw);
 const unsigned clip_attr = 
draw_current_shader_clipvertex_output(clip-stage.draw);
 unsigned j;
+   float t_nopersp;

 /* Vertex header.
  */
@@ -148,12 +150,36 @@ static void interp( const struct clip_stage *clip,
dst-data[pos_attr][2] = pos[2] * oow * scale[2] + trans[2];
dst-data[pos_attr][3] = oow;
 }
+
+   /**
+* Compute the t in screen-space instead of 3d space to use
+* for noperspective interpolation.
+*
+* The points can be aligned with the X axis, so in that case try
+* the Y.  When both points are at the same screen position, we can
+* pick whatever value (the interpolated point won't be in front
+* anyway), so just use the 3d t.
+*/
+   {
+  int k;
+  t_nopersp = t;
+  for (k = 0; k  2; k++)
+ if (in-data[pos_attr][k] != out-data[pos_attr][k]) {
+t_nopersp = (dst-data[pos_attr][k] - out-data[pos_attr][k]) /
+   (in-data[pos_attr][k] - out-data[pos_attr][k]);
+break;
+ }
+   }

 /* Other attributes
  */
 for (j = 0; j  nr_attrs; j++) {
-  if (j != pos_attr  j != clip_attr)
-interp_attr(dst-data[j], t, in-data[j], out-data[j]);
+  if (j != pos_attr  j != clip_attr) {
+ if (clip-noperspective_attribs[j])
+interp_attr(dst-data[j], t_nopersp, in-data[j], out-data[j]);
+ else
+interp_attr(dst-data[j], t, in-data[j], out-data[j]);
+  }
 }
  }

@@ -406,14 +432,14 @@ do_clip_tri( struct draw_stage *stage,
 /* If flat-shading, copy provoking vertex color to polygon vertex[0]
  */
 if (n= 3) {
-  if (clipper-flat) {
+  if (clipper-num_flat_attribs) {
   if (stage-draw-rasterizer-flatshade_first) {
  if (inlist[0] != header-v[0]) {
 assert(tmpnr  MAX_CLIPPED_VERTICES + 1);
 if (tmpnr= MAX_CLIPPED_VERTICES + 1)
return;
 inlist[0] = dup_vert(stage, inlist[0], tmpnr++);
-   copy_colors(stage, inlist[0], header-v[0]);
+   copy_flat(stage, inlist[0], header-v[0]);
  }
   }
   else {
@@ -422,7 +448,7 @@ do_clip_tri( struct draw_stage *stage,

Re: [Mesa-dev] [PATCH] softpipe: Fix everything that is wrong with clipping and interpolation.

[Dave Airlie]

On Tue, May 29, 2012 at 4:34 PM, Olivier Galibert galib...@pobox.com wrote:
 This includes:
 - picking up correctly which attributes are flatshaded and which are
  noperspective

 - copying the flatshaded attributes when needed, including the
  non-built-in ones

 - correctly interpolating the noperspective attributes in screen-space
  instead than in a 3d-correct fashion.


Have you checked llvmpipe with this? since it might need changes to go
along with this.

Also a count of piglit tests if fixes in the commit might be good.

Otherwise looks good to me, and fixes something I stared at for a few
hours in vain one day.

Dave.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] softpipe: Fix everything that is wrong with clipping and interpolation.

[Brian Paul]

On 05/29/2012 09:34 AM, Olivier Galibert wrote:

This includes:
- picking up correctly which attributes are flatshaded and which are
   noperspective

- copying the flatshaded attributes when needed, including the
   non-built-in ones

- correctly interpolating the noperspective attributes in screen-space
   instead than in a 3d-correct fashion.

Signed-off-by: Olivier Galibertgalib...@pobox.com
---
  src/gallium/auxiliary/draw/draw_pipe_clip.c |  105 +++
  1 file changed, 74 insertions(+), 31 deletions(-)

diff --git a/src/gallium/auxiliary/draw/draw_pipe_clip.c 
b/src/gallium/auxiliary/draw/draw_pipe_clip.c
index 4da4d65..3824ced 100644
--- a/src/gallium/auxiliary/draw/draw_pipe_clip.c
+++ b/src/gallium/auxiliary/draw/draw_pipe_clip.c
@@ -39,6 +39,7 @@

  #include draw_vs.h
  #include draw_pipe.h
+#include draw_fs.h


  #ifndef IS_NEGATIVE
@@ -56,11 +57,12 @@
  struct clip_stage {
 struct draw_stage stage;  /**  base class */

-   /* Basically duplicate some of the flatshading logic here:
-*/
-   boolean flat;
-   uint num_color_attribs;
-   uint color_attribs[4];  /* front/back primary/secondary colors */
+   /* List of the attributes to be flatshaded. */
+   uint num_flat_attribs;
+   uint flat_attribs[PIPE_MAX_SHADER_OUTPUTS];
+
+   /* Mask of attributes in noperspective mode */
+   boolean noperspective_attribs[PIPE_MAX_SHADER_OUTPUTS];

 float (*plane)[4];
  };
@@ -91,17 +93,16 @@ static void interp_attr( float dst[4],


  /**
- * Copy front/back, primary/secondary colors from src vertex to dst vertex.
- * Used when flat shading.
+ * Copy flat shaded attributes src vertex to dst vertex.
   */
-static void copy_colors( struct draw_stage *stage,
-struct vertex_header *dst,
-const struct vertex_header *src )
+static void copy_flat( struct draw_stage *stage,
+   struct vertex_header *dst,
+   const struct vertex_header *src )
  {
 const struct clip_stage *clipper = clip_stage(stage);
 uint i;
-   for (i = 0; i  clipper-num_color_attribs; i++) {
-  const uint attr = clipper-color_attribs[i];
+   for (i = 0; i  clipper-num_flat_attribs; i++) {
+  const uint attr = clipper-flat_attribs[i];
COPY_4FV(dst-data[attr], src-data[attr]);
 }
  }
@@ -120,6 +121,7 @@ static void interp( const struct clip_stage *clip,
 const unsigned pos_attr = 
draw_current_shader_position_output(clip-stage.draw);
 const unsigned clip_attr = 
draw_current_shader_clipvertex_output(clip-stage.draw);
 unsigned j;
+   float t_nopersp = 0;

 /* Vertex header.
  */
@@ -152,8 +154,27 @@ static void interp( const struct clip_stage *clip,
 /* Other attributes
  */
 for (j = 0; j  nr_attrs; j++) {
-  if (j != pos_attr  j != clip_attr)
-interp_attr(dst-data[j], t, in-data[j], out-data[j]);
+  if (j != pos_attr  j != clip_attr) {
+ if (clip-noperspective_attribs[j]) {
+if (!t_nopersp || !t) {


For floating point values, I think this is better/clearer:

   if (t_nopersp == 0.0 || t == 0.0) {


+   // Compute the t in screen-space instead of 3d space.
+   //
+   // When both points are at the same screen position,
+   // we can pick whatever value (the interpolated point
+   // won't be in front anyway), so just use t.
+   int k;
+   t_nopersp = t;


We need more comments here.  This is a loop over the vertex position X 
and Y components which looks for two different X or Y values, then 
computes the new interpolation parameter.


It should also be noted that t_nopersp only needs to be computed once. 
 It would be clearer if it were compute before the for-j loop instead 
of inside.  It's pretty cheap so we could probably do it all the time.




+   for (k=0; k  2; k++)
+  if (in-data[pos_attr][k] != out-data[pos_attr][k]) {
+ t_nopersp = (dst-data[pos_attr][k] - 
out-data[pos_attr][k]) /
+(in-data[pos_attr][k] - out-data[pos_attr][k]);
+ break;
+  }
+}
+interp_attr(dst-data[j], t_nopersp, in-data[j], out-data[j]);
+ } else
+interp_attr(dst-data[j], t, in-data[j], out-data[j]);
+  }
 }
  }

@@ -406,14 +427,14 @@ do_clip_tri( struct draw_stage *stage,
 /* If flat-shading, copy provoking vertex color to polygon vertex[0]
  */
 if (n= 3) {
-  if (clipper-flat) {
+  if (clipper-num_flat_attribs) {
   if (stage-draw-rasterizer-flatshade_first) {
  if (inlist[0] != header-v[0]) {
 assert(tmpnr  MAX_CLIPPED_VERTICES + 1);
 if (tmpnr= MAX_CLIPPED_VERTICES + 1)
return;
 inlist[0] = dup_vert(stage, inlist[0], tmpnr++);
-   copy_colors(stage, inlist[0], header-v[0]);

Re: [Mesa-dev] [PATCH] softpipe: Fix everything that is wrong with clipping and interpolation.

[Olivier Galibert]

On Wed, May 30, 2012 at 11:38:06AM +0100, Dave Airlie wrote:
 Have you checked llvmpipe with this? since it might need changes to go
 along with this.

It didn't look like llvmpipe was going anywhere near that.  OTOH, a
piglit run of llvmpipe I just did on the place gave me zero errors in
that area (including int interpolation if you can believe it) which
left me somewhat surprised.  I didn't think it was working correctly
in the first place...  It was failing in other places due to f.i. the
lack of support for anything other than z24s8, so I didn't hit
softpipe by mistake, I think.


 Also a count of piglit tests if fixes in the commit might be good.

That's going to take a little more time, given my trees are
accumulating not-yet-commited fixes.

  OG.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] softpipe: Fix everything that is wrong with clipping and interpolation.

[Olivier Galibert]

On Wed, May 30, 2012 at 07:32:16AM -0600, Brian Paul wrote:
 All the code above could use more comments.  Otherwise it takes some 
 pretty intense studying to understand what's going on there.

Ok, I'll take that into account (and the previous comments too).

  OG.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev