Console game missues

[Ch1156]

Thank you that clears it up much better for me. But I'm still confused as to the purpose of getting the old x and y to begin with.

Here is my updated code just for completeness.

//============================================================================
// Name             : Console Game
// Author           : Chay Hawk
// Version          : 0.1.0.11
// Version Date     : March 29th 2023 @ 11:10 AM
// Date Created     : 
// Lines of Code    : 220
// Description      : 
//============================================================================

#include <iostream>
#include <vector>
#include <string>
#include <map>
#include <random>
#include <utility>

void DirectionalError();

//=========================================================================================================
// TODO
//
// Make a way for the DrawMap function to draw maps of different sizes so when the player transitions
// maps, it can redraw the map and the new transition point. 
//=========================================================================================================

class MapGenerator;

//Can promote to a character class when i learn virtual, then inherit from there.
class Player
{
public:
	Player(char player, int posX, int posY) : mPlayer(player), mPosX(posX), mPosY(posY) {}

	enum class Direction
	{
		UP = 1, DOWN = 2, LEFT = 3, RIGHT = 4
	};

	int GetPositionX() const
	{
		return mPosX;
	}

	int GetPositionY() const
	{
		return mPosY;
	}

	char GetPlayerCharacter() const
	{
		return mPlayer;
	}

	void Movement(Player::Direction choice, MapGenerator& mapGenerator);

private:
	const char mPlayer{ 'O' };
	int mPosX{ };
	int mPosY{ };
};



class MapGenerator
{
public:
	MapGenerator(const std::string& mapName, int mapRows, int mapColumns, char mapTile) :
		mMapName(mapName), mMapRows(mapRows), mMapColumns(mapColumns), mMapTile(mapTile) {}

	//Comment this out for now so we dont mess up the working code while testing
	/*void InitializeMap()
	{
		mGameMap.assign(mMapRows, std::vector<char>(mMapColumns, mMapTile));
	}*/

	/*void DrawMap(int posX, int posY, char player)
	{
		for (int row{}; row < mMapRows; ++row)
		{
			for (int col{}; col < mMapColumns; ++col)
			{

				mGameMap[row][col] = mMapTile;
				mGameMap[posY][posX] = player;

				std::cout << mGameMap[row][col];
			}
			std::cout << '\n';
		}
	}*/


	void InitializeMap(const Player& player)
	{
		mGameMap.assign(mMapRows, std::vector<char>(mMapColumns, mMapTile));
		mGameMap[player.GetPositionY()][player.GetPositionX()] = player.GetPlayerCharacter();
	}

	void DrawMap() const
	{
		for (const auto& row : mGameMap)
		{
			for (const auto& column : row)
			{
				std::cout << column;
			}
			std::cout << '\n';
		}
	}

	void Update(size_t oldX, size_t oldY, size_t newX, size_t newY, char player)
	{
		//std::cout << "old x: " << oldX << " - old Y:" << oldY << '\n';
		mGameMap[oldY][oldX] = mMapTile;
		mGameMap[newY][newX] = player;
	}

	int MaxRows() const
	{
		return mGameMap.size();
	}

	int MaxColumns() const
	{
		return !mGameMap.empty() ? mGameMap[0].size() : 0;
	}

private:
	const std::string mMapName{ "Map Name" };
	int mMapRows{ 5 };
	int mMapColumns{ 5 };
	const char mMapTile{ '+' };
	std::vector<std::vector<char>> mGameMap;
	char mTransitionTile{ 'H' };
};


int main()
{
	Player Hero('O', 7, 5);
	MapGenerator Field("Field", 20, 50, '-');

	Field.InitializeMap(Hero);

	while (true)
	{
		Field.DrawMap();

		std::cout << "X: " << Hero.GetPositionX() << " Y: " << Hero.GetPositionY() << "\n\n";

		std::cout << "What do you want to do?\n\n";

		std::cout << "1) Move Up\n";
		std::cout << "2) Move Down\n";
		std::cout << "3) Move Left\n";
		std::cout << "4) Move Right\n";

		int choice{ };

		std::cin >> choice;
		Hero.Movement(static_cast<Player::Direction>(choice), Field);
	}
}

