[Mesa-dev] [PATCH 6/8] draw: fix front face injection

[Roland Scheidegger]

Am 02.08.2013 08:28, schrieb Zack Rusin:
> Inject front face only if the fragment shader uses it and
> propagate through all channels because otherwise we'll
> need to figure out the exact swizzle that the fs expects and
> it's just simpler to make sure all the components within
> the front face register are correctly set.
> 
> Signed-off-by: Zack Rusin <zackr at vmware.com>
> ---
>  src/gallium/auxiliary/draw/draw_pipe_unfilled.c |   24 ++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/src/gallium/auxiliary/draw/draw_pipe_unfilled.c b/src/gallium/auxiliary/draw/draw_pipe_unfilled.c
> index d8a603f..f9a31b0 100644
> --- a/src/gallium/auxiliary/draw/draw_pipe_unfilled.c
> +++ b/src/gallium/auxiliary/draw/draw_pipe_unfilled.c
> @@ -37,6 +37,7 @@
>  #include "pipe/p_defines.h"
>  #include "draw_private.h"
>  #include "draw_pipe.h"
> +#include "draw_fs.h"
>  
>  
>  struct unfilled_stage {
> @@ -67,18 +68,20 @@ inject_front_face_info(struct draw_stage *stage,
>        (stage->draw->rasterizer->front_ccw && ccw) ||
>        (!stage->draw->rasterizer->front_ccw && !ccw));
>     unsigned slot = unfilled->face_slot;
> -   struct vertex_header *v0 = header->v[0];
> -   struct vertex_header *v1 = header->v[1];
> -   struct vertex_header *v2 = header->v[2];
> +   unsigned i;
>  
>     /* In case the backend doesn't care about it */
>     if (slot < 0) {
>        return;
>     }
>  
> -   v0->data[slot][0] = is_front_face;
> -   v1->data[slot][0] = is_front_face;
> -   v2->data[slot][0] = is_front_face;
> +   for (i = 0; i < 3; ++i) {
> +      struct vertex_header *v = header->v[i];
> +      v->data[slot][0] = is_front_face;
> +      v->data[slot][1] = is_front_face;
> +      v->data[slot][2] = is_front_face;
> +      v->data[slot][3] = is_front_face;
> +   }
>  }
That part makes no sense to me. fs expects a float with a
negative/positive value in first component, and 0.0/0.0/1.0 anyway in
the others hence filling in all slots like that serves absolutely no
purpose at all. If you'd really want to do that here so you can blindly
use constant interpolation later you'd have to fill that in correspondingly.


>  
>     
> @@ -231,9 +234,12 @@ draw_unfilled_prepare_outputs( struct draw_context *draw,
>  {
>     struct unfilled_stage *unfilled = unfilled_stage(stage);
>     const struct pipe_rasterizer_state *rast = draw ? draw->rasterizer : 0;
> -   if (rast &&
> -       (rast->fill_front != PIPE_POLYGON_MODE_FILL ||
> -        rast->fill_back != PIPE_POLYGON_MODE_FILL)) {
> +   boolean is_unfilled = (rast &&
> +                          (rast->fill_front != PIPE_POLYGON_MODE_FILL ||
> +                           rast->fill_back != PIPE_POLYGON_MODE_FILL));
> +   const struct draw_fragment_shader *fs = draw->fs.fragment_shader;
> +   
> +   if (is_unfilled && fs && fs->info.uses_frontface)  {
>        unfilled->face_slot = draw_alloc_extra_vertex_attrib(
>           stage->draw, TGSI_SEMANTIC_FACE, 0);
>     } else {
> 
It's things like that which made me wish we could just always pass front
face information somewhere. Logic like that keeps piling up, and you can
just hope you don't end up in a situation where draw is configured not
to do the face culling itself.
But I guess on its own this looks alright.

Roland