Looking at adding another header and cpp file to OP2 Helper. Code allows you to set tiles or MAP_RECT to robo-dozed by setting both the LOCATION's cell type and tileIndex.. Code checks first if space is passable before dozing on it and then selects the correct color of dirt/mud/rock for the dozed look. This way you can select an area with a rock/cliff inside, and it will not doze away the impassable terrain.
It is useful during a scenario setup so you don't have to force part of the map to be dozed via setting it in the mapper.
Unsure if I should just wrap the code in a namespace or place it in a static class. In C# I would always place code within a class.
#pragma once
#include "Outpost2DLL\Outpost2DLL.h"
bool CanTileBeDozed(LOCATION location);
void DozeLocation(LOCATION loc);
void DozeRectangle(MAP_RECT mapRect);
#include "DozeHelper.h"
int FirstRockTileIndex = 439;
int FirstSandTileIndex = 1206;
bool CanTileBeDozed(LOCATION location)
{
int currentCellType = GameMap::GetCellType(location);
return
currentCellType == CellTypes::cellFastPassible1 ||
currentCellType == CellTypes::cellFastPassible2 ||
currentCellType == CellTypes::cellMediumPassible1 ||
currentCellType == CellTypes::cellMediumPassible2 ||
currentCellType == CellTypes::cellSlowPassible1 ||
currentCellType == CellTypes::cellSlowPassible2;
}
void DozeLocation(LOCATION loc)
{
if (!CanTileBeDozed(loc))
{
return;
}
int originalTileIndex = GameMap::GetTile(loc);
if (originalTileIndex < FirstRockTileIndex)
{
GameMap::SetTile(loc, 414);
}
else if (originalTileIndex < FirstSandTileIndex)
{
GameMap::SetTile(loc, 921);
}
else
{
GameMap::SetTile(loc, 1670);
}
GameMap::SetCellType(loc, CellTypes::cellDozedArea);
}
void DozeRectangle(MAP_RECT mapRect)
{
for (int y = 0; y < mapRect.Height(); ++y)
{
for (int x = 0; x < mapRect.Width(); ++x)
{
LOCATION dozeLoc = LOCATION(mapRect.x1 + x, mapRect.y1 + y);
DozeLocation(dozeLoc);
}
}
}
I like this idea.
A couple suggestions.
- Mark the tile index ints as const.
const int FirstRockTileIndex = 439;
const int FirstSandTileIndex = 1206;
- Mark the parameters as const references.
bool CanTileBeDozed(const LOCATION &location);
void DozeLocation(const LOCATION &loc);
void DozeRectangle(const MAP_RECT &mapRect);
- Add comments for terrain type:
GameMap::SetTile(loc, 1670); // Sand (or whatever is appropriate, I didn't check)
The reference makes parameter passing faster, and avoids needless object copies. You don't notice it at the source code level, but it adds a lot of noise at the assembly level. Adding const to the parameters (that are now references) allows variables that are themselves declared as const to be passed to the function. The location of a player base is likely to be declared as const. Normal variables can of course still be passed to a const parameter. The const just means the compiler will guarantee there are no writes to that parameter.
It might also be nice if people could comment on the suggested names (function names, parameter names, file names), to make sure they are clear, and fit in with the other names in the project. This can always be changed after an initial commit, but as names affect source code compatibility, particularly the function names, this should be reviewed before people start using the additions.
Taking a quick look at existing names, I see a few boolean valued functions named "Is...". Maybe "IsBulldozable" would fit better? I'm not sure I love the name. Perhaps you'll have a better idea.
You can potentially make use of function overloading and name both DozeLocation and DozeRectangle as simply "Doze". The different parameter types will be enough to distinguish them. Using clear variable names can also make the intent pretty clear. For example "Doze(baseRect)"/"Doze(baseArea)", or "Doze(meteorImpactPoint)".
Probably more consistent to strip "Helper" from the file names. That's used for the project name and main include file, but not the other include files.
@Hooman,
Thank you for the insights. Code is pasted below with changes. I Also placed the code within a namespace. I think the names Bulldozer.h & Bulldozer.cpp might be a little better than DozeHelper, although I'm not firm on the new name.
I am happy to change names based on other's input, just not sure the best way to do this. For now, I could just let this post simmer for a few days before adding to the code base in case anyone would like to make inputs on preferred names.
I thought it was preferred to make copies of small structures such as a point (LOCATION) or rectangle (MAP_RECT) since they are cheap to make. I guess anything larger than a pointer would be more efficient to pass as a reference?
#pragma once
#include "Outpost2DLL\Outpost2DLL.h"
namespace Bulldozer
{
// Checks if a LOCATION on the map can be bulldozed.
bool IsDozeable(const LOCATION &loc);
// Dozes a LOCATION only if LOCATION is valid to doze.
void Doze(const LOCATION &loc);
// Dozes all LOCATIONs contained in MAP_RECT that are valid to doze.
void Doze(const MAP_RECT &mapRect);
}
#include "Bulldozer.h"
namespace Bulldozer
{
const int FirstRockTileIndex = 439;
const int FirstSandTileIndex = 1206;
bool IsDozeable(const LOCATION &location)
{
int currentCellType = GameMap::GetCellType(location);
return
currentCellType == CellTypes::cellFastPassible1 ||
currentCellType == CellTypes::cellFastPassible2 ||
currentCellType == CellTypes::cellMediumPassible1 ||
currentCellType == CellTypes::cellMediumPassible2 ||
currentCellType == CellTypes::cellSlowPassible1 ||
currentCellType == CellTypes::cellSlowPassible2;
}
void Doze(const LOCATION &loc)
{
if (!IsDozeable(loc))
{
return;
}
int originalTileIndex = GameMap::GetTile(loc);
if (originalTileIndex < FirstRockTileIndex)
{
GameMap::SetTile(loc, 414); // Dozed Mud
}
else if (originalTileIndex < FirstSandTileIndex)
{
GameMap::SetTile(loc, 921); // Dozed Rock
}
else
{
GameMap::SetTile(loc, 1670); // Dozed Sand
}
GameMap::SetCellType(loc, CellTypes::cellDozedArea);
}
void Doze(const MAP_RECT &mapRect)
{
for (int y = 0; y < mapRect.Height(); ++y)
{
for (int x = 0; x < mapRect.Width(); ++x)
{
LOCATION dozeLoc = LOCATION(mapRect.x1 + x, mapRect.y1 + y);
Doze(dozeLoc);
}
}
}
}
You bring up a good point with the namespaces. I don't think the SDK really addressed namespaces.
The filename shouldn't be too big of a deal. It will likely be included from the main library header, rather than directly from user's of the library, so it could be updated later without breaking code.
For passing parameters any bigger than an intrinsic type, I generally prefer a reference. Even two element structs can benefit from passing a reference. If you turn on assembly output and compile some code that passes a LOCATION struct by value, and by reference, you'll see quite a difference in the number of instructions at the points of call.
I also just noticed something:
LOCATION dozeLoc = LOCATION(mapRect.x1 + x, mapRect.y1 + y);
Technically this code constructs a temporary LOCATION object, and then copies it into the variable. You can instead construct the value in place. Likely, the compiler will do this for you anyway. It's an easy and common enough optimization. Constructing in place is shorter though, and requires less repetition of the data type.
LOCATION dozeLoc(mapRect.x1 + x, mapRect.y1 + y);
I had an additional thought. Rather than using comments, why not use named constants:
GameMap::SetTile(loc, 414); // Dozed Mud
GameMap::SetTile(loc, 921); // Dozed Rock
GameMap::SetTile(loc, 1670); // Dozed Sand
const int dozedMud = 414;
const int dozedRock = 921;
const int dozedSand = 1670;
...
GameMap::SetTile(loc, dozedMud);
GameMap::SetTile(loc, dozedRock);
GameMap::SetTile(loc, dozedSand);
That would probably be better. It's clear and the constants can potentially be used elsewhere.
Errrr, I thought the code below was how you constructed an object in place in C++, probably since it is exactly how it is done in C#. Should have read through constructors a little closer... I'll have to work through the code I've written so far and fix.
I have some code written up for placing common/rare rubble on the map working very similar to this doze function. I'll post it soon, but perhaps it would be better if they live in the same namespace. I'm not sure we want a namespace containing only 3 bulldozing function. Perhaps some sort of MapManipulationDuringScenarioInitialization namespace.
LOCATION dozeLoc = LOCATION(mapRect.x1 + x, mapRect.y1 + y); //WRONG! :(
The code is by no means wrong, and should compile down to the same machine code. There's just a shorter more direct way.
As for why the code should compile to the same thing, you can read about Copy Elision (http://en.cppreference.com/w/cpp/language/copy_elision).
In particular, from the example there is the line:
std::vector<Noisy> v = std::vector<Noisy>(3); // copy elision from temporary to v
What's of particular note about this optimization is the following comment:
compilers are permitted to omit the copy- and move-constructors of class objects even if copy/move constructor and the destructor have observable side-effects.
This point is further emphasized in the Notes section, which starts with this interesting point:
Copy elision is the only allowed form of optimization that can change the observable side-effects.
Optimizations are optional. Not all compilers perform this optimization. Compilers that do, might not do it for all build settings (such as a debug build). Changing compilers or build settings can cause programs to generate different output depending on whether or not this optimization is done. This can be seen by placing code with observable effects in one of the affected functions. Logging to the console is a typical example of an observable side-effect.
I was looking at the following code, and had an idea to improve readability:
if (originalTileIndex < FirstRockTileIndex)
{
GameMap::SetTile(loc, DozedMudTileIndex);
}
else if (originalTileIndex < FirstSandTileIndex)
{
GameMap::SetTile(loc, DozedRockTileIndex);
}
else
{
GameMap::SetTile(loc, DozedSandTileIndex);
}
Instead of FirstRockTileIndex, we could use something like LastMudTileIndex instead, and change the < to <=.
That would read as:
if (originalTileIndex <= LastMudTileIndex)
{
GameMap::SetTile(loc, DozedMudTileIndex);
}
else if (originalTileIndex <= LastRockTileIndex)
{
GameMap::SetTile(loc, DozedRockTileIndex);
}
else
{
GameMap::SetTile(loc, DozedSandTileIndex);
}
Would it make more sense to switch the constants to start with a lowercase letter as well?
const int lastMudTileIndex = 438;
const int lastRockTileIndex = 1205;
const int dozedMudTileIndex = 414;
const int dozedRockTileIndex = 921;
const int dozedSandTileIndex = 1670;
void Doze(const LOCATION& loc)
{
if (!IsDozeable(loc))
return;
int originalTileIndex = GameMap::GetTile(loc);
if (originalTileIndex <= lastMudTileIndex)
{
GameMap::SetTile(loc, dozedMudTileIndex);
}
else if (originalTileIndex <= lastRockTileIndex)
{
GameMap::SetTile(loc, dozedRockTileIndex);
}
else
{
GameMap::SetTile(loc, dozedSandTileIndex);
}
GameMap::SetCellType(loc, CellTypes::cellDozedArea);
}