void Player::Movement(Player::Direction choice, MapGenerator& mapGenerator) 
{
	switch (choice) 
	{
		case Direction::UP:
			if (mPosY) 
			{
				//Initialize and set mPosY to the oldY. It can be used in the update 
				//function
				const auto oldY{ mPosY-- };

				mapGenerator.Update(mPosX, oldY, mPosX, mPosY, mPlayer);
				return;
			}
			break;

		case Direction::DOWN:
			if (mPosY < mapGenerator.MaxRows() - 1) 
			{
				const auto oldY{ mPosY++ };

				mapGenerator.Update(mPosX, oldY, mPosX, mPosY, mPlayer);
				return;
			}
			break;

		case Direction::LEFT:
			if (mPosX) 
			{
				const auto oldX{ mPosX-- };

				mapGenerator.Update(oldX, mPosY, mPosX, mPosY, mPlayer);
				return;
			}
			break;

		case Direction::RIGHT:
			if (mPosX < mapGenerator.MaxColumns() - 1) 
			{
				const auto oldX{ mPosX++ };

				mapGenerator.Update(oldX, mPosY, mPosX, mPosY, mPlayer);
				return;
			}
			break;

		default:
			std::cout << "Invalid Input\n";
			return;
	}

	DirectionalError();
}

void DirectionalError()
{
	std::cout << "Cannot go any further in this direction\n";
}

[seeplus]

But I'm still confused as to the purpose of getting the old x and y to begin with

Old is the current position before the move. That position on the map is reset to the default char in .Update(). Setting the old values has the side-effect of also generating the new position (mPos) via post inc/dec. The new position is set to the player symbol also in .Update().

[Ch1156]

So if we didnt get the old x and y positions then what would happen? would the player go out of the vector bounds when we switch maps?

[salem c]

> So if we didnt get the old x and y positions then what would happen?
This is why you need to have some design.

Who owns the players' position?
- the map
- the player
- something else

Without some idea of who does what and who has what, you're always going to be doing these on the fly rewrites when the first cut of the code was broken.

You can look at a design, ask a question "what happens to the player when I change maps" and know whether it's going to work or not in a few seconds.

Or you spend days writing a bunch of code, only to find out it won't work and have to either delete it or massively hack it.

[Ch1156]

Well I would say the map should keep track of the positions of everything on it. Items, enemies, player etc. That seems to make sense to me, since the map needs to know what's placed on it and where everything is at so it can draw it, but maybe I'm wrong. But I'm just confused as to why I need to get the old cords of the player, I'm sure there's a good reason and I'm just not seeing it. I can understand getting the future cords of a player to see if the move is valid. Unless that's what this code is doing here.

[Peter87]

Note that there are many possible ways to do this. This is just one of them.


What the Update function does is essentially 2 things:

1) It takes the old position of the player as argument and overwrites that position with the default tile value (mMapTile).

2) It takes the new/current position of the player and writes the player's symbol to that position.


Another possibility would be to do more inside the Update function. You could for example let the Update function take a Player and a Direction as arguments and let it update the player's position inside the Update function. You could even let it handle the collision detection if you wanted.

Yet another possibility would be to do less inside the Update function. You could let the Update function take only a position and a character as arguments and it would just write the character to that position. The caller would then instead be responsible for calling the function twice to move the player, once to overwrite the old position and once to write the player to its new position.

The choice depends a bit on what you want the class to represent and be responsible for. Is the map a data structure to store the whole game world, or just the graphical representation of the world, or is it only used for collision detection purposes? A real map does not contain all information so it doesn't necessarily have to do that here either. Your design choices will also affect the names that you give to the class and its member function. A "map" and "map generator" sounds to me like two different things.

[mbozzi]

Another approach:

#include <cassert>
#include <string>
#include <string_view>
#include <iostream>

int constexpr map_w = 10, map_h = 10, map_size = map_w * map_h;
char const map_empty = '-', map_player = 'O';

[[nodiscard]] static constexpr bool in_bounds(int x, int y)
{ return (x >= 0 && x < map_w) && (y >= 0 && y < map_h); }

[[nodiscard]] constexpr static std::int64_t xy(int x, int y)
{
	if (!std::is_constant_evaluated()) assert(in_bounds(x, y));
	return x + std::int64_t{ y } * map_w;
}

