Jump to content

  • Log In with Google      Sign In   
  • Create Account

Awesome job so far everyone! Please give us your feedback on how our article efforts are going. We still need more finished articles for our May contest theme: Remake the Classics

#ActualServant of the Lord

Posted 28 July 2012 - 10:37 AM

Advice A) Learning to use a debugger, and using breakpoints and watch variables greatly aids in figuring out what is going wrong.
Advice B) If something isn't working, trying a simpler version of it, and build upwards. In this case, if "Blit every tile of every layer" isn't working, try a "Blit every tile of the first layer only" for testing.

std::iterator uses [] to support dereferencing. It looks like you are forgetting to de-reference it, and are trying to use it to select the layer or the tile.
std::vector<int> myVector;
std::vector<int>::iterator it = myVector.begin();

it[3] //Accesses the fourth element after whereever "it" is pointing to.
it++;
it++;
it[3] //Accesses the 6th element.

myVector.begin() = 0
++ = 1
++ = 2
[3] = 5  (2 + [3] = 5)

It should be:
std::vector<myTile>::iterator tmpIt;
tmpIt = (*it)[X+(Y * MAP_WIDTH)].begin(); //De-reference 'it' first!

But really, for compact loops like that, it's better to break things out clearly, like this:
std::vector<myTile> tiles = (*it)[X+(Y * MAP_WIDTH)];
std::vector<myTile>::iterator tmpIt;
tmpIt = tiles.begin();
This makes it easier to see and debug where the mistakes are.

However, "tmpIt" is a meaningless name. All your iterators are temp, and all of them are iterators, so "temporary iterator" doesn't describe it at all. You might as well call all your integers, "holdsANumberOfSomeKind", because neither is descriptive. Posted Image
Better (more descriptive) names make it less likely for mistakes to occur, and easier to spot them when they do occur.

Furthermore, your loop is three loops deep, plus an embedded if() statement. This calls greatly for you to break out the function into more than one function for clarity, for ease-of-debugging, and for ease-of-reading.

void DrawArea()
{
	 for(...each layer in area...)
	 {
		  DrawLayer(layer);
	 }
}

void DrawLayer()
{
	 for(...every tile in layer...)
	 {
		  DrawTile(tile);
	 }
}

[Edit:] Ninja-d by two people saying the exact thing I'm saying, and then the OP responding. Posted anyway (despite seeing the "2 people posted while you were commenting" popup), for additional comments and explanations - and because I took the time to type it all out, and am not letting it go to waste. Posted Image

#2Servant of the Lord

Posted 28 July 2012 - 10:36 AM

Advice A) Learning to use a debugger, and using breakpoints and watch variables greatly aids in figuring out what is going wrong.
Advice B) If something isn't working, trying a simpler version of it, and build upwards. In this case, if "Blit every tile of every layer" isn't working, try a "Blit every tile of the first layer only" for testing.

std::iterator uses [] to support dereferencing. It looks like you are forgetting to de-reference it, and are trying to use it to select the layer or the tile.
std::vector<int> myVector;
std::vector<int>::iterator it = myVector.begin();

it[3] //Accesses the fourth element after whereever "it" is pointing to.
it++;
it++;
it[3] //Accesses the 6th element.

myVector.begin() = 0
++ = 1
++ = 2
[3] = 5  (2 + [3] = 5)

It should be:
std::vector<myTile>::iterator tmpIt;
tmpIt = (*it)[X+(Y * MAP_WIDTH)].begin(); //De-reference 'it' first!

But really, for compact loops like that, it's better to break things out clearly, like this:
std::vector<myTile> tiles = (*it)[X+(Y * MAP_WIDTH)];
std::vector<myTile>::iterator tmpIt;
tmpIt = tiles.begin();
This makes it easier to see and debug where the mistakes are.

However, "tmpIt" is a meaningless name. All your iterators are temp, and all of them are iterators, so "temporary iterator" doesn't describe it at all. You might as well call all your integers, "holdsANumberOfSomeKind", because neither is descriptive. Posted Image
Better (more descriptive) names make it less likely for mistakes to occur, and easier to spot them when they do occur.

Furthermore, your loop is three loops deep, plus an embedded if() statement. This calls greatly for you to break out the function into more than one function for clarity, for ease-of-debugging, and for ease-of-reading.

void DrawArea()
{
	 for(...each layer in area...)
	 {
		  DrawLayer(layer);
	 }
}

void DrawLayer()
{
	 for(...every tile in layer...)
	 {
		  DrawTile(tile);
	 }
}

[Edit:] Ninja-d by two people saying the exact thing I'm saying, and then the OP responding. Posted anyway, for additional comments and explanations - and because I took the time to type it all out, and am not letting it go to waste. Posted Image

#1Servant of the Lord

Posted 28 July 2012 - 10:35 AM

Advice A) Learning to use a debugger, and using breakpoints and watch variables greatly aids in figuring out what is going wrong.
Advice B) If something isn't working, trying a simpler version of it, and build upwards. In this case, if "Blit every tile of every layer" isn't working, try a "Blit every tile of the first layer only" for testing.

std::iterator uses [] to support dereferencing. It looks like you are forgetting to de-reference it, and are trying to use it to select the layer or the tile.
std::vector<int> myVector;
std::vector<int>::iterator it = myVector.begin();

it[3] //Accesses the fourth element after whereever "it" is pointing to.
it++;
it++;
it[3] //Accesses the 6th element.

myVector.begin() = 0
++ = 1
++ = 2
[3] = 5  (2 + [3] = 5)

It should be:
std::vector<myTile>::iterator tmpIt;
tmpIt = (*it)[X+(Y * MAP_WIDTH)].begin(); //De-reference 'it' first!

But really, for compact loops like that, it's better to break things out clearly, like this:
std::vector<myTile> tiles = (*it)[X+(Y * MAP_WIDTH)];
std::vector<myTile>::iterator tmpIt;
tmpIt = tiles.begin();
This makes it easier to see and debug where the mistakes are.

However, "tmpIt" is a meaningless name. All your iterators are temp, and all of them are iterators, so "temporary iterator" doesn't describe it at all. You might as well call all your integers, "holdsANumberOfSomeKind", because neither is descriptive. Posted Image
Better (more descriptive) names make it less likely for mistakes to occur, and easier to spot them when they do occur.

Furthermore, your loop is three loops deep, plus an embedded if() statement. This calls greatly for you to break out the function into more than one function for clarity, for ease-of-debugging, and for ease-of-reading.

void DrawArea()
{
     for(...each layer in area...)
     {
          DrawLayer(layer);
     }
}

void DrawLayer()
{
     for(...every tile in layer...)
     {
          DrawTile(tile);
     }
}

PARTNERS