Adding A DozeHelper header/cpp file to OP2Helper

[Vagabond]

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.

Code: [Select]

#pragma once

#include "Outpost2DLL\Outpost2DLL.h"

bool CanTileBeDozed(LOCATION location);
void DozeLocation(LOCATION loc);
void DozeRectangle(MAP_RECT mapRect);

Code: [Select]

#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);
		}
	}
}

[Hooman]

I like this idea.

A couple suggestions.
- Mark the tile index ints as const.

Code: [Select]

const int FirstRockTileIndex = 439;
const int FirstSandTileIndex = 1206;

- Mark the parameters as const references.

Code: [Select]

bool CanTileBeDozed(const LOCATION &location);
void DozeLocation(const LOCATION &loc);
void DozeRectangle(const MAP_RECT &mapRect);

- Add comments for terrain type:

Code: [Select]

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.

[Vagabond]

@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?

Code: [Select]

#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);
}

Code: [Select]

#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);
			}
		}
	}
}

[Hooman]

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:

Code: [Select]

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.

Code: [Select]

LOCATION dozeLoc(mapRect.x1 + x, mapRect.y1 + y);

[Hooman]

I had an additional thought. Rather than using comments, why not use named constants:

Code: [Select]

	GameMap::SetTile(loc, 414); // Dozed Mud
	GameMap::SetTile(loc, 921); // Dozed Rock
	GameMap::SetTile(loc, 1670); // Dozed Sand

Code: [Select]

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.

[Vagabond]

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.

Code: [Select]

LOCATION dozeLoc = LOCATION(mapRect.x1 + x, mapRect.y1 + y); //WRONG! :(

[Hooman]

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.

In particular, from the example there is the line:

Code: [Select]

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.

[Vagabond]

I went ahead and committed Bulldozer.h and Bulldozer.cpp into the repo with changes discussed. If anyone wants different nomenclature in the future, just let me know since I'll have to update the projects I'm working on.

I accidentally committed changes to ColonyType.h and ColonyType.cpp at the same time, without the updates discussed in the other post. I don't want to try to revert the commit since it still compiles and works fine unless someone wants me to. Still plan to update ColonyType in the near future as discussed in other post.

[Hooman]

Are you including the new header files directly from your own project? Hmm, it looks like you are. I suggest you include the new file from the main OP2Helper.h. If it's not included from the main header file, there's little difference to just putting the code in it's own separate library. Having the main OP2Helper.h include the additional file will isolate changes to the filename to within the library.

Don't worry about accidentally checked in changes. It happens, especially at first. You can try doing a rollback if you want (which is actually a new commit that undoes the undesired changes from the older commit), or you can just finish your second set of changes and commit them on top.

I often make two unrelated sets of changes, which I want to commit separately, but forget about one of them. That's particularly the case with small one line comment typo fixes that I just happen to spot while working on something else. Generally I like to commit such fixes separately from what I'm working on. It's easy to forget though, so I've gotten into the habit of always checking SVN status and SVN diffs. The TortoiseSVN commit screen shows a status summary of files, and I always go through and double click each selected file to view the diff. It's the only way I remember about those pesky unrelated comment updates. When the unrelated change is the only change in a file, it's easy enough to just uncheck the file from the commit screen, but it's a bit of a pain when it's in the same file as another change. For that I tend to make a backup copy the file elsewhere to preserve the changes for later, and then from the diff screen undo the changes I don't want in the commit, do the commit, and then copy the backup file back to restore the remaining changes. With Git there is a staging area, so the process is a little more integrated.


Note: There is a distinction between "if", "only if", and "if and only if" (sometimes abbreviated as "iff").
if: A -> B  (A implies B)
only if: A <- B  (B implies A)
iff: A <-> B  (A is equivalent to B)

[Vagabond]

I was thinking to give more fine grained choice of which header files to include in a project. I do see how placing it as a subheader of OP2Helper.h could insulate breaking other projects if the filename changes in the future.

Modified the repository to place Bulldozer.h as an include in OP2Helper.h.

I'll hold off on the rollback since my mistake didn't actually break anything (that I'm aware of anyways). I am fairly familiar with Mercurial but am still learning Subversion. I found Mercurial more intuitive than Git, but didn't play with Git long enough to give it a real chance. Git also seems to be significantly more popular than Mercurial.

[Hooman]

I was looking at the following code, and had an idea to improve readability:

Code: [Select]

	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:

Code: [Select]

	if (originalTileIndex <= LastMudTileIndex)
	{
		GameMap::SetTile(loc, DozedMudTileIndex);
	}
	else if (originalTileIndex <= LastRockTileIndex)
	{
		GameMap::SetTile(loc, DozedRockTileIndex);
	}
	else
	{
		GameMap::SetTile(loc, DozedSandTileIndex);
	}

[Vagabond]

Would it make more sense to switch the constants to start with a lowercase letter as well?

Code: [Select]

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);
}

[Vagabond]

Went ahead and pushed this change into the repository.

[leeor_net]

Sweet!

I really wish I had more time to look at this and help out. Well, more like I wish I wasn't always so burned out that I end up just staring at youtube and netflix on the few days off I get a month.

But super awesome!

I want to see about putting together a single downloadable package with all of this together. Probably the subject of a different discussion.

In the mean time, do you think you'd have time to put together usage samples on the Wiki? One of the goals for the wiki is to provide tutorials and coding examples and such and this and the other things you've worked on I think are prime things to work with for setting up a development section.

[Vagabond]

Leeor,

I'm pushing changes to OP2Helper into the repository as we go along, so if you repackage everything into a zip file from there, it will be current. We fixed the repository's level templates to properly handle a fixed DLL base address and statically link to the helper projects. I noticed when using the original package you put out of the SDK that they were not set properly (probably because the repository's were not set correctly that you pulled from).

I think there is a balance to strike between making it easy for a new comer to get programming new scenarios quickly and encouraging them to link properly to the repository and commit their custom scenarios to it.

[Hooman]

Would it make more sense to switch the constants to start with a lowercase letter as well?

I have no real opinion on this. I tend to use UpperCamelCase for const values, though have at times used lowerCamelCase. These days the IDE generally highlights or provides popup information to let you know if something is constant or not, so there's less reason to mark something as const with a naming convention. I don't see a particular reason why constants should be named differently than variables. Consistency is most important, so if there's a pre-established convention in a project, generally just stick with that. ...unless you really don't like the pre-established convention    (SHOUTY_CASE, I'm looking at you!)