static void direction_error()
{ std::cout << "Cannot go any further in this direction\n"; }

[[nodiscard]] static bool obstacle(std::string_view map, int x, int y)
{ return !in_bounds(x, y) || map[xy(x, y)] != map_empty; }

int main()
{
	std::string map( map_size, map_empty );
	int player_x = 7, player_y = 5;

	while (true)
	{
		std::cout << "Player x: " << player_x << " y: " << player_y << '\n';

		for (int y = 0; y < map_h; ++y)
		{
			for (int x = 0; x < map_w; ++x)
			{
				if (x == player_x && y == player_y)
					std::cout << map_player;
				else
					std::cout << map[xy(x, y)];
			}
			std::cout << '\n';
		}

		int input = 0;
		std::cout << "\n1) up\n2) down\n3) left\n4) right\n";
		std::cin >> input;

		switch (input)
		{
		case 1: 
			if (obstacle(map, player_x, player_y - 1)) direction_error(); 
			else player_y--; 
			break;
		case 2: 
			if (obstacle(map, player_x, player_y + 1)) direction_error(); 
			else player_y++; 
			break;
		case 3: 
			if (obstacle(map, player_x - 1, player_y)) direction_error(); 
			else player_x--; 
			break;
		case 4:
			if (obstacle(map, player_x + 1, player_y)) direction_error(); 
			else player_x++; 
			break;
		default: 
			std::cout << "Invalid input\n";
		}
	}
}

[Ch1156]

thank you for your replies i think i understand this a bit better now. Ive already implemented an enemy that randomly moves around the map and created a character class to inherit from, ive run into a few snags but nothing major.

[Ch1156]

So i have an question about my code. I created an enemy and need to draw them at the same time as the player because they werent appearing on the map until i moved my player, and i was struggling with it. I tried this:

void InitializeMap(const std::vector<Character>& character)
		{
			mGameMap.assign(mMapRows, std::vector<char>(mMapColumns, mMapTile));

			for (auto& i : mGameMap)
			{
				for (auto& j : character)
				{
					//why doesnt this work? mGameMap is a 2d vector, i should be able to use
					//the double subscript operators.
					i[j.GetPositionY()][j.GetPositionX()] = j.GetCharacter();
				}
			}
		}

I was messing around with the Chat GPT AI and i asked it if it could help me with this problem, so i posted my code to it and this was its response:

The reason why the double subscript operators i[j.GetPositionY()][j.GetPositionX()] don't work is because the i variable is an element of the outer vector, which is a std::vector<char>. It's not a 2D vector itself, so it doesn't have a subscript operator that takes two arguments.

To fix the issue, you'll need to keep track of the row and column indices separately. You can do this using a nested loop, like this:

void InitializeMap(const std::vector<Character>& characters)
{
    mGameMap.assign(mMapRows, std::vector<char>(mMapColumns, mMapTile));

    for (const auto& character : characters)
    {
        const int row = character.GetPositionY();
        const int col = character.GetPositionX();
        const char ch = character.GetCharacter();

        mGameMap[row][col] = ch;
    }
}

In this updated implementation, we iterate over the characters vector, extract the row, column, and character values for each character, and then use those values to update the corresponding element in the 2D game map vector mGameMap. Note that I've also changed the type of the character parameter to const std::vector<Character>& to avoid unnecessary copying of the input vector.

This code works, i am wondering if this is the correct solution though. I posted my entire program to it and it analized it and it actually helped me fix several issues and listed other problems with it, its actually pretty crazy how smart it is, but its till just an AI so i dont fully trust that its going to give me the best solutions so thats why im asking here.

Here are some things the AI told me after analyzing my code:

There are a few issues that can be seen in the provided code:

The GetPositionX() and GetPositionY() functions in the Character class return the X and Y position of the character, respectively, but the implementation mixes them up. The GetPositionX() function should return mPosX, and the GetPositionY() function should return mPosY.

The DrawObjects function in the MapGenerator class has a logical error. The function first generates random coordinates using rows(mt) and columns(mt), but then it checks if those coordinates are not equal to the player's X and Y positions. However, it should check if the generated coordinates are not equal to the player's X position OR the player's Y position. The correct way to do this would be to use the logical OR operator (||) instead of the logical AND operator (&&).

