[Mesa-dev] [PATCH] draw: fix line stippling

[Roland Scheidegger]

Am 15.03.2016 um 08:20 schrieb Jose Fonseca:
> On 13/03/16 20:40, sroland at vmware.com wrote:
>> From: Roland Scheidegger <sroland at vmware.com>
>>
>> The logic was comparing actual ints, not true/false values.
>> This meant that it was emitting always multiple line segments instead
>> of just
>> one even if the stipple test had the same result, which looks
>> inefficient, and
>> the segments also overlapped thus breaking line aa as well.
>> (In practice, with the no-op default line stipple pattern, for a 10-pixel
>> long line from 0-9 it was emitting 10 segments, with the individual
>> segments
>> ranging from 0-1, 0-2, 0-3 and so on.)
>>
>> This fixes https://bugs.freedesktop.org/show_bug.cgi?id=94193
>>
>> CC: <mesa-stable at lists.freedesktop.org>
>> ---
>>   src/gallium/auxiliary/draw/draw_pipe_stipple.c | 30
>> +++++++++++++-------------
>>   1 file changed, 15 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/gallium/auxiliary/draw/draw_pipe_stipple.c
>> b/src/gallium/auxiliary/draw/draw_pipe_stipple.c
>> index dcf05aa..04854ad 100644
>> --- a/src/gallium/auxiliary/draw/draw_pipe_stipple.c
>> +++ b/src/gallium/auxiliary/draw/draw_pipe_stipple.c
>> @@ -108,11 +108,11 @@ emit_segment(struct draw_stage *stage, struct
>> prim_header *header,
>>   }
>>
>>
>> -static inline unsigned
>> +static inline boolean
>>   stipple_test(int counter, ushort pattern, int factor)
>>   {
>>      int b = (counter / factor) & 0xf;
>> -   return (1 << b) & pattern;
>> +   return !!((1 << b) & pattern);
>>   }
>>
>>
>> @@ -126,7 +126,7 @@ stipple_line(struct draw_stage *stage, struct
>> prim_header *header)
>>      const float *pos0 = v0->data[pos];
>>      const float *pos1 = v1->data[pos];
>>      float start = 0;
>> -   int state = 0;
>> +   boolean state = 0;
>>
>>      float x0 = pos0[0];
>>      float x1 = pos1[0];
>> @@ -143,29 +143,29 @@ stipple_line(struct draw_stage *stage, struct
>> prim_header *header)
>>         stipple->counter = 0;
>>
>>
>> -   /* XXX ToDo: intead of iterating pixel-by-pixel, use a look-up table.
>> +   /* XXX ToDo: instead of iterating pixel-by-pixel, use a look-up
>> table.
>>       */
>>      for (i = 0; i < length; i++) {
>> -      int result = stipple_test( (int) stipple->counter+i,
>> -                                 (ushort) stipple->pattern,
>> stipple->factor );
>> +      boolean result = stipple_test((int)stipple->counter + i,
>> +                                    (ushort)stipple->pattern,
>> stipple->factor);
>>         if (result != state) {
>>            /* changing from "off" to "on" or vice versa */
>> -     if (state) {
>> -        if (start != i) {
>> +         if (state) {
>> +            if (start != i) {
>>                  /* finishing an "on" segment */
>> -           emit_segment( stage, header, start / length, i / length );
>> +               emit_segment(stage, header, start / length, i / length);
>>               }
>> -     }
>> -     else {
>> +         }
>> +         else {
>>               /* starting an "on" segment */
>> -        start = (float) i;
>> -     }
>> -     state = result;   
>> +            start = (float)i;
>> +         }
>> +         state = result;
>>         }
>>      }
>>
>>      if (state && start < length)
>> -      emit_segment( stage, header, start / length, 1.0 );
>> +      emit_segment(stage, header, start / length, 1.0);
>>
>>      stipple->counter += length;
>>   }
>>
> 
> I think we're avoiding "boolean" over the standard C99 "bool".
> 
> Otherwise looks good.
> 
> Reviewed-by: Jose Fonseca <jfonseca at vmware.com>

Ok I'll change that. The reason I used boolean was that draw code near
exclusively is using boolean, not bool (with a ratio of about 100:1) but
it probably makes sense to use bool when things are changed.

Roland