Outpost Monopoly Download

[Vagabond]

This project is turning out to be a lot larger then I was originally expecting since it needs significant refactoring and pulling out of bugs. But this is okay because I'm learning a lot about C++ as I go. 

I'm also really excited about how HiIdiot fused Outpost and Monopoly. It is cool to drive your ConVec around to build houses/hotels and drive your scout onto waypoints to purchase properties. Using ore instead of money works out naturally as well. Using the chat messaging to interact with trading is clunky, but I don't see a way around it. It is also neat to idle buildings to represent them being mortgaged. So melding Outpost 2 and Monopoly together actually works out really well!

I'm working through refactoring the code base now, which will take a fair number of hours. I'll come back to changing out the deprecated functions once things are in a more readable state.

A couple of questions on C++ implementation:

1. Typedef use: For all of the custom defined structures in the code base, the keyword typedef is used as below.

Code: [Select]

typedef struct PlayerDetailsType
{
    short PlayerNumber;
    short Spot;
} PlayerDetails;

typedef struct HouseTriggerType
{
    Trigger HouseBuildTrigger;
    short LotNumber;
} HouseDetails;

Then in practice, the structure is called like this:

Code: [Select]

PlayerDetails PlayerOrder [4];
HouseDetails HouseBuyTriggers [22];

So it looks like he would name every structure CustomNameTYPE and then rename it using a typedef. Then in the code, he would just refer to the new name and not the original of the structure. Is there any reason to do this in practice? My instinct is to just remove all the typedef calls and rename the custom structures to remove the ending word type.

2. Data ranges in C++
In a couple of places short is used on values that clearly do not require the huge range of an int. For example, there are 40 different spaces on the board, so he uses short to represent variables like lotNumber (representing which lot is the player currently on). Since this value and a fair amount of other values marked as short should never be negative and never need to be greater then about 40, I was thinking about switching them all to unsigned __int8. This would take up 1 byte and allow values of 0 to 255. I think this would be equivalent to the byte type in a language like C#.

I also know that with modern computing power, there will unlikely be a problem just leaving them as short or changing them all to be int (4 bytes each). Besides checking to make sure he isn't using -1 to represent null/illegal values when using short, are there any downsides to switching these variables to unsigned __Int8?

[Hooman]

The typedef struct thing is a holdover from C. There's no particularly compelling reason to do that in C++. I never do it. My inclination is also to drop the typedefs and use just the struct name.

You can read more about the typdef struct vs struct differences on StackOverflow.

Basically the reason for the typdefs boils down to different namespaces. C needed to prefix type names with "struct" to access the struct namespace, while C++ will search that namespace automatically. Think declarations such as "struct TypeName variableName;" (from C), as opposed to "TypeName variableName;" (from C++). The typedef just introduces an alias in the global namespace that references the struct, which is only really convenient for C code, which doesn't automatically check the struct namespace.

The difference can also affect forward declares. As an interesting case, consider building a linked list node which contains a pointer to itself. The struct name appears before the start of the struct, and can be used within it (as a pointer, since we don't want infinite recursion from self inclusion), while the typedef name does not become active until after the struct definition is finished, and then the typedef alias is applied. In such a case, you can't use the typedef name for a node to point to itself.

Code: [Select]

typedef struct NodeStructName {
  NodeStructName* next;  // Here NodeStructName is visible, while NodeTypedefName is not
  Data* data;
} NodeTypedefName;

Typically the same name is used in both places

Code: [Select]

typedef struct Name { ... } Name; // Both names are the same

Sometimes people will create an aliased name for an anonymous struct

Code: [Select]

typedef struct { ... } TypedefName; // No struct name

My preference is to simply name the struct, and forget about the typedef alias nonsense. Unless you're writing a library that you might expect to be called from straight C code, I don't see a compelling reason to use typedef.


In regards to the data range sizes, and changing things to use a uint8 datatype, go for it. I avoid short unless there's a really good reason to use it. A short can be slower to access than an int, and padding rules likely means you'll be using the memory of an int anyway. If you can't fit something in a byte, you should probably just use int. Reasons for using short are generally compatibility with an existing data format, packing data in a binary format where it pairs up and aligns nicely, and perhaps use in very large arrays where memory limitations are likely to be a concern (and you know that padding rules won't prevent the elements from packing nicely).

For single byte sized variables (see what I did there?), padding rules might also cause them to take up the size of an int. A slight benefit here is that byte sized variables have no alignment requirements, and being able to pack more of them into the space of an int means greater data savings when doing this. Again though, this is more useful for struct fields or large arrays than free floating variables.

[Vagabond]

Status update on Outpost Monopoly refactoring,

The typedefs have been removed. For data structures, I've made most of the short values into UINT8 values. The source code still compiles and runs fine with these changes. I'm still waffling on the UINT8 vs setting everything as INT. I have a poor handle on casting in C++ and I feel like making them all INT values keeps me out of trouble, but maybe not right answer.

For parsing out messages to initiate trades with other players, I decided to try transforming the char* pulled from the in game chat message into a std::string.

First is a helper function that splits a string into a vector<string> based on a delimiter. I couldn't find a standard library function to do this and didn't want to deal with adding Boost library dependency. If someone knows of a better way, I would be interested. Function is lifted from internet besides some light changes.

Code: [Select]

//https://www.safaribooksonline.com/library/view/c-cookbook/0596007612/ch04s07.html
void splitStringToVector(const string& stringToSplit, char delimiter, vector<string>& splitString) 
{
	string::size_type i = 0;
	string::size_type j = stringToSplit.find(delimiter);

	while (j != string::npos) 
	{
		splitString.push_back(stringToSplit.substr(i, j - i));
		i = ++j;
		j = stringToSplit.find(delimiter, j);

		if (j == string::npos)
		{
			splitString.push_back(stringToSplit.substr(i, stringToSplit.length()));
		}
	}
}

The Hook function checks to see if incoming chat messages are meant to be parsed and consumed . It then farms to message out to the appropriate function. I'm sending both the pointer to the chat text and the split string for parsing because the chat text is actually replaced by a new message and I need to pass a reference/pointer to do this.

Code: [Select]

void Hook(char* chatText, int sourcePlayerNum)
{
	vector<string> splitString;
	string chatString(chatText);
	std::transform(chatString.begin(), chatString.end(), chatString.begin(), toupper);
	splitStringToVector(chatString, ' ', splitString);

	string command = splitString[0];
	if (command.compare("/TRADE") == 0)
	{
		ParseTrade(chatText, sourcePlayerNum, splitString);
	}
	else if (command.compare("/PAY") == 0 && splitString[1].compare("JAIL") == 0)
	{
		ParsePayJail(chatText, sourcePlayerNum, splitString);
	}
	else if (command.compare("/KICK") == 0)
	{
		ParseKick(chatText, sourcePlayerNum, splitString);
	}
	else if (command.compare("/QUIT") == 0)
	{
		ParseQuit(chatText, sourcePlayerNum, splitString);
	}
	else if (command.compare("/START") == 0)
	{
		ParseStart(chatText, sourcePlayerNum, splitString);
	}
	else if (command.compare("/REVIEW") == 0)
	{
		ParseReview(chatText, sourcePlayerNum, splitString);
	}
}