The Movement function in the Character class takes a Direction parameter, but this is not used in the function body. Instead, the Direction parameter is converted to a string using a switch statement, but the result is not used. It is not clear what the purpose of this switch statement is, but it seems to be unnecessary.

The Update function in the MapGenerator class takes four parameters: oldX, oldY, newX, and newY. However, these parameters are not used in the function body, and instead the GetPositionX() and GetPositionY() functions are called on the Character object to get the old X and Y positions and the GetCharacter() function is called on the Character object to get the new character. It would be more appropriate to pass in the Character object itself as a parameter instead of its individual attributes, since the function already requires a Character object to update the map.

There is an unused variable mTransitionTile in the MapGenerator class.

[Ch1156]

Here is my full code:

//============================================================================
// Name             : Console Game
// Author           : Chay Hawk
// Version          : 0.1.0.19
// Version Date     : March 31th 2023 @ 4:52 PM
// Date Created     : 
// Lines of Code    : 264
// Description      : 
//============================================================================

#include <iostream>
#include <vector>
#include <string>
#include <random>
#include <chrono>

class MapGenerator;

class Character
{
	public:
		Character(const std::string& name, char character, int posX, int posY) : mName(name), mCharacter(character), mPosX(posX), mPosY(posY) {}

		enum class Direction
		{
			UP = 1, DOWN = 2, LEFT = 3, RIGHT = 4
		};

		int GetPositionX() const
		{
			return mPosX;
		}

		int GetPositionY() const
		{
			return mPosY;
		}

		char GetCharacter() const
		{
			return mCharacter;
		}

		std::string GetName() const
		{
			return mName;
		}

		void Movement(Character::Direction choice, MapGenerator& mapGenerator);

	private:
		std::string mName{ "Character" };
		const char mCharacter{ 'C' };
		int mPosX{ };
		int mPosY{ };
};


class Player : public Character
{
public:
	Player(const std::string& name, char character, int posX, int posY) : Character{name, character, posX, posY}
	{}

private:

};

class Enemy : public Character
{
	public:
		Enemy(const std::string& name, char character, int posX, int posY): Character{name, character, posX, posY}
		{}

		void Move(std::mt19937& mt, MapGenerator& mapGenerator)
		{
			//Randomize directions
			std::uniform_int_distribution<> moveEnemy{ 1, 4};

			Movement(static_cast<Character::Direction>(moveEnemy(mt)), mapGenerator);
		}

	private:
};

class MapGenerator
{
	public:
		MapGenerator(const std::string& mapName, int mapRows, int mapColumns, char mapTile) :
			mMapName(mapName), mMapRows(mapRows), mMapColumns(mapColumns), mMapTile(mapTile) {}

		//void InitializeMap(const Character& character)
		//{
		//	mGameMap.assign(mMapRows, std::vector<char>(mMapColumns, mMapTile));
		//	mGameMap[character.GetPositionY()][character.GetPositionX()] = character.GetCharacter();
		//}

		void InitializeMap(const std::vector<Character>& characters)
		{
			mGameMap.assign(mMapRows, std::vector<char>(mMapColumns, mMapTile));

			for (const auto& character : characters)
			{
				const int row{ character.GetPositionY() };
				const int col{ character.GetPositionX() };
				const char ch{ character.GetCharacter() };

				mGameMap[row][col] = ch;
			}
		}

		void DrawMap() const
		{
			for (const auto& row : mGameMap)
			{
				for (const auto& column : row)
				{
					std::cout << column;
				}
				std::cout << '\n';
			}
		}

		void Update(size_t oldX, size_t oldY, size_t newX, size_t newY, char character)
		{
			mGameMap[oldY][oldX] = mMapTile;
			mGameMap[newY][newX] = character;
		}

		size_t MaxRows() const
		{
			return mGameMap.size();
		}

		size_t MaxColumns() const
		{
			return !mGameMap.empty() ? mGameMap[0].size() : 0;
		}

