Quote:Original post by yewbie
Wanna tear this one apart too? haha im so awful
Sure thing. :)
Please skim through the whole post first, ignoring source, and then make sure to read it all later. All the ideas here are important, and aren't ordered by importance but by logical order of execution.Step 1: tell, don't ask. One of the reasons for this is that "asking" imposes on the other object to be able to answer. Another is that solving a problem by "asking" tends to require doing a lot of asking, compared to just one round of telling that provides all the necessary information.
In our case, AnimateTiles constantly tries to "ask" MyAnimTiles[i] about itself. We should instead demand that it animate(), and "tell" it the necessary information. That requires that we add the .animate() function to the Tile interface, but in exchange we get to make lots of things private (like they ought to be) and/or remove accessors (if you were using them). That's
real encapsulation.
Notice, though, that the tile needs to be able to break AnimateTiles' loop. To do that, we return a value, and check it in the loop.
// Notice how a zillion instances of 'MyAnimTiles' simply disappear.// Also notice how I stealthily removed a lot of useless comments, and// reformatted a few things...// Animate the tile. Return whether or not this is the "sentinel".// I'm guessing about the names of your classes, of course. Adjust as needed.bool Tile::Animate(int currenttickcount, const SceneData& scenedata, TileMap& tilemap, Surface* offscreenmap, const Tile* mapinfo) { // I assume your "mapinfo" is a single rectangular array, and not some // kind of dynamically allocated monstrosity. if (iAnimNum == 0) { return true; } if ((iLastTickCount + iAnimSpeed) < currenttickcount) { iTile++; // if we are past our last tile, reset it to the original // start tile in the animation. if (iTile > (iOrignalTile + iAnimNum) - 1) // reset if past our anim count (-1 so we count our first tile) { iTile = iOrignalTile; } //only do below if its on screen + 2 tiles if ((x > (scenedata.topleftX - 2)) && //top left (x < (scenedata.topleftX + TILE_SCREEN_MAX_X + 2)) && //bottom right (y > (scenedata.topleftY - 2)) && //top left (y < (scenedata.topleftY + TILE_SCREEN_MAX_Y + 2))) { //huge if statement with bounds start //update the tiles on top of it for (int layer = 0; layer < TILEMAP_LAYERS; layer++) { // Notice how we disambiguate the local variable // from the member variable here: if (layer == this->layer) { //update the location on our off screen map tilemap.PutTile(offscreenmap, x * TILE_SIZE_X, y * TILE_SIZE_Y, iTile); } else { if (mapinfo[x][y][layer].iTile > 0) // if there is a tile there { // update the location on our off screen // map using our animated tile to show // where we are tilemap.PutTile(offscreenmap, x * TILE_SIZE_X, y * TILE_SIZE_Y, mapinfo[x][y][layer].iTile); } } } } iLastTickCount = currenttickcount; } return false; // not the last one}void AnimateTiles(){ DWORD currenttickcount = GetTickCount(); for (int i = 0; i < MAX_ANIM_TILES; i++) { if (MyAnimTiles.Animate(currenttickcount, MySceneData, tsTileMap, lpddsOffScreenMap, MyMapInfo)) { break; } } }
Step 2: Fix our container. This is C++; we don't need to make an array of some "max" size and then check how many elements are actually being used with some kind of "flag" data. Instead, we use the standard library container std::vector. This will avoid the need for our hack to break the loop.
I'm a little worried about something at this point. Is there, in fact, a Tile for every coordinate on the map? If there is, then there's no reason to store x and y values within each tile. I'm going to assume, though, that you only have Tiles for the parts of the map which are animating.
Anyway, while I'm on this step, I'm going to fix up some names to be a little more consistent, mostly by getting rid of the useless Hungarian notation. The C++ type system is much stronger than C's, and it wants to help you; let it.
// More comments disappear as the names become clearer.void Tile::Animate(int currentTickCount, const SceneData& sceneData, TileMap& tileMap, Surface* offScreenMap, const Tile* mapInfo) { if ((lastTickCount + ticksPerAnimationFrame) < currentTickCount) { frame++; // if we are past our last tile, reset it to the original // start tile in the animation. if (frame > (firstFrame + frameCount) - 1) { frame = firstFrame; } // Check if the tile is within the scene bounds. // BTW, I think you'll agree that names like 'topleftX' and // 'topleftY' are quite strangely round-about ways of saying... if ((x > (sceneData.left - 2)) && // left (x < (sceneData.left + TILE_SCREEN_MAX_X + 2)) && // right (y > (sceneData.top - 2)) && // top (y < (sceneData.top + TILE_SCREEN_MAX_Y + 2))) // bottom { // Update each layer at the tile's location. for (int layer = 0; layer < TILEMAP_LAYERS; layer++) { if (layer == this->layer) // draw our tile in the current layer. { tilemap.PutTile(offScreenMap, x * TILE_SIZE_X, y * TILE_SIZE_Y, frame); } else // draw tiles from the map info in other layers. { if (mapInfo[x][y][layer].frame > 0) // there is a tile to draw { // update the location on our off screen // map using our animated tile to show // where we are tilemap.PutTile(offScreenMap, x * TILE_SIZE_X, y * TILE_SIZE_Y, mapInfo[x][y][layer].frame); } } } } lastTickCount = currentTickCount; }}void AnimateTiles(){ DWORD currentTickCount = GetTickCount(); std::vector<Tile>::iterator end = animatedTiles.end(); for (std::vector<Tile>::iterator it = animatedTiles.begin(); i != end; ++it) { it->Animate(currentTickCount, sceneData, tileMap, offScreenMap, mapInfo); } }
Step 3: Use "guard clauses" in the Animate function (early returns) to avoid deep nesting. The basic idea is, we replace "if something, do the rest of this function" with "if not something, ignore the rest of this function". Notice that we want to update the tick count even if the sprite is off screen, so we can't just return there. However, the inner if scope doesn't care about the value of the tick count, so we can fix the problem by just updating the tick count
before checking if the tile is in bounds.
// More comments disappear as the names become clearer.void Tile::Animate(int currentTickCount, const SceneData& sceneData, TileMap& tileMap, Surface* offScreenMap, const Tile* mapInfo) { if ((lastTickCount + ticksPerAnimationFrame) >= currentTickCount) { return; // not time to update this tile. } frame++; // if we are past our last tile, reset it to the original // start tile in the animation. if (frame > (firstFrame + frameCount) - 1) { frame = firstFrame; } lastTickCount = currentTickCount; // Check if the tile is within the scene bounds. // Pay close attention to how the condition is inverted. The ands get // replaced by ors. Google 'de Morgan' if you don't understand why. if ((x <= (sceneData.left - 2)) || // left (x >= (sceneData.left + TILE_SCREEN_MAX_X + 2)) || // right (y <= (sceneData.top - 2)) || // top (y >= (sceneData.top + TILE_SCREEN_MAX_Y + 2))) // bottom { return; } // Update each layer at the tile's location. for (int layer = 0; layer < TILEMAP_LAYERS; layer++) { if (layer == this->layer) // draw our tile in the current layer. { tilemap.PutTile(offScreenMap, x * TILE_SIZE_X, y * TILE_SIZE_Y, frame); } else // draw tiles from the map info in other layers. { if (mapInfo[x][y][layer].frame > 0) // there is a tile to draw { // update the location on our off screen // map using our animated tile to show // where we are tilemap.PutTile(offScreenMap, x * TILE_SIZE_X, y * TILE_SIZE_Y, mapInfo[x][y][layer].frame); } } }}void AnimateTiles(){ DWORD currentTickCount = GetTickCount(); std::vector<Tile>::iterator end = animatedTiles.end(); for (std::vector<Tile>::iterator it = animatedTiles.begin(); i != end; ++it) { it->Animate(currentTickCount, sceneData, tileMap, offScreenMap, mapInfo); } }
Step 4: Same idea as step 1, again.
When we check the tile bounds, what are we checking against? The bounds of the SceneData, yes? Who has that information? The SceneData - so that's who should perform the check. Similarly, the MapInfo knows what non-animating tiles are at the current location, so it is what should do the drawing of that "stack" of tiles. In the first case, we just pass the x and y along, so the SceneData knows what location to check. In the second, we need to communicate x and y, but also our frame - so that the MapInfo can substitute it in where appropriate - and our layer - so that the MapInfo knows where it goes in the order.
Oh, but there's a problem, you say - the MapInfo isn't an object! It's just some multi-dimensional array. Well, we can easily fix that - just make the existing array be a member of some object, and instantiate the object instead of an array. I'll assume we call that member 'data'.
That will make a lot of other things easier, too. For example, the previous steps
don't actually work, because passing the array as a pointer lost the information about the array size. ;) (You could make them work by doing arithmetic with the known dimensions of the array, but it's not very much fun.)
// Even more comments disappear, too. Creating new functions gives you a way to// attach descriptive names to processes, which avoids having to explain what// you are doing.bool SceneData::isInBounds(int x, int y){ // I re-inverted the check. Sorry! :) return (x > (left - 2)) && (x < (left + TILE_SCREEN_MAX_X + 2)) && // right (y > (top - 2)) && (y < (top + TILE_SCREEN_MAX_Y + 2)); // bottom}void MapInfo::drawLocationWithAnimatedTile(int x, int y, int layer, int frame, TileMap& tileMap, Surface* offScreenMap){ // I change the counter variable now to avoid a name conflict. for (int i = 0; i < TILEMAP_LAYERS; i++) { if (i == layer) // draw our tile in the current layer. { tilemap.PutTile(offScreenMap, x * TILE_SIZE_X, y * TILE_SIZE_Y, frame); } else // draw tiles from the map info in other layers. { if (data[x][y].frame > 0) // there is a tile to draw { // update the location on our off screen // map using our animated tile to show // where we are tilemap.PutTile(offScreenMap, x * TILE_SIZE_X, y * TILE_SIZE_Y, data[x][y].frame); } } }}void Tile::Animate(int currentTickCount, const SceneData& sceneData, TileMap& tileMap, Surface* offScreenMap, const MapInfo& mapInfo) { if ((lastTickCount + ticksPerAnimationFrame) >= currentTickCount) { return; // not time to update this tile. } frame++; // if we are past our last tile, reset it to the original // start tile in the animation. if (frame > (firstFrame + frameCount) - 1) { frame = firstFrame; } lastTickCount = currentTickCount; if (!sceneData.isInBounds(x, y)) { return; } mapInfo.drawLocationWithAnimatedTile(x, y, layer, frame, tileMap, offScreenMap);}void AnimateTiles(){ DWORD currentTickCount = GetTickCount(); std::vector<Tile>::iterator end = animatedTiles.end(); for (std::vector<Tile>::iterator it = animatedTiles.begin(); i != end; ++it) { it->Animate(currentTickCount, sceneData, tileMap, offScreenMap, mapInfo); } }
Step 5: We can now go backwards a little - our second guard clause doesn't make much sense any more in Tile::Animate(), because it only "guards" one statement.
Meanwhile, the drawLocationWithAnimatedTile() implementation can be simplified: in both cases, we calculate a tile to draw, and call a function with all the same parameters. If we determine the tile index *first*, then we only have to write the function call once. Also, SceneData::isInBounds() is a little paranoid with the brackets.
Finally, we can do an arithmetic simplification on the frame, if we change our convention: store the frame *relative to the sequence*, as well as the frame count. Then, our 'frame' member loops from 0 up to (frameCount - 1) and back, which we can do with a simple increment-and-modulus. Then to get the "actual" frame, we just add the 'frame' to the "offset" represented by 'firstFrame'. (This will presumably require changes to some of your other code which isn't illustrated here. In particular, the Tile constructor.)
bool SceneData::isInBounds(int x, int y){ return x > (left - 2) && x < (left + TILE_SCREEN_MAX_X + 2) && // right y > (top - 2) && y < (top + TILE_SCREEN_MAX_Y + 2); // bottom}void MapInfo::drawLocationWithAnimatedTile(int x, int y, int layer, int frame, TileMap& tileMap, Surface* offScreenMap){ // I change the counter variable now to avoid a name conflict. for (int i = 0; i < TILEMAP_LAYERS; i++) { int toDraw = (i == layer) ? frame : data[x][y]; if (toDraw > 0) // There is a tile to draw. That will always happen when // i == layer, but it doesn't hurt to check. :) { tilemap.PutTile(offScreenMap, x * TILE_SIZE_X, y * TILE_SIZE_Y, toDraw); } } }}void Tile::Animate(int currentTickCount, const SceneData& sceneData, TileMap& tileMap, Surface* offScreenMap, const MapInfo& mapInfo) { if ((lastTickCount + ticksPerAnimationFrame) >= currentTickCount) { return; // not time to update this tile. } frame = (frame + 1) % frameCount; lastTickCount = currentTickCount; if (sceneData.isInBounds(x, y)) { mapInfo.drawLocationWithAnimatedTile(x, y, layer, frame + firstFrame, tileMap, offScreenMap); }}void AnimateTiles(){ DWORD currentTickCount = GetTickCount(); std::vector<Tile>::iterator end = animatedTiles.end(); for (std::vector<Tile>::iterator it = animatedTiles.begin(); i != end; ++it) { it->Animate(currentTickCount, sceneData, tileMap, offScreenMap, mapInfo); } }
Step 6: Conceptually, this is the big one. Separate updating and rendering logic. The 'drawLocationWithAnimatedTile' function tries to do too much - as you can tell by the name. Besides, what if you put two animated tiles one on top of the other? The first will get overwritten with a normal tile, when you process the second. What you want to do is let the MapInfo object track the changes to the data - by storing the frames into the array! - and then render, all at once, everything that needs to be rendered.
But those are still not steps that need to follow each other directly. By separating updating and rendering logic, you gain the ability to render several times per update, or vice versa. A common strategy in games, to keep the physics consistent, is to update at regular intervals, while rendering as often as possible in the remaining CPU time. This way, the game doesn't *behave differently* if there is some lag; it merely looks a little blocky for a while.
To make this work, we'll need to add some data to the MapInfo to keep track of whether a "column" (set of layers at a given x, y) has changed since the last rendering. To update it, we'll set the appropriate tile, and also flag the corresponding column. To render it, we'll render all the tiles in every flagged column, and clear all the flags. This also avoids special case code in the rendering: we just read the tile values, since they're already set as needed.
We should update every tile, not just those that are in bounds; otherwise, animations can get out of sync. We'll only render the tiles that are in bounds, by having the SceneData ask the MapInfo to do the rendering, telling it what bounds to use. Notice that the control flow changes radically, but the guiding principle of "tell, don't ask" is the same.
void SceneData::render(const MapInfo& mapInfo, TileMap& tileMap, Surface* offScreenMap){ // Check that the constant values are what they ought to be: // the width and height of the map, not "corrected" by -1. mapInfo.render(x, y, TILE_SCREEN_MAX_X, TILE_SCREEN_MAX_Y, tileMap, offScreenMap);}void MapInfo::render(int x, int y, int width, int height, TileMap& tileMap, Surface* offScreenMap){ for (int i = x; i < x + width; ++i) { if (i < 0 || i >= MAP_WIDTH) { continue; } for (int j = y; j < y + height; ++j) { if (j < 0 || j >= MAP_HEIGHT || !dirty[j]) { continue; } for (int layer = 0; layer < TILEMAP_LAYERS; ++layer) { tilemap.PutTile(offScreenMap, x * TILE_SIZE_X, y * TILE_SIZE_Y, data[x][y][layer]); } dirty[j] = false; } }}void MapInfo::Update(int x, int y, int layer, int frame){ data[x][y][layer] = frame; dirty[x][y] = true;}// Our old "Animate" function for the tile will really only update;// rendering is taken care of completely by the MapInfo.// We end up removing a whole bunch of parameters here.void Tile::Update(int currentTickCount) { if ((lastTickCount + ticksPerAnimationFrame) >= currentTickCount) { return; // not time to update this tile. } frame = (frame + 1) % frameCount; lastTickCount = currentTickCount; mapInfo.Update(x, y, layer, frame + firstFrame);}void UpdateTiles(){ DWORD currentTickCount = GetTickCount(); std::vector<Tile>::iterator end = animatedTiles.end(); for (std::vector<Tile>::iterator it = animatedTiles.begin(); i != end; ++it) { it->Animate(currentTickCount); } }// To render the tiles, we call the sceneData.Render().
There are probably further steps, depending on the rest of your code :)