I'll just pull ParseTrade and then the ParseTradeGoOJF as an example, as it is too much code to show all the different parse functions here (I also haven't finished reformatting them all anyways). GoOJF stands for Get Out Of Jail Free. I'm not happy with this acronym in the code, but I don't have a better solution.

Code: [Select]

void ParseTrade(char* chatText, const int sourcePlayerNum, const vector<string>& splitString)
{
	string tradeType = splitString[1];
	if (tradeType.compare("LOT") == 0 || tradeType.compare("PROPERTY") || tradeType.compare("LAND"))
	{
		ParseTradeProperty(chatText, sourcePlayerNum, splitString);
	}
	else if (tradeType.compare("GOOJF") == 0 || tradeType.compare("JAIL"))
	{
		ParseTradeGoOJF(chatText, sourcePlayerNum, splitString);
	}
	else if (tradeType.compare("ORE") == 0 || tradeType.compare("MONEY") || IsNumeric(tradeType))
	{
		ParseTradeOre(chatText, sourcePlayerNum, splitString);
	}
	else
	{
		strcpy(chatText, "Unrecognized trade command.");
		TethysGame::AddGameSound(sndBld_not, -1);
	}
}

Below are helper functions dealing with parsing the message.

I'm not a fan of the format of functions like bool FindPlayersGoOJFCard(const int playerIndex, int& cardIndex). The other ways I know to solve this would be to use try/catch or set the index to -1 if the parse doesn't work out. I'm not really a fan of any of these solutions though, so I just went with setting the value and returning a bool if it is valid value or not.

Code: [Select]

bool IsNumeric(const std::string& input) {
	return std::all_of(input.begin(), input.end(), ::isdigit);
}

bool ParsePlayerIndex(char* chatText, const string& inputString, int& outputInt)
{
	if (!IsNumeric(inputString))
	{
		FormatInputNotNumberResponse(chatText);
		return false;
	}

	outputInt = stoi(inputString);

	if (!IsValidPlayerIndex(outputInt))
	{
		FormatPlayerDoesNotExistResponse(chatText);
		return false;
	}

	return true;
}

bool PlayerActive(const int playerIndex)
{
	for (int i = 0; i < numberActivePlayers; i++)
	{
		if (MonopolyPlayers[i].TurnIndex == playerIndex - 1)
		{
			return true;
		}
	}

	return false;
}

bool FindPlayersGoOJFCard(const int playerIndex, int& cardIndex)
{
	for (int i = 2; i >= 0; i--) //Find an owned GOoJF card
	{
		if (GOOJF[i] == playerIndex) //Make sure it's owned by the sender
		{
			cardIndex = i;
			return true;
		}
	}

	return false;
}

Below is the meat of parsing trade requests for GoOJF cards. Trading Ore (money) and properties would be formatted the same way.

Code: [Select]

void ParseTradeGoOJF(char* chatText, const int sourcePlayerIndex, const vector<string> splitString)
{
	int receivingPlayerIndex; 
	
	if (!ParsePlayerIndex(chatText, splitString[2], receivingPlayerIndex))
	{
		return;
	}

	if (sourcePlayerIndex == receivingPlayerIndex)
	{
		FormatTradeWithSelfResponse(chatText);
		return;
	}

	if (!PlayerActive(receivingPlayerIndex))
	{
		FormatPlayerNoLongerActive(chatText);
		return;
	}

	int goojfIndex;
	if (!FindPlayersGoOJFCard(sourcePlayerIndex, goojfIndex))
	{
		FormatDoNotOwnGetOutOfJailFreeCard(chatText);
		return;
	}

        TradeGOoJF(goojfIndex, receivingPlayerIndex, sourcePlayerIndex, chatText);
}

Okay, if you made it through all of that, I am interested in thoughts/critiques before implementing this all over the code base. Once I nail down the message parsing better, I'll plan to commit what I have back to the repository.

Random other question: is there a preferred guideline for function name capitalization? I'm used to writing the first letter of a function as capital, like ParsePlayerIndex but all the standard library functions in c++ start with lowercase, like stoi or atoi.

[Hooman]

No, I haven't made it through all that. 

I recommend doing the refactoring in stages, with multiple commits. Don't wait to commit all changes at once. Each stage of changes, which still produces working code, should probably be committed. This makes it easier to understand and verify changes. It also makes it easier to back out one particular change if it has an issue.

I personally prefer UpperCamelCase for class and function names, and lowerCamelCase for variable names. I picked up those habits from Windows programming, where Microsoft code follows that convention. The C++ standard library stuff dates back pretty far when the typical convention was nabbrvtedmss. I don't recommend it. There's also the common C and Ruby practice of using snake_case, but I find typing that ways hurts my wrists after a while. Perhaps I just have bad typing habits though.

I'm sending both the pointer to the chat text and the split string for parsing because the chat text is actually replaced by a new message and I need to pass a reference/pointer to do this.

That's a relevant point. Maybe it should go in a comment in the source code.

GoOJF stands for Get Out Of Jail Free.

Thank you very much for explaining that. That might also be a good comment to add. Or perhaps spell it out in full in the code?

I'm not a fan of the format of functions like bool FindPlayersGoOJFCard(const int playerIndex, int& cardIndex). The other ways I know to solve this would be to use try/catch or set the index to -1 if the parse doesn't work out. I'm not really a fan of any of these solutions though, so I just went with setting the value and returning a bool if it is valid value or not.

I hear you. Those do seem to be the common three ways of handling things. Which one you use often depends on context. I don't have any clear guidelines off the top of my head on which way you should write it.

[Vagabond]

Hooman,

Thank you for the feedback. Yeah, it was a lot of code to post. I was trying to catch the background of what I was doing. Thank you for the insight on the standard library. I think part of what makes C++ difficult to learn is all the baggage it carries around due to it's fantastic backwards compatibility. I suppose a newer language like C# or Java doesn't have to worry about this so much.

I jumped into making lots of major changes and didn't approach the refactoring in a way that was revision control friendly. My current copy is now in the repository. I'll try to be better about pushing changes regularly. I'm afraid the scenario is currently more broken then it started (It still compiles, but I have not been thoroughly testing each refactor).

Outpost Monopoly was programmed from a procedural standpoint (no classes). I thought about switching the project over to more of an object oriented solution. After some thought, I realized procedural was working quite well and it would probably be better to stick with this. For any larger project I work on, I always think in Object-Oriented terms, so I think it will be a good exercise to keep with procedural.