		void DrawObjects(std::mt19937& mt, char object, int amountToPlace, const Character& character)
		{
	
			std::uniform_int_distribution<> rows{0, 19};
			std::uniform_int_distribution<> columns{0, 49};

			for (int i{ }; i < amountToPlace; ++i)
			{
				//Do not draw over player
				if (rows(mt) != character.GetPositionX() && rows(mt) != character.GetPositionY() || columns(mt) != character.GetPositionX() && columns(mt) != character.GetPositionY())
				{
					mGameMap[rows(mt)][columns(mt)] = object;
				}
			}
		}

	private:
		const std::string mMapName{ "Map Name" };
		int mMapRows{ 5 };
		int mMapColumns{ 5 };
		const char mMapTile{ '+' };
		std::vector<std::vector<char>> mGameMap;
		char mTransitionTile{ 'H' };
		char mObject{ '#'};
};


int main()
{
	std::mt19937 mt{ static_cast<unsigned int>(std::chrono::steady_clock::now().time_since_epoch().count()) };

	std::vector<Character> characterContainer;

	Player Hero("Hero", 'O', 5, 5);
	Enemy Goblin("Goblin", 'X', 15, 15);
	MapGenerator Field("Field", 20, 50, '-');

	characterContainer.push_back(Hero);
	characterContainer.push_back(Goblin);

	Field.InitializeMap(characterContainer);
	//Field.InitializeMap(Goblin);
	//Field.DrawObjects(mt, '&', 10, Hero);

	while (true)
	{
		Field.DrawMap();
		Goblin.Move(mt, Field);

		std::cout << "X: " << Hero.GetPositionX() << " Y: " << Hero.GetPositionY() << "\n\n";

		std::cout << "What do you want to do?\n\n";

		std::cout << "1) Move Up\n";
		std::cout << "2) Move Down\n";
		std::cout << "3) Move Left\n";
		std::cout << "4) Move Right\n";

		int choice{ };

		std::cin >> choice;
		Hero.Movement(static_cast<Player::Direction>(choice), Field);
	}
}

void Character::Movement(Character::Direction choice, MapGenerator& mapGenerator)
{
	switch (choice)
	{
		case Direction::UP:
			if (mPosY)
			{
				//Initialize and set mPosY to the oldY. It can be used in the update 
				//function
				const auto oldY{ mPosY-- }; //This

				//Is the same exact thing as this:
				//const auto oldY{ mPosY };
				//mPosY--;

				mapGenerator.Update(mPosX, oldY, mPosX, mPosY, mCharacter);
				return;
			}
			break;

		case Direction::DOWN:
			if (mPosY < mapGenerator.MaxRows() - 1)
			{
				const auto oldY{ mPosY++ };

				mapGenerator.Update(mPosX, oldY, mPosX, mPosY, mCharacter);
				return;
			}
			break;

		case Direction::LEFT:
			if (mPosX)
			{
				const auto oldX{ mPosX-- };

				mapGenerator.Update(oldX, mPosY, mPosX, mPosY, mCharacter);
				return;
			}
			break;

		case Direction::RIGHT:
			if (mPosX < mapGenerator.MaxColumns() - 1)
			{
				const auto oldX{ mPosX++ };

				mapGenerator.Update(oldX, mPosY, mPosX, mPosY, mCharacter);
				mapGenerator.Update(oldX, mPosY, mPosX, mPosY, mCharacter);
				return;
			}
			break;

		default:
			std::cout << "Invalid Input\n";
			return;
	}

	std::cout << "Cannot go any further in this direction\n";
	std::cout << GetName() << " tried to go " << " but couldnt!\n";
}

[Peter87]

Try to understand the problem. That'll make it easier to come up with a solution (or judge if a solution is correct).

for (auto& i : mGameMap)
{
	for (auto& j : character)
	{
		//why doesnt this work? mGameMap is a 2d vector, i should be able to use
		//the double subscript operators.
		i[j.GetPositionY()][j.GetPositionX()] = j.GetCharacter();
	}
}

i is a row in the map.

Ask yourself what the purpose of the outer loop is.

