Quote:Original post by Zahlman
Yep, looks like a particularly strange case of this problem. :)
Yeah, that's exactly what happened. Unfortunately, it doesn't offer a good method of avoiding it, as far as I can see.
Quote:The way to avoid this kind of thing is to eliminate the code duplication, so that there's nowhere to forget to swap x and y. In C++, we can do this by using references to alias the x and y directions, or (as shown) by (conditionally) swapping the data around to a consistent order, and swapping it back at the end. It's still possible to mess up, but the code that can mess up is localized and minimized.
I'll do that, that's a good idea. I rarely use references, so it'll be good for me to use them here to get more familiar with them. I don't see the benefit in passing the callback by reference as opposed to a pointer, though. Why do it that way?
Quote:Also, why not initialize things with a constructor? :)
Actually, I didn't post the entire class. I had already isolated the bug to
Traverse(), and didn't want to just toss my entire code at someone for them to shift through, wasting their time.
The class does optionally initialize using the constructor, or else the constructor sets default values. I want to be able to change the positions of the line later, which is why I have the
SetPoints() function. If you decide to initialize it in the constructor, it just calls the
SetPoints() function.
Quote:// the old 'currentPoint' should not be a member, since the concept is only
// used in Traverse(), where it's a local anyway.
'currentPoints' is important, because the class also has a way to step through the line at your own leisure, instead of all at once like
Traverse() does. 'error' is needed as a member of the class for the same reason.
Traverse() uses 'currentPoints' locally, so you can
Step() through the line, and halfway through
Traverse() the line, without messing up
Step(). The same bug was happening to both
Traverse() and
Step(), so I posted the function that had all the code in one spot.
My entire class looks like this:
#ifndef LINE_H#define LINE_Hstruct LinePoint{ int x; int y;};LinePoint NewLinePoint(int x, int y); typedef bool (LineFunctionPointer)(LinePoint *point);class Line{ private: //Where the line starts. LinePoint startingPoint; //Where the line ends. LinePoint endingPoint; //The amount to increment the current position by, on each 'step'. LinePoint deltaIncrement; //Used to offset the line when calculating the next point. LinePoint offsetPoint; //The current position along the line. LinePoint currentPoint; //Used by the Step() function. int error; void _ConfigurePoints(); public: Line::Line(); Line::Line(LinePoint start, LinePoint end); Line::~Line(); //Starts a new line, with a new starting and ending point. void SetPoints(LinePoint start, LinePoint end); //Steps along the line 'x' times, where x is 'steps'. Returns false if it reaches the end //of the line. 'point' points to where the line stopped. bool Step(LinePoint *point, int steps = 1); //Returns the point where the line is currently at. LinePoint GetLastStep(); //Resets the point where the line is currently, back to the starting point. void Reset(); //Switches the direction the line traverses. (From foreward to backward, or vice versa) //You may want to call Reset() afterward, to reset the current step, depending on your needs. //This actually just swaps the ending and starting points. void Reverse(); //Used to loop through the entire line at one time, calling the callback function on each step. //If the callback returns true, the function will end. void Traverse(LineFunctionPointer *callback); //Used to loop through the line backward, calling the callback function on each step. //If the callback returns true, the function will end. void ReverseTraverse(LineFunctionPointer *callback);};#endif /* LINE_H */
#include "Line.h"LinePoint NewLinePoint(int x, int y){ LinePoint newPoint; newPoint.x = x; newPoint.y = y; return newPoint;}int abs(int value){ if(value > 0) return value; else return -value;}/////////////////////////////////////////////////////////////////////////////////////////////Line::Line(){ startingPoint.x = 0; startingPoint.y = 0; endingPoint.x = 0; endingPoint.y = 0; deltaIncrement.x = 0; deltaIncrement.y = 0; offsetPoint.x = 0; offsetPoint.y = 0; currentPoint.x = 0; currentPoint.y = 0; error = 0;}Line::Line(LinePoint start, LinePoint end){ this->SetPoints(start, end);}Line::~Line(){ }//Starts a new line, with a new starting and ending point.void Line::SetPoints(LinePoint start, LinePoint end){ startingPoint = start; endingPoint = end; currentPoint = start; deltaIncrement.x = 0; deltaIncrement.y = 0; offsetPoint.x = 0; offsetPoint.y = 0; this->_ConfigurePoints();}//Steps along the line 'x' times, where x is 'steps'. Returns false if it reaches the end//of the line. 'point' points to where the line stopped.bool Line::Step(LinePoint *point, int steps){ if(point) *point = currentPoint; for(int i = 0; i < steps; i++) { if(deltaIncrement.x > deltaIncrement.y) { if(currentPoint.x != endingPoint.x) { error -= deltaIncrement.y; if(error < 0) { currentPoint.y += offsetPoint.y; error += deltaIncrement.x; } currentPoint.x += offsetPoint.x; } else return false; } else { if(currentPoint.y != endingPoint.y) { error -= deltaIncrement.x; if(error < 0) { currentPoint.x += offsetPoint.x; error += deltaIncrement.y; } currentPoint.y += offsetPoint.y; } else return false; } } return true;}//Returns the point where the line is currently at.LinePoint Line::GetLastStep(){ return currentPoint;}//Resets the point where the line is currently, back to the starting point.void Line::Reset(){ currentPoint = startingPoint;}//Switches the direction the line traverses. (From foreward to backward, or vice versa)//You may want to call Reset() afterward, to reset the current step, depending on your needs.//This actually just swaps the ending and starting points.void Line::Reverse(){ LinePoint tempPoint = startingPoint; startingPoint = endingPoint; endingPoint = tempPoint; this->_ConfigurePoints();}//Used to quickly loop through the entire line at one time, calling the callback function on each step.void Line::Traverse(LineFunctionPointer *callback){ if(!callback) return; LinePoint currentPoint = startingPoint; if((*callback)(¤tPoint) == true) return; if(deltaIncrement.x > deltaIncrement.y) { while(currentPoint.x != endingPoint.x) { error -= deltaIncrement.y; if(error < 0) { currentPoint.y += offsetPoint.y; error += deltaIncrement.x; } currentPoint.x += offsetPoint.x; if((*callback)(¤tPoint) == true) return; } } else { while(currentPoint.y != endingPoint.y) { error -= deltaIncrement.x; if(error < 0) { currentPoint.x += offsetPoint.x; error += deltaIncrement.y; } currentPoint.y += offsetPoint.y; if((*callback)(¤tPoint) == true) return; } }}//Used to loop through the line backward, calling the callback function on each step.void Line::ReverseTraverse(LineFunctionPointer *callback){ this->Reverse(); this->Traverse(callback); this->Reverse();}void Line::_ConfigurePoints(){ deltaIncrement.x = abs(endingPoint.x - startingPoint.x); deltaIncrement.y = abs(endingPoint.y - startingPoint.y); if(startingPoint.x > endingPoint.x) offsetPoint.x = -1; else offsetPoint.x = 1; if(startingPoint.y > endingPoint.y) offsetPoint.y = -1; else offsetPoint.y = 1; if(deltaIncrement.x > deltaIncrement.y) error = deltaIncrement.x / 2; else error = deltaIncrement.y / 2;}
I'll definitely rewrite it to use references as you showed. I particularily didn't like the double 'while' loop much. Thanks for the tips!