On that note, here is the refactored Outpost Dice code.  It is meant to show robo miners on screen in a pattern that represents the pips on a standard 6 sided dice. Perhaps someone else will have need for such Outpost code in the future. 

Outpost Dice!!!

Code: [Select]

namespace OutpostDice
{
	//Dice Pip Index locations
	// 0   4
	// 1 3 5
	// 2   6

	LOCATION relativePipLocs[] =
	{
		LOCATION(0,0),
		LOCATION(0,1),
		LOCATION(0,2),
		LOCATION(1,1),
		LOCATION(2,0),
		LOCATION(2,1),
		LOCATION(2,2)
	};

	int pipsOn1[] = { 3 };
	int pipsOn2[] = { 0, 6 };
	int pipsOn3[] = { 0, 3, 6 };
	int pipsOn4[] = { 0, 2, 4, 6 };
	int pipsOn5[] = { 0, 2, 3, 4, 6 };
	int pipsOn6[] = { 0, 1, 2, 4, 5, 6 };

	void CreateRoboMinerDie(const LOCATION& dieTopLeftLoc, const int pipLocations[], const int pipCount, const int pipSpacing, const int playerIndex)
	{
		Unit roboDie;

		for (int i = 0; i < pipCount; ++i)
		{
			int pipIndex = pipsOn1[i];
			LOCATION roboDieLoc = dieTopLeftLoc + LOCATION(relativePipLocs[pipIndex].x * pipSpacing, relativePipLocs[pipIndex].y * pipSpacing);
			TethysGame::CreateUnit(roboDie, mapRoboMiner, roboDieLoc, playerIndex, mapNone, TethysGame::GetRand(7));
		}
	}

	void CreateRoboMinerDiceRoll(const LOCATION& dieTopLeftLoc, const int pipSpacing, const int rollValue, const int playerIndex)
	{
		LOCATION roboDieLoc = dieTopLeftLoc;
		Unit roboDie;

		switch (rollValue)
		{
		case 1:
			CreateRoboMinerDie(dieTopLeftLoc, pipsOn1, rollValue, pipSpacing, playerIndex);
			return;
		case 2:
			CreateRoboMinerDie(dieTopLeftLoc, pipsOn2, rollValue, pipSpacing, playerIndex);
			return;
		case 3:
			CreateRoboMinerDie(dieTopLeftLoc, pipsOn3, rollValue, pipSpacing, playerIndex);
			return;
		case 4:
			CreateRoboMinerDie(dieTopLeftLoc, pipsOn4, rollValue, pipSpacing, playerIndex);
			return;
		case 5:
			CreateRoboMinerDie(dieTopLeftLoc, pipsOn5, rollValue, pipSpacing, playerIndex);
			return;
		case 6:
			CreateRoboMinerDie(dieTopLeftLoc, pipsOn6, rollValue, pipSpacing, playerIndex);
			return;
		}
	}
}

And here is Outpost Dice being called in code.

Code: [Select]

SCRIPT_API void PlaceDice()
{
    LOCATION dieLocFirst = LOCATION(233 - 1, 112 - 1);
    LOCATION dieLocSecond = LOCATION(247 - 1, 112 - 1);
    int pipSpacing = 4;

    dieValue = 1 + TethysGame::GetRand(6);
    diceTotal = dieValue;

    int turnIndex = MonopolyPlayers[playerTurn].TurnIndex;

    OutpostDice::CreateRoboMinerDiceRoll(dieLocFirst, pipSpacing, dieValue, turnIndex);

    dieValue = 1 + TethysGame::GetRand(6); 
    diceTotal += dieValue;

    OutpostDice::CreateRoboMinerDiceRoll(dieLocSecond, pipSpacing, dieValue, turnIndex);

    CountRolls(); //Check which roll we're on and continue processing
}

There is still a bug in the code causing multiple dice rolls to be made each turn. I thought perhaps the dice code was causing it, but after refactoring, it looks like the problem is related to the triggers involved in calling for dice rolls. I'm also seeing TethysGame::GetRand(6); produces the vast majority of the numbers 2 and 3. Once I get the dice just rolling one time per turn, I'll look into this some more and see what is causing the lack of randomness.

[leeor_net]

After playing for a while, we thing the dice is rolling multiple times per turn and just using the final roll. Hopefully it won't be too far to fix this.

I noticed this myself. I have a sneaking suspicion about what's causing this but until we clean up the code and get a good understanding of what exactly it's trying to do there's no way of really understanding it.

Hooman,

Thank you for the feedback. Yeah, it was a lot of code to post. I was trying to catch the background of what I was doing. Thank you for the insight on the standard library. I think part of what makes C++ difficult to learn is all the baggage it carries around due to it's fantastic backwards compatibility. I suppose a newer language like C# or Java doesn't have to worry about this so much.

Not sure I'd call it fantastic but meh. The current STL and c++11 standards suffice. The legacy stuff, IMHO, should just be dumped but that would literally break decades worth of code.

I jumped into making lots of major changes and didn't approach the refactoring in a way that was revision control friendly. My current copy is now in the repository. I'll try to be better about pushing changes regularly. I'm afraid the scenario is currently more broken then it started (It still compiles, but I have not been thoroughly testing each refactor).

I was wondering why it seemed to be the original. I wanted to compile it and see for myself how it worked but I found an older binary and tested. Still not sure how I feel about it in its original state though it's an interesting idea.

Outpost Monopoly was programmed from a procedural standpoint (no classes). I thought about switching the project over to more of an object oriented solution. After some thought, I realized procedural was working quite well and it would probably be better to stick with this. For any larger project I work on, I always think in Object-Oriented terms, so I think it will be a good exercise to keep with procedural.

I looked over it myself and was floored by what I saw. Saying it was programmed from a procedural standpoint roughly translates to it's just bad code. This isn't to say that procedural code is bad, just that this code is. No offense to the original author (Hidiot I believe) but when I saw the giant nested if's for basic string processing I was blown away. Truly deserving of a submission to The Daily WTF.

On that note, here is the refactored Outpost Dice code.  It is meant to show robo miners on screen in a pattern that represents the pips on a standard 6 sided dice. Perhaps someone else will have need for such Outpost code in the future. 

/* snip */

There is still a bug in the code causing multiple dice rolls to be made each turn. I thought perhaps the dice code was causing it, but after refactoring, it looks like the problem is related to the triggers involved in calling for dice rolls. I'm also seeing TethysGame::GetRand(6); produces the vast majority of the numbers 2 and 3. Once I get the dice just rolling one time per turn, I'll look into this some more and see what is causing the lack of randomness.

I'm not sure how OP2's internal random number generator is -- since we're using VS2015 which implements most of the C++11 standards we can just use the better built in PRNG code. Some quick code (stolen from cppreference):

Code: [Select]

#include <random>
#include <iostream>

