Re: [AD] clipping line algorithm

[Jon Rafkind <workmin@xxxxxxxxxx>]

ok, looks good to me. I agree about the 'const int' thing, but maybe it wouldnt be a bad idea to #undef them at the end of the function? Otherwise, someone might run into macro evilness eventually.

On Fri, 19 Sep 2003 16:14:07 +0200
Eric Botcazou <ebotcazou@xxxxxxxxxx> wrote:

> > I have fixed up your version so that it compiles[ you were right, it
> > didnt compile as is ;) ]. Performance was vastly improved using vline
> > and hline. Clipline now performs almost the same as line when no
> > clipping must be performed. Tests show clipline actually performs faster
> > than line even when no clipping is performed, I dont know why though.
> 
> "clipline" is typically 50% faster with Linux/DGA2 on my machine with actual 
> clipping, no difference without actual clipping.
> 
> > Attached is clipline3 and the main loop I used to benchmark it with.
> 
> Thanks for both.
> 
> 
> A few nits:
> 
> Try to follow Allegro's coding conventions.
> 
> >  const int TOP = 0x8;
> >  const int BOTTOM = 0x4;
> >  const int RIGHT = 0x2;
> >  const int LEFT = 0x1;
> 
> This is a C++ idiom. Use preprocessor symbols instead in Allegro.
> 
> >  float slope;
> >  int b;
> 
> Both are unused. Compile at least with -Wall with GCC.
> 
> > ASSERT( w );
> 
> There is no symbol named 'w'. It's 'bmp'.
> 
> >  /* not sure if this is really necessary.
> >  * if not, rename the arguements without the 
> >  * leading underscore: _x1 -> x1
> >  */
> >  if ( _x1 > _x2 ){
> >	x1 = _x2;
> >	x2 = _x1;
> >  } else {
> >	x1 = _x1;
> >	x2 = _x2;
> >  }
> >
> > if ( _y1 > _y2 ){
> >		y1 = _y2;
> >		y2 = _y1;
> > } else {
> >		y1 = _y1;
> >		y2 = _y2;
> > }
> 
> This is both unnecessary and wrong: you can't swap the x-coordinates without 
> also swapping the y-coordinates.
> 
> >  } else if ( outcode & BOTTOM ){
> > if ( y2 - y1 == 0 )
> >  	x = x1;
> > else
> >	x = x1 + ( x2 - x1 ) * ( ymax - y1 ) / ( y2 - y1 );
> > y = ymin;
> 
> 'ymin', not 'ymax', in the formula.
> 
> >  } else {
> > if ( x2 - x1 == 0 )
> > 	y = y1;
> > else
> > 	y = y1 + (y2-y1) * (xmax-x1)/(x2-x1);
> > x = xmin;
> 
> 'xmin', not 'xmax', in the formula.
> 
> 
> And note that you use the wrong orientation for the y-axis:
> 
>    if ( y > ymax ) code |= TOP; else if ( y < ymin ) code |= BOTTOM; \
> 
> The TOP of the bitmap is (y < ymin), the BOTTOM is (y > ymax). But this 
> doesn't affect the algorithm since you can rename TOP into BOTTOM and vice 
> versa.
> 
> I've applied the attached patch to mainline.
> 
> -- 
> Eric Botcazou