Problem with OO classes
Author
I'm trying to create a Object Oriented OpenGL game using C++ with classes. But the problem is that I don't really know how to design the game. For some reason every class I create have only one instance.
For example, I have a Player class. But this class is more of a Player Manager class because it have a list of all players in the game. Each player have a struct with its values and the Player class have methods such as Create(playerstruct), Destroy(playerstruct) etc.
I also have a Print class that will write text on screen. This class initalize and destroys the text which is kept in memory, which makes it better to just keep one instance of this class to avoid initializing every time I need to write something on screen. But what if I have different classes that all needs to write something on screen? I then have to pass the Print class instance to all those classes.
All these classes makes everything bloated and ugly. What am I doing wrong? Should I just skip the classes things because I don't have anything real use of it?
You'll find a use for classes with multiple instances. Objects, Shaders, Textures, Callbacks, such things.
The examples you gave are well-suited for singletons, however personally I find the singleton approach overrated. Can't say I've ever used it.
In my projects, whenever I have these kinds of 'global' functionalities, I just dump them in a namespace instead of a class:
So that I can use:
Namespaces still modularize the content, it shows what's going on, can be accessed from anywhere, and there's no need to pass pointers around.
Some people might say "well, what if you want to print in another way, you could've used a print interface and made a different printer instance, etc etc". I just ignore them [smile]. This way works for me.
The examples you gave are well-suited for singletons, however personally I find the singleton approach overrated. Can't say I've ever used it.
In my projects, whenever I have these kinds of 'global' functionalities, I just dump them in a namespace instead of a class:
namespace Console{ void Write(const char* format, ...); void Initialize(); void Uninitialize();}So that I can use:
Console::Write("Blah blah: %s\n", message);Namespaces still modularize the content, it shows what's going on, can be accessed from anywhere, and there's no need to pass pointers around.
Some people might say "well, what if you want to print in another way, you could've used a print interface and made a different printer instance, etc etc". I just ignore them [smile]. This way works for me.
If the Player class doesn't actually represent a player, why did you name it that way?
You should create a player class that represents one player. Each player will store the data you are currently storing in the player struct, along with whatever member functions it needs. Then most of the code in Create()/Destroy() will move to the Player's constructor and destructor, respectively.
You can also have an std::vector of Players, and use that as the player list. The Players don't need to know that they are stored in a list (it makes the class more flexible), so the code to add and remove players from the list should be outside of the player class.
Passing around an instance of Print shouldn't be that bad because you should only have a handful of classes that print text on the screen. If you have many, you should probably rethink your design. One idea is for a class to return a string of what it wants printed, and have some other class actually print it. This will minimize the number of classes that need access to Print.
Since I'm not very experienced with this, that's all I'll say for now (I don't want to accidentally give you bad advice), but if you have any more questions, Ill try my best to answer (I really like these types of threads and sometimes can't resist replying. [smile])
You should create a player class that represents one player. Each player will store the data you are currently storing in the player struct, along with whatever member functions it needs. Then most of the code in Create()/Destroy() will move to the Player's constructor and destructor, respectively.
You can also have an std::vector of Players, and use that as the player list. The Players don't need to know that they are stored in a list (it makes the class more flexible), so the code to add and remove players from the list should be outside of the player class.
Passing around an instance of Print shouldn't be that bad because you should only have a handful of classes that print text on the screen. If you have many, you should probably rethink your design. One idea is for a class to return a string of what it wants printed, and have some other class actually print it. This will minimize the number of classes that need access to Print.
Since I'm not very experienced with this, that's all I'll say for now (I don't want to accidentally give you bad advice), but if you have any more questions, Ill try my best to answer (I really like these types of threads and sometimes can't resist replying. [smile])
In C++ structs and classes are almost the same, except that structs normally have public members. Structs can even have constructors and methods. So your player structs are in fact classes that don't have any methods (at least I assume so).
There's nothing wrong with having a PlayermManager that holds a list of Players.
Also, passing the Print class around would not be that bad. Just think about if all those classes really need to print something themselves.
You also could have a Game/Engine class that holds a pointer to the print class and pass that around. Then, when you need the Print class, simply say game->getPrinter()->print(...);
There's nothing wrong with having a PlayermManager that holds a list of Players.
Also, passing the Print class around would not be that bad. Just think about if all those classes really need to print something themselves.
You also could have a Game/Engine class that holds a pointer to the print class and pass that around. Then, when you need the Print class, simply say game->getPrinter()->print(...);
Quote:Original post by Mike nl
You'll find a use for classes with multiple instances. Objects, Shaders, Textures, Callbacks, such things.
The examples you gave are well-suited for singletons, however personally I find the singleton approach overrated. Can't say I've ever used it.
In my projects, whenever I have these kinds of 'global' functionalities, I just dump them in a namespace instead of a class:namespace Console{ void Write(const char* format, ...); void Initialize(); void Uninitialize();}
So that I can use:Console::Write("Blah blah: %s\n", message);
Namespaces still modularize the content, it shows what's going on, can be accessed from anywhere, and there's no need to pass pointers around.
Some people might say "well, what if you want to print in another way, you could've used a print interface and made a different printer instance, etc etc". I just ignore them [smile]. This way works for me.
class Console {public: Console() { // Initialize } ~Console() { // Uninitialize }};...extern Console console;You encapsulate the local data, get implicit construction/destruction, and allow creation of multiple instances.
At worst, in order to prevent initialization order problem, you construct the console instance at same point where you'd call Initialize function.
Destruction is taken care of. Even more:
void foo(int x, Console & c = console){ c.write("Hello World");}...foo(20);// let's write to error streamConsole anotherConsole(std::cerr); foo(42, anotherConsole);All the convenience, all the benefits. And yes, the default argument could also be a singleton, without enforcing the limitations:
void foo(int x, Console & c = Console::getInstance()){ c.write("Hello World");}Quote:For example, I have a Player class. But this class is more of a Player Manager class because it have a list of all players in the game. Each player have a struct with its values and the Player class have methods such as Create(playerstruct), Destroy(playerstruct) etc.
Create/Destroy in OO are realized as constructor/destructor.
Calling Player something that is PlayerManager also doesn't help with simplicity.
Author
Thanks alot for all the help.
I don't really have any code. More like empty classes. This is how my design looks like now:
main.cpp - Only have WinMain method.
globals.h - Have all the important includes that are used on many classes like <windows.h>
Input Class - Class to set and get inputs.
Window Class - Initialize and destroys the win32 window. Uses Input instance to update the input from example WM_MOUSEMOVE.
OpenGL class - Initialize and destroys OpenGL. Uses HWND handle from window class as input.
Print Class - Print message to screen.
World Class - Loads and draws the current map.
PlayerManager Class - Holds a list of all the players. (Renamed it to PlayerManager)
Menu Class - Creates a window box in-games for use as menu.
Game Class - Creates one instance of Input, Window and OpenGL. Creates one instance of the World Class, Player Class and Menu Class.
I haven't got further than this. About the print class, I'm not sure where to use it. I was thinking maybe the Game class needs it to print messages on screen, but the Menu class also needs it to print messages on the menu.
I'm not entirely sure how to make the menu class. I will be using several menus in the game, such as game menu, inventory, player stats etc. So I was thinking to make the Menu class general for all menus. But should I then have a new class that draws the contents of each window?
I think I'll do like Gage64 said about Player class. So the Game class will hold a list of all Player classes, and each player have their own create method etc.
This is hard. I'm probably missing alot of classes. Should I have a Object class and a Texture class that the Player class uses? Does anyone have a good example how a game design looks like with all the classes and what class uses what.
I don't really have any code. More like empty classes. This is how my design looks like now:
main.cpp - Only have WinMain method.
globals.h - Have all the important includes that are used on many classes like <windows.h>
Input Class - Class to set and get inputs.
Window Class - Initialize and destroys the win32 window. Uses Input instance to update the input from example WM_MOUSEMOVE.
OpenGL class - Initialize and destroys OpenGL. Uses HWND handle from window class as input.
Print Class - Print message to screen.
World Class - Loads and draws the current map.
PlayerManager Class - Holds a list of all the players. (Renamed it to PlayerManager)
Menu Class - Creates a window box in-games for use as menu.
Game Class - Creates one instance of Input, Window and OpenGL. Creates one instance of the World Class, Player Class and Menu Class.
I haven't got further than this. About the print class, I'm not sure where to use it. I was thinking maybe the Game class needs it to print messages on screen, but the Menu class also needs it to print messages on the menu.
I'm not entirely sure how to make the menu class. I will be using several menus in the game, such as game menu, inventory, player stats etc. So I was thinking to make the Menu class general for all menus. But should I then have a new class that draws the contents of each window?
I think I'll do like Gage64 said about Player class. So the Game class will hold a list of all Player classes, and each player have their own create method etc.
This is hard. I'm probably missing alot of classes. Should I have a Object class and a Texture class that the Player class uses? Does anyone have a good example how a game design looks like with all the classes and what class uses what.
Why don't you create a class to represent the player as well?
Also, like I said, you should give the Player class a constructor instead of having a create function. Same thing for the destructor.
Can you post some of the code for the PlayerManager class? (at the very least, what member functions it has) Also, can you show how you are using the player struct? It can help explain the benefits of upgrading the struct to a full blown class.
Finally, you might want to take a look at some of the articles in the first link in my sig. In particular, the one called "The Single-Responsibility Principle".
Also, like I said, you should give the Player class a constructor instead of having a create function. Same thing for the destructor.
Can you post some of the code for the PlayerManager class? (at the very least, what member functions it has) Also, can you show how you are using the player struct? It can help explain the benefits of upgrading the struct to a full blown class.
Finally, you might want to take a look at some of the articles in the first link in my sig. In particular, the one called "The Single-Responsibility Principle".
Author
Here is the code for PlayerManager class. I keep the list of all players on the Game class now. The loading is not finished, I will probably change it so it loads data from a file instead or something.
PlayerManager.cpp:
PlayerManager.h:
Game.cpp where I use the PlayerManager. For now I'm only using one player.
[Edited by - RandomPixel on July 3, 2008 5:38:05 AM]
PlayerManager.cpp:
#include "PlayerManager.h"#include <gl/gl.h>#include <gl/glu.h>/*****************************************************************************************************************************************/void CPlayerManager::Load(stObject *p_pObject, float *p_pData, int p_nSize){ // Clear old values if(p_pObject->pVertices !=NULL) delete p_pObject->pVertices; if(p_pObject->pColors != NULL) delete p_pObject->pColors; if(p_pObject->pTexCoords != NULL) delete p_pObject->pTexCoords; // Create arrays p_pObject->pVertices = new stVertex[p_nSize]; p_pObject->pColors = new stColor[p_nSize]; p_pObject->pTexCoords = new stTexCoord[p_nSize]; // Set data int nPos = 0; for(int i = 0; i < p_nSize; i++) { p_pObject->pVertices.x = p_pData[nPos++]; p_pObject->pVertices.y = p_pData[nPos++]; p_pObject->pVertices.z = p_pData[nPos++]; p_pObject->pColors.r = p_pData[nPos++]; p_pObject->pColors.g = p_pData[nPos++]; p_pObject->pColors.b = p_pData[nPos++]; p_pObject->pTexCoords.t = p_pData[nPos++]; p_pObject->pTexCoords.u = p_pData[nPos++]; }}/*****************************************************************************************************************************************/void CPlayerManager::Draw(stObject *p_pObject){ if(p_pObject != NULL) { glTranslatef(p_pObject->stPos.x, p_pObject->stPos.y, p_pObject->stPos.z); glRotatef(p_pObject->nAngle, p_pObject->stRotate.x, p_pObject->stRotate.y, p_pObject->stRotate.z); glVertexPointer(3, GL_FLOAT, 0, p_pObject->pVertices); glColorPointer(3, GL_FLOAT, 0, p_pObject->pColors); glTexCoordPointer(2, GL_FLOAT, 0, p_pObject->pTexCoords); glDrawArrays(GL_QUADS, 0, 4); }}/*****************************************************************************************************************************************/void CPlayerManager::Delete(stObject *p_pObject){ if(p_pObject != NULL) { delete p_pObject->pVertices; delete p_pObject->pColors; delete p_pObject->pTexCoords; }}PlayerManager.h:
#ifndef __GENERAL_PLAYERMANAGER_H#define __GENERAL_PLAYERMANAGER_H#include "globals.h"struct stVertex { float x, y, z; };struct stColor { float r, g, b; };struct stTexCoord { float t, u; };struct stObject{ stVertex stPos; stVertex stRotate; float nAngle; stVertex *pVertices; stColor *pColors; stTexCoord *pTexCoords; stObject() { pVertices = NULL; pColors = NULL; pTexCoords = NULL; }};class CPlayerManager{// Methodspublic: void Load(stObject *pObject, float *pData, int nSize); void Draw(stObject *pObject); void Delete(stObject *pObject);// Membersprivate:};#endifGame.cpp where I use the PlayerManager. For now I'm only using one player.
#include "Game.h"#include <wchar.h>/*****************************************************************************************************************************************/CGame::CGame(){ m_pInterface = new CInterface(); m_pGraphics = new CGraphics(); m_pInput = new CInput(); m_pPlayerManager = new CPlayerManager(); m_pFPS = new CFPS();}/*****************************************************************************************************************************************/CGame::~CGame(){ delete m_pFPS; delete m_pPlayerManager; delete m_pInput; delete m_pGraphics; delete m_pInterface;}/*****************************************************************************************************************************************/int CGame::Create(void){ // Create the system if(!m_pInterface->Create()) return(0); if(!m_pInput->Create()) return(0); if(!m_pGraphics->Create(m_pInterface->m_hWnd)) return(0); // Create the tools m_pFPS->Create(); // Create the player static float g_fTestObject[] = { 1.0f, -1.0f, 0.0f, 1.0f, 0.0f, 0.0f, 1.0f, 0.0f, -1.0f, -1.0f, 0.0f, 1.0f, 1.0f, 1.0f, 0.0f, 0.0f, -1.0f, 1.0f, 0.0f, 1.0f, 1.0f, 1.0f, 0.0f, 1.0f, 1.0f, 1.0f, 0.0f, 0.0f, 0.0f, 1.0f, 1.0f, 1.0f, }; m_pPlayerManager->Load(&m_stPlayer, g_fTestObject, 4); m_stPlayer.stPos.x = 0.0f; m_stPlayer.stPos.y = 0.0f; m_stPlayer.stPos.z = -6.0f; m_stPlayer.nAngle = 0.0f; m_stPlayer.stRotate.x = 1.0f; m_stPlayer.stRotate.y = 0.0f; m_stPlayer.stRotate.z = 0.0f; return(1);}/*****************************************************************************************************************************************/int CGame::DisplayMode(void){ // Destroy the old display settings m_pGraphics->Destroy(); m_pInterface->Destroy(); // Change mode m_pInterface->SetDisplayMode(); // Create a new display settings if(!m_pInterface->Create()) return(0); if(!m_pGraphics->Create(m_pInterface->m_hWnd)) return(0); return(1);}/*****************************************************************************************************************************************/int CGame::Update(void){ // Quick exit game if(m_pInput->GetKey(KEY_ESC)) return(0); // Change display mode if(m_pInput->GetKey(KEY_ALT) && m_pInput->GetKey(KEY_ENTER)) { if(!DisplayMode()) return(0); } // Set to desktop mode on Window key if(m_pInput->GetKey(KEY_LEFTWIN)) { if(m_pInterface->GetDisplayMode() == 1) { if(!DisplayMode()) return(0); } } // Draw the scene m_pGraphics->Clear(); m_stPlayer.nAngle += 0.5f; m_pPlayerManager->Draw(&m_stPlayer); m_pGraphics->Render(); // Display FPS in title m_pFPS->Count(); wchar_t szFPS[20]; swprintf(szFPS, L"%2.0f", m_pFPS->m_fFramesPerSecond); wchar_t *szTitle = new wchar_t[strlen("Game Engine")+10]; swprintf(szTitle, L"Game Engine (%s FPS)", szFPS); SetWindowText(m_pInterface->m_hWnd, szTitle); delete[] szTitle; // Make sure to give resources to windows Sleep(1); return(1);}/*****************************************************************************************************************************************/void CGame::Destroy(void){ // Destroy the player m_pPlayerManager->Delete(&m_stPlayer); // Destroy the system m_pInput->Destroy(); m_pGraphics->Destroy(); m_pInterface->Destroy();}[Edited by - RandomPixel on July 3, 2008 5:38:05 AM]
Now I really recommend that you read the article I mentioned.
There are two things that define an object: It's data, and it's behavior (it's member functions). In this case you have separated those two things into two classes: stObject (I would lose the st prefix, it doesn't convey any useful information), which holds the data, and PlayerManager, which does the operations. This separation is confusing and pointless.
Unify these two into one class. Also, replace the Load() function with a constructor and the Delete() function with a destructor.
The article I mentioned basically says that each class should have only one responsibility. In this case, all the code you've shown basically represents a 3D model. If you put it in the Player class, the class will have two responsibilities: handle 3D models, and handle player functionality (what that is depends on you game). Doing this has several disadvantages that are explained in the article.
To fix this, put all the code you posted in a Model class. This class has only one responsibility: to draw a 3D model. Then when you create your Player class, it will just store an instance of the Model class and use it to handle the drawing. Thus, this responsibility has been removed from the Player class.
Doing this is sometimes called "delegation" because the Player class delegates a responsibility to the Model class.
BTW, calling delete on a NULL pointer does nothing, so there's no need to check if a pointer is NULL before calling delete on it.
EDIT: One last note - give your classes meaningful names. Like I explained, PlayerManager represents (the operations of) a 3D model, it has nothing to do with managing players. Besides, using Manager in a class' name is usually a bad idea because it doesn't have a clear meaning. It also often implies that the class does too many things and should be broken into separate classes (like described above).
There are two things that define an object: It's data, and it's behavior (it's member functions). In this case you have separated those two things into two classes: stObject (I would lose the st prefix, it doesn't convey any useful information), which holds the data, and PlayerManager, which does the operations. This separation is confusing and pointless.
Unify these two into one class. Also, replace the Load() function with a constructor and the Delete() function with a destructor.
The article I mentioned basically says that each class should have only one responsibility. In this case, all the code you've shown basically represents a 3D model. If you put it in the Player class, the class will have two responsibilities: handle 3D models, and handle player functionality (what that is depends on you game). Doing this has several disadvantages that are explained in the article.
To fix this, put all the code you posted in a Model class. This class has only one responsibility: to draw a 3D model. Then when you create your Player class, it will just store an instance of the Model class and use it to handle the drawing. Thus, this responsibility has been removed from the Player class.
Doing this is sometimes called "delegation" because the Player class delegates a responsibility to the Model class.
BTW, calling delete on a NULL pointer does nothing, so there's no need to check if a pointer is NULL before calling delete on it.
EDIT: One last note - give your classes meaningful names. Like I explained, PlayerManager represents (the operations of) a 3D model, it has nothing to do with managing players. Besides, using Manager in a class' name is usually a bad idea because it doesn't have a clear meaning. It also often implies that the class does too many things and should be broken into separate classes (like described above).
Author
Thanks, It makes better sense. I have read the article (couldn't find it in the link you provided, but found it via google).
I guess I'm used to create big classes that does everything. The reason I created that struct, was because I thought it would be easier to make a list of that struct, which only contains variables, instead of the whole class with all its methods.
I like your method much better. So I will create a Model class and a Player class. How should they interact? Should I have another class that binds those two like the article suggested that you can do? I mean for example the Player location will probably be used in both the Model class and the Player class.
I guess I'm used to create big classes that does everything. The reason I created that struct, was because I thought it would be easier to make a list of that struct, which only contains variables, instead of the whole class with all its methods.
I like your method much better. So I will create a Model class and a Player class. How should they interact? Should I have another class that binds those two like the article suggested that you can do? I mean for example the Player location will probably be used in both the Model class and the Player class.
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