int main()
{
	std::random_device rd;
	std::mt19937_64 gen(rd());

	std::uniform_int_distribution<> dis(1, 6);

	// A few random numbers:
	for (int n = 0; n < 10; ++n)
		std::cout << dis(gen) << ' ';

	std::cout << std::endl;

	return 0;
}

I'd like to work with you on this project and get it refactored. I believe it's a worthwhile project and this code could really use the touch of some more experienced programmers. While the implementation is... questionable... the idea is solid.

I'll need to look through your revisions and see if I can understand what's going on and what's what, etc.

[Hooman]

I'm also seeing TethysGame::GetRand(6); produces the vast majority of the numbers 2 and 3. Once I get the dice just rolling one time per turn, I'll look into this some more and see what is causing the lack of randomness.

Thanks for the info on the dice rolling. That makes it easier to understand.

Not sure I'd call it fantastic but meh. The current STL and c++11 standards suffice. The legacy stuff, IMHO, should just be dumped but that would literally break decades worth of code.

I believe that's called D.

I looked over it myself and was floored by what I saw. Saying it was programmed from a procedural standpoint roughly translates to it's just bad code. This isn't to say that procedural code is bad, just that this code is. No offense to the original author (Hidiot I believe) but when I saw the giant nested if's for basic string processing I was blown away. Truly deserving of a submission to The Daily WTF.

...

I believe it's a worthwhile project and this code could really use the touch of some more experienced programmers. While the implementation is... questionable... the idea is solid.

  Ouch! Play nice!

I don't think Hidiot was particularly inexperienced. He seemed to do alright. And consider that level works by hooking the in game chat. That's not exactly a beginner thing to do.

Of course I should mention Arklon and BlackBox sometimes tear apart my code.

I'm not sure how OP2's internal random number generator is -- since we're using VS2015 which implements most of the C++11 standards we can just use the better built in PRNG code.

Keep in mind game sync issues. If you use the built-in random number generator, it already has a shared seed between all client in multiplayer. If you use a custom random number generator, you'll need to consider sync issues, which means setting a shared initial seed, and always calling the random number generator the same number of times, and in the same order on all clients.

[Vagabond]

I think HiIdiot had some great ideas, the source code just wasn't designed with maintainability or for sharing with others. But that is fine as he was working by himself. Cloning Monopoly isn't a trivial project. As Hooman mentioned, it is beyond my skill to hook into existing code like he did with the chat system.

I just uploaded a newer version of Outpost Monopoly to the repository. I fixed a couple of minor errors in some of my refactored methods. I also compiled and ran the scenario. Currently, the first player just continuously roles and moves across the board, so I created a new bug somewhere...

Leeor, the help would be appreciated. I have never collaborated with someone else in C++, so I probably have some funny habits. I'll start to post what part of the code I'm working on so if you do start making changes we can try to avoid working the same pieces and having major subversion diff fights. It would also be helpful to start posting a list of things that need done. Maybe starting a new forum thread or a redmine section, although redmine might be a bit overkill?

Current Plan

My current project is to finish refactoring the code that parses incoming chat messages and separate this code into its own file. I'm leaning towards calling the file MonopolyChatParse.cpp/h. I'll post here when done with that. Next task after that should probably be either writing a couple of short comment paragraphs explaining the source code or refactoring the code that determines the turn flow/order.

Post Build Event Trouble

I cannot get the OutpostMonopoly post build event to work. I'm just trying to copy the map, tech tree, and scenario dll into the Outpost 2 executable folder in the repository for easier testing. This has not been a problem for me in my maps in the past, and I can't track the error down. Below is the actual post build event and the associated compile time error. This code is in the repository, but Use in Build is set to NO since it isn't working. Any help solving would make testing easier on my part. 

Code: [Select]

copy /Y “$(TargetPath)” “..\..\..\..\GameDownload\Outpost2\trunk\$(TargetFilename)”
copy /Y “$(ProjectDir)atwmon.map” “..\..\..\..\GameDownload\Outpost2\trunk\atwmon.map”
copy /Y “$(ProjectDir)monoptech.txt” “..\..\..\..\GameDownload\Outpost2\trunk\monoptech.txt”

Error MSB3073   
The command
"copy /Y “C:\Users\Brett\Documents\Outpost2SVN\LevelsAndMods\trunk\Levels\OutpostMonopoly\Debug\Ml4Mono.dll” “..\..\..\..\GameDownload\Outpost2\trunk\Ml4Mono.dll”
copy /Y “C:\Users\Brett\Documents\Outpost2SVN\LevelsAndMods\trunk\Levels\OutpostMonopoly\atwmon.map” “..\..\..\..\GameDownload\Outpost2\trunk\atwmon.map”
copy /Y “C:\Users\Brett\Documents\Outpost2SVN\LevelsAndMods\trunk\Levels\OutpostMonopoly\monoptech.txt” “..\..\..\..\GameDownload\Outpost2\trunk\monoptech.txt”
:VCEnd" exited with code 1

[leeor_net]

Not sure I'd call it fantastic but meh. The current STL and c++11 standards suffice. The legacy stuff, IMHO, should just be dumped but that would literally break decades worth of code.

I believe that's called D.

Yeah yeah.

I looked over it myself and was floored by what I saw. Saying it was programmed from a procedural standpoint roughly translates to it's just bad code. This isn't to say that procedural code is bad, just that this code is. No offense to the original author (Hidiot I believe) but when I saw the giant nested if's for basic string processing I was blown away. Truly deserving of a submission to The Daily WTF.

...

I believe it's a worthwhile project and this code could really use the touch of some more experienced programmers. While the implementation is... questionable... the idea is solid.

  Ouch! Play nice!

I don't think Hidiot was particularly inexperienced. He seemed to do alright. And consider that level works by hooking the in game chat. That's not exactly a beginner thing to do.

Of course I should mention Arklon and BlackBox sometimes tear apart my code.

I'm not trying to be mean. When you see code like this:

Code: [Select]