No matter where you get your help, from a bot or a human, you still need to make sure you understand it. If you don't understand it you won't be able to make changes and adjust the code for your needs without asking for more help. You're much more likely to understand a solution that you have created yourself, so don't be so obsessed with finding "the best solution" (it doesn't exist anyway). A solution that you have created and fully understands is much more valuable than a supposedly "better" solution that you don't understand.

[Peter87]

The GetPositionX() and GetPositionY() functions in the Character class return the X and Y position of the character, respectively, but the implementation mixes them up. The GetPositionX() function should return mPosX, and the GetPositionY() function should return mPosY.

I don't see this being a problem in the code that you posted above.

The DrawObjects function in the MapGenerator class has a logical error. The function first generates random coordinates using rows(mt) and columns(mt), but then it checks if those coordinates are not equal to the player's X and Y positions. However, it should check if the generated coordinates are not equal to the player's X position OR the player's Y position. The correct way to do this would be to use the logical OR operator (||) instead of the logical AND operator (&&).

Not sure I understand this function. It's called "DrawObjects" but tries to randomly place objects on the map? I think what the bot is complaining about is that you compare rows(mt) to both x and y coordinates which looks suspicious. Another potential problem is that rows(mt) gives you a new random number each time that expression is executed. Is that really what you want?

The Movement function in the Character class takes a Direction parameter, but this is not used in the function body. Instead, the Direction parameter is converted to a string using a switch statement, but the result is not used. It is not clear what the purpose of this switch statement is, but it seems to be unnecessary.

It's talking nonsense. The Direction is not converted to a string and it is used!

[Ch1156]

The solution it provided was definitely easier to understand than my solution that you quoted for sure. and if i dont understand something i can ask it to go into greater depth and it will explain it to me, its actually quite helpful and its impressive how it knows.

My reasoning is that i is the game map, and its a 2d vector so i should be able to use the double subscript operator, at least thats how i saw it, obviously it was wrong. but the AI's solution is understandable for sure. I see that its looping through the rows and columns and setting all characters i create to their positions allowing them to be drawn on the screen at the start of the game, whereas before the enemies would only appear if i made a move, now the solution works.

[Ch1156]

Not sure I understand this function. It's called "DrawObjects" but tries to randomly place objects on the map? I think what the bot is complaining about is that you compare rows(mt) to both x and y coordinates which looks suspicious. Another potential problem is that rows(mt) gives you a new random number each time that expression is executed. Is that really what you want?

its not, i just want to randomize positions of objects once on the map.

And yeah some of the stuff it says is strange but i believe its suggestion is based on old code had, I update my code in the post above several times so its not the same as when the AI made those suggestions so thats probably why it said that. I deleted some stuff from the movement function.

[Peter87]

My reasoning is that i is the game map, and its a 2d vector so i should be able to use the double subscript operator, at least thats how i saw it, obviously it was wrong.

The problem becomes more obvious if you don't use auto.

for (std::vector<char>& i : mGameMap)
{
  ...
     i[ ... ][ ... ] = ...
  ...
}

[Ch1156]

Yeah i believe the range based for loop syntax messed me up, i shoudl have made a regular for loop and got it working then converted it to a range based once i saw it work correctly.

[Peter87]

What I have been trying to hint at is that you probably don't need this loop at all. It doesn't exist in the bot's solution either. Only the inner loop does. If you want to use mGameMap then just write mGameMap.

[Ch1156]

Something like this?

void InitializeMap(const Character& character)
	{
		mGameMap.assign(mMapRows, std::vector<char>(mMapColumns, mMapTile));
		mGameMap[character.GetPositionY()][character.GetPositionX()] = character.GetPlayerCharacter();
	}

[Peter87]

Yes, except that you now have changed your code back to only use one Character instead of a vector of Characters.

Your version with vector<Character> would with minimal changes look like the following:

void InitializeMap(const std::vector<Character>& character)
{
	mGameMap.assign(mMapRows, std::vector<char>(mMapColumns, mMapTile));
	
	for (auto& j : character)
	{
		mGameMap[j.GetPositionY()][j.GetPositionX()] = j.GetCharacter();
	}
}

Which is exactly the same as the bot's code. The only difference is that the bot's code uses more intermediate variables and different names.

[Ch1156]

Ahhh, ok thank you. What was confusing me is that i knew i needed to get all the characters created and loop through them at once so they would all draw on tyhe map at the same time, so the inner loop was supposed to loop through the character vector passed in, i dont know what i was thinking with that. but your fix makes more sense now that i see it, thank you.