feedback on this line function?

Started by
4 comments, last by Zahlman 14 years, 1 month ago
This is as clean a function as I've written, I'd like to get some pointers on coding smarter in general if anyone has the inclination. Function draws a line between two points in array unsigned buf[ROW*ROW*RGB]. If slope is outside the range of 1 to -1 then swap x and y values. If x is negative then swap points.
double round(double x) { //thanks kccarter
  return ((x - floor(x)) >= 0.5) ? ceil(x) : floor(x);
}
void drawLine( int x1, int y1, int x2, int y2, unsigned char buf[], unsigned char color[] ) {
	int xdif = x2-x1;
	int ydif = y2-y1;
	if( abs(xdif) > abs(ydif) ) {
		if( xdif  > 0 ) {
			float slope = ydif / float(xdif);
			for(int x = 0; x < xdif; x++) {
				int y = int( round(slope*x) );
				buf[ ( x+x1 + (y+y1)*ROW ) * RGB + RED]   = color[RED];
				buf[ ( x+x1 + (y+y1)*ROW ) * RGB + GREEN] = color[GREEN];
				buf[ ( x+x1 + (y+y1)*ROW ) * RGB + BLUE]  = color[BLUE];
			}
		} else drawLine( x2, y2, x1, y1, buf ); //swap points 1 & 2
	} else { //same code but swap x and y values
		if( ydif  > 0 ) {
			float slope = xdif / float(ydif);
				for(int y = 0; y < ydif; y++) {
					int x = int( round(slope*y) );
					buf[ ( x+x1 + (y+y1)*ROW ) * RGB + RED]   = color[RED];
					buf[ ( x+x1 + (y+y1)*ROW ) * RGB + GREEN] = color[GREEN];
					buf[ ( x+x1 + (y+y1)*ROW ) * RGB + BLUE]  = color[BLUE];
				}
		} else if( xdif != ydif ) drawLine( x2, y2, x1, y1, buf ); //swap points 1 & 2
	}
}



[Edited by - avocados on March 5, 2010 9:12:19 AM]
Advertisement
There's one major bug: It will loop forever (possibly stack overflowing) if the start and end coordinates are equal.

I'd also suggest letting the caller of the function pick the colour of the line they want drawing.

Also adding 0.5 then casting to int doesn't round negative numbers correctly. Consider -0.75 for example.
Addressed your 3 points, thanks!
Bresenham's algorithm is typically implemented without floating point computations.

See the article for a thorough discussion on trade-offs and implementation details.
I defeintely second Bresenham's algorithm for a start. In your case we're easily looking at a 5 times speedup. Float to int conversion is slow! There even are improvement over Bresenham's, but it's a good starting point.

Also, don't put too much faith in your compiler's common subexpression elimination optimisation. For one, your code will run far slower in debug when it isn't performing that optimisation and it might not as fast as it could in a release build if it doesn't see the optimisation there either.
"In order to understand recursion, you must first understand recursion."
My website dedicated to sorting algorithms
Quote:Original post by iMalc
Also, don't put too much faith in your compiler's common subexpression elimination optimisation. For one, your code will run far slower in debug when it isn't performing that optimisation and it might not as fast as it could in a release build if it doesn't see the optimisation there either.


For another, it's messy. If you eliminate a common subexpression, and then there turns out to be something wrong with the subexpression, now there's only one place where you have to fix it instead of many.

This topic is closed to new replies.

Advertisement