if (atoi(argument2) > 0 && atoi(argument2) <= TethysGame::NoPlayers()) //If the target player exists
{
	for (i = 0; i < noActivePlayers; i++)
		if (PlayerOrder[i].IsPlayerNo == atoi(argument2) - 1)
		{
			if (atoi(argument2) != Lands[atoi(argument1)].Owner + 1) //If the target is not the sender
				if (Lands[atoi(argument1)].Owner == sourcePlayerNum) //If the sender owns the lot to be traded
					if (atoi(argument1) > 0 && atoi(argument1) < 40) //If in the list
						if (atoi(argument1) == 5 || atoi(argument1) == 12 || atoi(argument1) == 15 || atoi(argument1) == 25 || atoi(argument1) == 28 || atoi(argument1) == 35) //If Utility/Railroad, simply trade
							TradeLot(atoi(argument1), atoi(argument2) - 1, sourcePlayerNum, chatText);
								else if (Lands[atoi(argument1)].Housing == 0) TradeLot(atoi(argument1), atoi(argument2) - 1, sourcePlayerNum, chatText); //If upgradable lot, check for 0 houses and trade
									else {strcpy(chatText, "Can't trade upgraded lots."); TethysGame::AddGameSound (sndBld_not, -1);}
							else {strcpy(chatText, "Not a lot."); TethysGame::AddGameSound (sndBld_not, -1);}
						else {strcpy(chatText, "You don't own that lot."); TethysGame::AddGameSound (sndBld_not, -1);}
					else {strcpy(chatText, "Can't trade to yourself."); TethysGame::AddGameSound (sndBld_not, -1);}
				targetPlayerFound = true;
			break;
} else;

You have to take a step back and realize that there has to be a better way to handle this.

I'm not saying I'm superior -- I write bad code. With bugs! It's part of being trapped in this primitive primate brain. But I can take one look at this and thing there has to be a better way. When you start seeing comments that explain what the code is doing instead of why you know you have a problem. When you see magic numbers, you know you have a problem. When you see very deeply nested code, you know you have a problem. When you see all three together, it's time to refactor. Also the coding style is a little wonky -- but that seems to be in an effort to reduce the number of physical lines this block takes up.

I see the same sort of thing a few times and it makes me think that a lot of these can probably be broken into seperate functions that can be used in place of super deep nesting.

But I haven't yet taken the time to fully understand this -- I'm just glossing over it at this point.

I'm not sure how OP2's internal random number generator is -- since we're using VS2015 which implements most of the C++11 standards we can just use the better built in PRNG code.

Keep in mind game sync issues. If you use the built-in random number generator, it already has a shared seed between all client in multiplayer. If you use a custom random number generator, you'll need to consider sync issues, which means setting a shared initial seed, and always calling the random number generator the same number of times, and in the same order on all clients.

I had not considered this. Not sure why it never occurred to me.

I think HiIdiot had some great ideas, the source code just wasn't designed with maintainability or for sharing with others. But that is fine as he was working by himself. Cloning Monopoly isn't a trivial project. As Hooman mentioned, it is beyond my skill to hook into existing code like he did with the chat system.

Again had not considered this. My statement about bad code isn't a reflection on what I think of the original author. Just of the code itself. When I'm doing something super quick and not intending to release for someone else to maintain or something I just need to get working, I bang out some truly heinous code.

I just uploaded a newer version of Outpost Monopoly to the repository. I fixed a couple of minor errors in some of my refactored methods. I also compiled and ran the scenario. Currently, the first player just continuously roles and moves across the board, so I created a new bug somewhere...

Whoops.

Leeor, the help would be appreciated. I have never collaborated with someone else in C++, so I probably have some funny habits. I'll start to post what part of the code I'm working on so if you do start making changes we can try to avoid working the same pieces and having major subversion diff fights. It would also be helpful to start posting a list of things that need done. Maybe starting a new forum thread or a redmine section, although redmine might be a bit overkill?

I would consider this large enough of a project to justify creating a Redmine project. Tasks can be created, updated, discussed and expanded upon and tracked. Users will also be able to post bugs as the project further develops.

[Hooman]

Error MSB3073   
The command
"copy /Y “C:\Users\Brett\Documents\Outpost2SVN\LevelsAndMods\trunk\Levels\OutpostMonopoly\Debug\Ml4Mono.dll” “..\..\..\..\GameDownload\Outpost2\trunk\Ml4Mono.dll”
copy /Y “C:\Users\Brett\Documents\Outpost2SVN\LevelsAndMods\trunk\Levels\OutpostMonopoly\atwmon.map” “..\..\..\..\GameDownload\Outpost2\trunk\atwmon.map”
copy /Y “C:\Users\Brett\Documents\Outpost2SVN\LevelsAndMods\trunk\Levels\OutpostMonopoly\monoptech.txt” “..\..\..\..\GameDownload\Outpost2\trunk\monoptech.txt”
:VCEnd" exited with code 1

What's curious about this error message, is it's one error message for three commands. Perhaps it's trying to execute the "copy copy copy" command?

Code: [Select]

    <PostBuildEvent>
      <Command>copy /Y “$(TargetPath)” “..\..\..\..\GameDownload\Outpost2\trunk\
$(TargetFilename)”
copy /Y “$(ProjectDir)atwmon.map” “..\..\..\..\GameDownload\Outpost2\trunk\atwmo
n.map”
copy /Y “$(ProjectDir)monoptech.txt” “..\..\..\..\GameDownload\Outpost2\trunk\mo
noptech.txt”</Command>
      <Message>Copy Level DLL, Map File and Tech Tree file to Outpost 2 game dir
ectory in SVN. NOTE: Remaked out by default.</Message>
    </PostBuildEvent>

Maybe see if there is a way to break that up into three separate commands. You can also drop the destination file names and supply the destination folder only. The default destination file name will match the source file name. Another option is to copy all the files with one command.

Code: [Select]

copy file1 file2 file3 destFolder

See Copy Command for more interesting stuff. Funny to think, but after all these years, I just learned a thing or two about the copy command from reading that. Like copying from the clipboard to a file, and appending files. 

I have never collaborated with someone else in C++, so I probably have some funny habits. I'll start to post what part of the code I'm working on so if you do start making changes we can try to avoid working the same pieces and having major subversion diff fights.

You can work on different sections of the code, or you can work at different times of the day. Just make sure not to have non-committed changed when you stop working. Dev shops with remote workers in different time zones can effectively get 24-hours/day of development time. Think how much that can speed up development.

I'm not saying I'm superior ...

Based on your choice of language, you kind of did.

[lordpalandus]

Looks like prototype code to me. Of course its not going to look pretty or be designed for maintainability.

[Vagabond]

Hey everyone,

I've been plugging away at this. A little slower lately but still plodding along. I had a couple of questions. I'll keep the code snippet short this time. 

Transferring a std::string to a char*
Once complete parsing and acting on a chat message in the game, a response is formed that replaces the original chat message's text. I'm looking for a little feedback if this seems like a reasonable way to push the string back into a char*. The function below is called after the parsing is completed and the request is checked to make sure it is valid. (IE valid player indexes, lot index, and no houses/hotels on lot). Similar code to this will replace many of the deprecated functions currently in use for working with char*.

Code: [Select]

void TradeLot(const int lotIndex, const int targetPlayerIndex, const int sourcePlayerIndex, char* chatMessage)
{
    ChangeLotOwner(lotIndex, targetPlayerIndex);

    string successMessage = "Player " + std::to_string(sourcePlayerIndex) + 
			    " traded lot " + std::to_string(lotIndex) + 
			    " to player " + std::to_string(targetPlayerIndex);

    std::vector<char> charVector(successMessage.begin(), successMessage.end());
    charVector.push_back('\0');

    chatMessage = &charVector[0];
}

General C++ Using const on arguments passed as values
Just a quick best practice question. Looking at the code above, does it make since to mark up the int arguments (or other non reference values) passed into a function as const to formally state they will not be changed? I am aware when operating on a value passed into a function, it will not change the value in the originator's scope since a local copy is made for the function.

So putting const on the integer values will throw a compile error telling me that I am locally changing a value when I marked the value as constant. I guess I see this as maybe catching errors on occasion, but I also feel like it clutters up the function's declaration when the person using the function doesn't have to worry about the code changing their copy of the value anyways.

Post Build Event Woes Continue
Hooman, thank you for the tips on the build events. I played around for a while longer on the post build events. Switching to xcopy started successfully copying the map and tech file. Unfortunately, I still cannot figure out why the actual DLL will not copy over, the most important file to copy.

Code: [Select]

xcopy "$(ProjectDir)atwmon.map" "..\..\..\..\GameDownload\Outpost2\trunk\" /Y
xcopy "$(ProjectDir)monoptech.txt" "..\..\..\..\GameDownload\Outpost2\trunk\" /Y
rem xcopy “$(TargetPath)” “..\..\..\..\GameDownload\Outpost2\trunk\” /Y

Thanks for the help everyone has/is providing.

[leeor_net]

std::string offers a method to do exactly that. You can simply call the c_str() method:

http://www.cplusplus.com/reference/string/string/c_str/

If you need a non-const string, there are two possibilities. Either you can copy the const char* array to a non-const char* array... or you can cast the constness away using const_cast. E.g.:

Code: [Select]

void TradeLot(const int lotIndex, const int targetPlayerIndex, const int sourcePlayerIndex, char* chatMessage)
{
    ChangeLotOwner(lotIndex, targetPlayerIndex);

    string successMessage = "Player " + std::to_string(sourcePlayerIndex) + 
			    " traded lot " + std::to_string(lotIndex) + 
			    " to player " + std::to_string(targetPlayerIndex);

    chatMessage = const_cast<char*>(successMessage.c_str());
}

Both methods, however, suffer from a dangling pointer problem. In this case we're passing a reference/pointer to a char* array that ultimately is freed due to loss of scope: when the function returns, the destructor of successMessage is called and the string is destroyed. This leaves the chatMessage pointer pointing to a segment of memory that has been freed. At best you won't see anything happen. At worst, you'll get a memory access exception thrown. These kinds of bugs are generally what are used by malicious users to execute arbitrary code.

Anyway, I'm not sure what the structure of the game is, does it use char* arrays everywhere? This should be converted to using std::string's instead which are automatically freed when they go out of scope.

If it's because the game itself requires char* arrays, I would still recommend using std::string's can calling std::string.c_str() where the code interfaces with the game. As stated before, if the game needs non-const strings, const_cast<char*>() is an effective way of dealing with this despite subverting C++'s casting system.

A better implementation would look something like this:

Code: [Select]

const std::string& TradeLot(int lotIndex, int targetPlayerIndex, int sourcePlayerIndex)
{
    ChangeLotOwner(lotIndex, targetPlayerIndex);

    string successMessage = "Player " + std::to_string(sourcePlayerIndex) + 
			    " traded lot " + std::to_string(lotIndex) + 
			    " to player " + std::to_string(targetPlayerIndex);

    return successMessage;
}

This implementation eliminates the dangling pointer problem, avoids having to pass around pointers to memory buffers that need to be managed, etc. And, when calling it you could do something like char* = TradeLot(0, 0, 0).c_str(); or however it's used.


Using the const's for parameters passed by value vs. by reference is, for the most part, useless. It was pointed out to me once, however, that it's not useless in the sense that it's documenting. I somewhat disagree with this -- I think it needlessly complicates the parameter list and that if you're messing with these values in your function you're doing it wrong. However it does help to avoid possible subtle bugs if you are playing with the values. Ultimately it's correct but I think generally boils down to a stylistic choice.

[Hooman]

The string formatting in your TradeLot function can be reduced to a single line using snprintf. This can write to the destination buffer directly, so no need to mess with return pointers, or converting between std::string and character arrays. Plus you can set a cap on the destination buffer size, which should be done due to game limitations on the length of such strings.

I never use const for by-value parameters. There's basically no point. The real strength of defining parameters as const is for type checking and optimization between functions, rather than within a function. Putting const on a by-value parameter would only impose limits within a function. How a function works is of course up to that function, so not really needed as part of the signature. It's an implementation detail. In practice, by-value parameters are usually treated as const, but never declared as such. You want to worry about const when you're passing pointer and reference types.

Side note: You can also declare a function as a whole as const, when it is a class (or struct) member function. This means none of the object's variables can be set by that function. It's basically declaring const on the hidden this pointer, something like this:

Code: [Select]

// Const member function
void SomeClass::SomeMethod(const ParamType1* param1, ParamType2 param2) const;
// Roughly equivalent to
void SomeClass_SomeMethod(const SomeClass* this, const ParamType1* param1, ParamType2 param2);

Also note that if you declare a class instance as const, you can only call const member functions on it.

Xcopy has more error codes. If the post-built step shows an error number, you can look up what it means.

This should be converted to using std::string's instead which are automatically freed when they go out of scope.

You realize you say this just after showing how the automatic cleanup leads to a dangling pointer problem, and undefined behaviour.

Avoid const_cast if you can, or any casting really, unless you know what you're doing. There are very few specific circumstances where they should be used.

[leeor_net]

The string formatting in your TradeLot function can be reduced to a single line using snprintf. This can write to the destination buffer directly, so no need to mess with return pointers, or converting between std::string and character arrays. Plus you can set a cap on the destination buffer size, which should be done due to game limitations on the length of such strings.

I try to avoid the C style functions. They're somewhat more useful and less verbose than using the C++ std:: functions/objects. Personal preference.

This should be converted to using std::string's instead which are automatically freed when they go out of scope.

You realize you say this just after showing how the automatic cleanup leads to a dangling pointer problem, and undefined behaviour.

Avoid const_cast if you can, or any casting really, unless you know what you're doing. There are very few specific circumstances where they should be used.

I modified my post so the ordering may make it seem like that but I'm recommending moving away from C-style strings and instead using std::string's which I find works better and solves a lot of potential bugs.

I would agree with the casts -- the only reason I mention is is because I don't know if the TethysGame functions ask for a const char* or a char*. If they're looking for a char* that would be a reason to const_cast<char*>(). It's not a very clean way to do it and could lead to other potential problems (e.g., if you pass a const_cast'd pointer to the internal char* array in a std::string and the function you're passing it to decides to modify it, that leads to all sorts of potentially serious bugs.

[Vagabond]

Leeor and Hooman,

Thank you for the quick and helpful responses! Leeor, the code hoooking into the Outpost Chat messaging system requires char*, so char* either must be used or transformed to/from string. Thank you for the suggestion on moving the transfer from string to char* out of the TradeLot function. It makes a lot more sense to make this transfer in only one place in the code.

I refactored the Monopoly chat messaging code a lot. I pulled out some snippets below to show what I am doing. Hopefully there is enough supporting code. Sorry for the walls of code again, but I didn't know how to shorten it up.

I think the char* pointer remains in scope in the code below. Please let me know if this is false!

Hooking into the Outpost 2 Messaging System

The code below shows how Outpost Monopoly hooks into Outpost 2's chat system. I'm putting it here for context. I have not touched this block of code from the original implementation. The way this code works is beyond my current understanding of C++ and programming in general.

Code: [Select]

const int returnAddress = 0x40FD8A;
int* const callRelativeAddrPtr = (int*)(returnAddress - 4); /* Look back 4 bytes for the offset part of the call instruction */
void* originalFunctionPtr = 0;

BOOL APIENTRY DllMain(HINSTANCE hinstDLL, DWORD fdwReason, LPVOID lpvReserved)
{
    if (fdwReason == DLL_PROCESS_ATTACH)
    {
        DisableThreadLibraryCalls(hinstDLL);
        InstallMessageHook();
    }

    if (fdwReason == DLL_PROCESS_DETACH)
    {
        UninstallMessageHook();
    }

	return true;
}

void InstallMessageHook()
{
    // Unprotect section of code at <returnAddress>
    DWORD oldAttributes;
    if (VirtualProtect(callRelativeAddrPtr, 4, PAGE_EXECUTE_READWRITE, &oldAttributes) == 0)
    return;   // Could not unprotect pages. Abort

    // Store the old destination of the call instruction
    // Note: The call operand is an offset relative to the start of the next instruction
    originalFunctionPtr = (void*)(*callRelativeAddrPtr + returnAddress);

    // Replace the address of the call instruction with a pointer to our function
    // Note: The call operand is an offset relative to the start of the next instruction
    *callRelativeAddrPtr = (int)((int)&Hook - returnAddress);
}

void UninstallMessageHook()
{
   *callRelativeAddrPtr = (int)originalFunctionPtr - returnAddress;
}

Modified Hook Function

On to the damage I did! I modified the Hook function by adding a try/catch, and adding functions to transfer the char* chatText to and from a string. I also added an enum to represent the different categories of available Monopoly Commands (enum definition not shown).

My hope is to catch most all possible errors while parsing the command and return appropriate messages to the user. Of course someone will likely outsmart me and send a curve ball i didn't expect, so hopefully the try/catch will save the program from crashing. If the Monopoly data is partially modified before the error occurs, this may leave the game in an awkward state. I am pretty new to try/catch in C++, but I think it is right.

If MonopolyChat determines that no command was issued, it just exits and allows the original chat message to run its course through Outpost 2.

Code: [Select]

void Hook(char* chatText, int sourcePlayerIndex)
{
	std::string response;

	try
	{
		vector<string> splitString = CastOutpostChatToVectorString(chatText);

		MonopolyCommand command = MonopolyChat::DetermineChatCommand(splitString);

		if (command == MonopolyCommand::None)
		{
			return;
		}

		response = MonopolyChat::ParseMonopolyCommand(command, sourcePlayerIndex, splitString);
	}
	catch (...)
	{
		response = "Unkown Chat Parse Error";
	}

	if (!response.empty())
	{
		CastOutpostChatToCharStar(response, chatText);
	}
}

How Char*/string are Transformed

I decided to limit the maximum size of a message to 29 characters. This is based on the post about modifying the chat message length by Arkon: http://forum.outpost2.net/index.php/topic,5871.0.html, which I much appreciated having available.

Please pay special attention to how I'm transforming the string back to a char*. I'm really new to pointers, and am unsure that I did this right.

I'm making the chat string all uppercase to make more flexibility in how the command is entered. IE /Trade, /trade, /TRADE and my favorite, /TrAdE all work.

Code: [Select]

vector<string> CastOutpostChatToVectorString(char* chatText)
{
	string chatString(chatText);
	MakeStringUppercase(chatString);

	vector<string> splitString;
	SplitStringToVector(chatString, ' ', splitString);

	return splitString;
}

void CastOutpostChatToCharStar(string& textString, char* textCharStar)
{
	size_t maxMessageStringSize = 29;
	if (textString.length() > maxMessageStringSize)
	{
		textString = "Too many characters in chat";
	}

	TethysGame::AddGameSound(sndBld_not, -1);
	textCharStar = const_cast<char*>(textString.c_str());
}

Okay, assuming this code looks somewhere in the ballpark of what is needed, I'm going to move on to solving the bug causing the first player to take a turn every half second. Once I have this bug resolved, I can actually try testing the MonopolyChat parse code reliably. I'm nervous to work on the code dealing with turn flow. It is pretty complex with the use of a lot of triggers in colorful ways and I haven't sorted through it yet.

[leeor_net]

This implementation eliminates the dangling pointer problem, avoids having to pass around pointers to memory buffers that need to be managed, etc. And, when calling it you could do something like char* = TradeLot(0, 0, 0).c_str(); or however it's used.

Whoopsie, I see my mistake here. Returning a reference to an object that's instantiated and destroyed in the same function.

Code: [Select]

std::string TradeLot(int lotIndex, int targetPlayerIndex, int sourcePlayerIndex)

This would be the correct version albeit inefficient.

Perhaps just passing in std::string& references instead. E.g.

Code: [Select]

void TradeLot(int lotIndex, int targetPlayerIndex, int sourcePlayerIndex, std::string& msg)

But I digress now.


I was going to suggest limiting the length of the messages due to the aforementioned message length limit. I think Arklon was looking into extending this but until that happens it makes sense to keep the messages short.

I haven't looked over all of your code, just briefly skimmed it and it looks a lot better, especially the string parsing. It's a shame C/C++'s string handling mechanisms are so archaic and obscure. I know Boost has some great string manipulation functions but adding Boost functions to this would be... overkill... to say the least. It's a huge library.

Anyway, nice work so far! I'll have some time to look over it this weekend and see what I can help with.


Okay, I'm taking a closer look at what you posted and most of it seems pretty good. I like that you're transforming the string before checking it (I usually do to lowercase but it doesn't matter in this case, the point is that it's normalized).

The funciton names are still throwing me off but minor point here, not enough to really be concerned about.

You have a function called 'CastOutpostChatToVectorString'. You're not actually casting anything here as far as I can tell, a better name for this function would be 'TokenizeOutpostChatString' as that's what you're effectively doing -- breaking the string down into tokens based on a delimiter (in this case a space ' ').

You can probably avoid instantiating a string in your 'CastOutpostChatToVectorString' function. A good C++ compiler should be able to create a std::string for you like this:

Code: [Select]

vector<string> CastOutpostChatToVectorString(char* chatText)
{
	vector<string> splitString;
	SplitStringToVector(MakeStringUppercase(chatText), ' ', splitString);

	return splitString;
}

[Vagabond]

I finally got the entire post build event to work! Something to do with the keywords $(TargetPath) and $(TargetFilename) was causing the problem. After removing these keywords, it started working. This is great news because I can now build the scenario and immediately start Outpost 2 and step debug through the scenario without leaving Visual Studio. I was a little worried that hitting a breakpoint would desync a multiplayer match, but after testing, the breakpoint worked and the match continued afterwords. The fixed post build segment is below.

Code: [Select]

xcopy "$(ProjectDir)Debug\Ml4Mono.dll" "..\..\..\..\GameDownload\Outpost2\trunk\" /Y

@Leeor_net,
I think you are right about the function name being misleading. CastOutpostChatToVectorString doesn't mention anything about changing all the characters in the string to uppercase. I wasn't sure whether to call switching between char* and std::string a cast or not. I guess from how I'm reading your comments, it isn't actually casting.

I'm lamenting the loss of my beginner c++ book. I probably need to buy a new reference book and make sure it covers both string and char* in depth. I have another book on c++ from about 2001, but it is one of the learn to code in 60 minutes a day books and doesn't go much in depth.

Player Turn Flow
I've been reviewing the code involved in determining the player's flow. I'm thinking about ripping it out and re-writing. This section of code is proving difficult for me to follow. I'll sit on it for another day or so before deciding. I have a general idea of what is happening, but it is difficult to follow the code. The functions are very long and the triggers allow the code to jump to different segments outside of the standard program flow.

[Arklon]

Your CastOutpostChatToCharStar won't work - you set the value of the pointer that is a local variable to the function, not set the value of the external variable provided to it, for that you'd need to use a pointer-to-pointer type.

However, I'd write it more like this.

Code: [Select]

/* Can use any STL container that implements insert(iterator, value), e.g. vector, list, (unordered_)set */
template <class T>
void TokenizeString(const std::string &in, const std::string &delimiters, T &out) {
  std::string::size_type curPosition, lastPosition = 0;

  while (lastPosition < in.length()) {
    curPosition = in.find_first_of(delimiters, lastPosition);
    if (curPosition == std::string::npos) {
      curPosition = in.length();
    }

    if (curPosition != lastPosition) {
      out.insert(out.end(),
        T::value_type(in.data() + lastPosition,
                      static_cast<T::size_type>(curPosition - lastPosition)));
    }

    lastPosition = curPosition + 1;
  }
}

/* std::string can be implicitly constructed from char*
 * Make vector<string> and pass it as the out argument, more efficient than regular returning one
 * Does this really need to be its own function at this point? */
void ProcessChatString(std::string &in, std::vector<std::string> &out) {
  std::transform(in.begin(), in.end(), in.begin(), std::toupper);
  TokenizeString(in, " ", out);
}

/* Again, does this need to be its own function?
 * Just copying string contents because const_cast is sick */
void CopyProcessedStringToChat(const std::string &in, char *out) {
  TethysGame::AddGameSound(sndBld_not, -1);
  strncpy_s(out, 30, in.c_str(), _TRUNCATE);
}

FYI, C++ exceptions are pretty disgusting, to the point where many software companies and major open-source projects ban the use of them, though the STL will throw exceptions unless you use a custom one that doesn't.

[leeor_net]

Not sure why the code syntax highlighter works in some cases and not in others. I'll have to look into that.

Anyway, yes, I'd agree with Arklon on the exceptions. I have a try/catch block in the very beginning of my code (entry point) to catch the few exceptions I may throw but in general I find the system cumbersome and clunky and not very effective. Most of the time when you get an exception thrown you should be barfing anyway so catching them doesn't do much except prevent your program from crashing in some cases but recovering from an exception can be difficult so I personally find it better to just back out entirely.

The only time I really attempt to handle any sort of an exception without crashing is in cases where I'm allocating large chunks of memory (say in the case of a map editor with really really large dimensions). In a case like this a bad_alloc can be thrown which indicates running out of memory but since we know it's possible we can continue running the program and simply alert the user that their map or whatever is too big to fit in available memory.

But again I digress and am getting into hypotheticals.

[Vagabond]

Arklon,

Thank you for the example on how to use strncpy_s, the tip on pointer to pointer, and the templated version of TokenizeString!

Makes my knowledge of C++ feel pretty lacking. I plan to massage the provided code into OutpostMonopoly this weekend.

I am currently refactoring the turn order, but it will likely take a while. I haven't been able to refactor it in small chunks that keep the program working, so I'm making some large changes and the code is currently broken locally as I work through it. I haven't pushed the broken code to the repository.

[Vagabond]

I stalled out on OutpostMonopoly in the beginning of November. I had originally set aside about a week to get code recompiled and the scenario playable. After discovering the bugs in the source code and the deprecated functions being used, I was hoping to be able to make spend some time refactoring the code to be more readable and massage out the problems. This turned into much longer than I was hoping. Due to the way the scenario was programmed, my lack of understanding of how to work with c style strings (char*) and my lack of understanding on hooking into the messaging chat this turned into a major project. I also didn't think about how complex Monopoly by itself is and what a task it would be to review all the code required to get it into Outpost 2.

My current code base is committed to the repository. It compiles and runs, but does not really work at simulating Monopoly. It contains the code Arklon helped re-write for transferring the message char* to std::string and back and passing it to the Outpost Chat hook.

While I am excited by the idea of playing a Monopoly game through Outpost, there are a lot of other things I'm interested in doing within Outpost 2 that would be much smaller time commitments. If I pick it up again, there is still a substantial amount of refactoring to accomplish, and it would probably take me at least a month of dedicated evening work to accomplish.

Leeor, if you are interested in working on this still let me know if I can do anything to spool you up with what I've done.

Thanks Arklon, Hooman, and Leeor for the help you provided pushing the project this far. Also, here is the forum post where Hooman and Hidiot originally worked through how to hook into the chat messages, which could come in handy if it needs troubleshot: http://forum.outpost2.net/index.php/topic,4910.msg72232.html#msg72232 (I stumbled on it when looking up info on OllyDbg). I know at least one other scenario makes use of hooking into the chat program to run scripts, so perhaps that part of the code base should be pulled out and placed in an article in the Wiki for future use as needed.

[leeor_net]

Thanks for all your work!

It occurred to me after I started poking through the code that Monopoly as a set of game rules is rather complex and so the code would necessarily be complex to go with it. I would definitely like to help with this I'm just finding myself stretched a little thing at the moment between work and illness. Seems every year around this time I get really really ill. I'm hoping this year I don't need to make a trip to the hospital.

Anyway, that stated, I'm on the mend and we're almost through the holiday season. After the New Year I'll start having a lot more time as our hours will go back down to normal times (from 95 hours a week to 73) and we'll all have some time to breath.

[Hooman]

Yes, thank you for the work you put into this.