Static pointers to classes and delete
Author
Hi! I'm making a tycoon-like game and I'm currently using a stack-based approach to manage the game states. Basically there's an abstract base class called CGmSt which all the rest of the game state classes inherit from. Every inherited class has a static pointer to itself and a protected constructor. There's also a public method called GetInstancePtr which allocates that pointer if it's the first time the function is being called and returns that pointer for every call afterwards. That pointer is deleted inside the Shutdown method.
There's algo a CgmStMngr class, which mantains the stack and has some functions such as PushState (which calls the Initialize method of the state that is being pushed) and PopState (which calls the Shutdown method of the state that is being popped). Every game state holds a pointer to the CGmStMngr.
Today, I was working with the code and realized that there might be one case where this stack-based approach could fail. Let me explain: suppose that program execution is inside CSomeStateGmSt::Initialize() and CGmStMngr::PopState() is called. As a result of that, CSomeStateGmSt::Shutdown() would be called, deleting its static pointer. After the call to CSomeStateGmSt::Shutdown() returns, is the stack frame from CSomeStateGmSt::Initialize() still valid? So far, everything seems to be working OK, but I would like to ensure things are not going to blow up :)
Thanks in advance for your answers,
--Nacho
Hi,
Do you mean you're using the singleton pattern everywhere?
Also, a part of the question isn't clear to me: are you talking about multithreading? When you're in the Initialize() function of SomeState, do you mean that PopState will be called by another thread? If not, why would you call PopState from the Initialize function of your state?
Do you mean you're using the singleton pattern everywhere?
Also, a part of the question isn't clear to me: are you talking about multithreading? When you're in the Initialize() function of SomeState, do you mean that PopState will be called by another thread? If not, why would you call PopState from the Initialize function of your state?
Author
Quote:Original post by Lode
Hi,
Do you mean you're using the singleton pattern everywhere?
For the game states, yes, I'm using singletons. The game states manager is not a singleton. There's only one instance of it inside the CGameEngine class and every game state holds a pointer to that instance.
Quote:Original post by Lode
Also, a part of the question isn't clear to me: are you talking about multithreading? When you're in the Initialize() function of SomeState, do you mean that PopState will be called by another thread? If not, why would you call PopState from the Initialize function of your state?
No, I'm not using multithreading. As a matter of fact, I mistyped something in my original post since I'm actually calling PopState inside CSomeStateGmSt::Update(), not CSomeStateGmSt::Initialize() as I've originally stated. Nonetheless, the example is still valid. Suppose that CSomeStateGmSt::Update() is done with its processing and we need to get back to the previous state, would the call to CGmStMngr::PopState() render CSomeStateGmSt::Update()'s stack frame invalid?
--Nacho
Quote:Original post by Ignacio Liverotti
Basically there's an abstract base class called CGmSt.
There's algo a CgmStMngr class.
Ouch. Seriously, buy a vowel.
Your problem seems to stem from an over-use of a half-hearted singleton. I'd suggest making game states non-singletons and just creating and deleting them on demand (such as new-ing them before pushing them as state, and holding them as a smart pointer so they get deleted when popped). Alternatively if your states have values which persist between usages then create them all at startup in a higher level class, push and pop them as you please with no allocation/deletion, and finally delete them all on game shutdown.
Deleting while still updating as state is going to blow up in odd ways, but it might not crash and instead just start scribbling over unallocated memory. Alternatively keep a list of states to be popped and only pop them after you've updated everything. Or if a state has to pop itself, then have it indicate this via a return code from Update().
Quote:Original post by Ignacio LiverottiI wd lk to hlp, bt yr cls nms r to dfclt to rd :-|
Hi! I'm making a tycoon-like game and I'm currently using a stack-based approach to manage the game states. Basically there's an abstract base class called CGmSt which all the rest of the game state classes inherit from. Every inherited class has a static pointer to itself and a protected constructor. There's also a public method called GetInstancePtr which allocates that pointer if it's the first time the function is being called and returns that pointer for every call afterwards. That pointer is deleted inside the Shutdown method.
There's algo a CgmStMngr class, which mantains the stack and has some functions such as PushState (which calls the Initialize method of the state that is being pushed) and PopState (which calls the Shutdown method of the state that is being popped). Every game state holds a pointer to the CGmStMngr.
Today, I was working with the code and realized that there might be one case where this stack-based approach could fail. Let me explain: suppose that program execution is inside CSomeStateGmSt::Initialize() and CGmStMngr::PopState() is called. As a result of that, CSomeStateGmSt::Shutdown() would be called, deleting its static pointer. After the call to CSomeStateGmSt::Shutdown() returns, is the stack frame from CSomeStateGmSt::Initialize() still valid? So far, everything seems to be working OK, but I would like to ensure things are not going to blow up :)
Thanks in advance for your answers,
--Nacho
j/k :)
This is just me, but I'd probably take a different approach.
I'm not quite clear on all the details of how your system works, but it sounds like:
1. Each game state type is a singleton, more or less (is that right?)
2. The individual game states have direct access to the object that manages them
3. The game states are created and destroyed via the initialization and shut down methods
I'm not sure I understand the purpose of the singleton approach in this case, but regardless, IMO the individual states should not have access to the 'game state stack' object that manages them (following the general principle that objects should not know about the containers in which they reside, as this creates an unnecessary dependency).
The other problem is that of pushing and popping states inside state initialization or update functions. I'm not sure of the exact answer to the question you posed, but I think it might be better to handle all pushing and popping in a single location in the code, preferably at the beginning or end of the update cycle. This will prevent problems such as the 'what if I pop the state stack while a state is updating?' problem.
The question remains, of course, of how to communicate to the game state stack that it needs to 'do something'.
Although my own game state stack system is certainly not without flaws, software-design wise, it does address some of the issues under discussion here.
First of all, states are not singletons or anything of that sort - they are just objects pushed onto and popped off of the state stack.
The game states themselves certainly need to be able to request that a state be pushed or popped (they might 'pop themselves', request that a new menu state be pushed onto the stack, or what have you), but this is achieved via messaging rather than through direct access to the game state stack.
These 'push/pop request' messages are collected elsewhere in the application, and then at the beginning of the update loop they are processed in sequence. If you want to be able to push and pop inside game state constructors or destructors, you can buffer the commands so that any such requests will not be processed until the next update cycle.
Anyway, that would be a pretty drastic architectural change, and may not be something you want to take on, but at the very least I would take another look at the use of singletons, and at the relationship between the game states objects and the game state stack.
[Edit: slow...]
So you're in a function of an object of class CSomeStateGmSt, and then you delete the object?
AFAIK, that's similar to doing "delete this", and then in the rest of the Update function you may not access any of the object's data members or virtual functions anymore, but apart from that the function will execute just fine (since you can't access the datamembers, you won't be able to do much anymore, so unless your call to the deleting function is at the end of Update, it's going to give problems).
I don't really understand why you'd create and delete singletons all the time for a stack of states though. How exactly does that work?
AFAIK, that's similar to doing "delete this", and then in the rest of the Update function you may not access any of the object's data members or virtual functions anymore, but apart from that the function will execute just fine (since you can't access the datamembers, you won't be able to do much anymore, so unless your call to the deleting function is at the end of Update, it's going to give problems).
I don't really understand why you'd create and delete singletons all the time for a stack of states though. How exactly does that work?
Author
Quote:Original post by OrangyTangOuch. Seriously, buy a vowel.
I originally used lots of vowels, but when I started having statements such as the following in my code
m_lpSPMGameStateNewGame->m_lpSPMGameStateMngr->ChangeState(CSPMGmStWorldMap::GetInstancePtr());I decided that having an acronym list would be a much better idea [lol]
Quote:Original post by OrangyTangI'd suggest making game states non-singletons and just creating and deleting them on demand (such as new-ing them before pushing them as state, and holding them as a smart pointer so they get deleted when popped).
They are being created and deleted on demand, notice that GetInstancePtr allocates a new instance of the game state and Shutdown frees it. I realize that the singleton approach is weird, though, so I'm gonna change that. My problem is that I'm not sure what happens with the stack frame of a game state's method if the pointer to that game state is deleted inside that method.
--Nacho
Author
Wow, lots of replies! [smile]
Ahh, that's what I needed to know. Thanks Lode.
Yes, OrangyTang made me realize it's a pretty bad approach, so I'm gonna change it.
@ jyk
Your suggestion has been very helpful. Although I prefer to leave the messaging system for my next project, I think I can add some kind of return code such as GM_ST_POP_ST [grin] to my game states and check for that in the game's main loop in order to avoid pushing and popping of states inside the update function.
Thank you all for your replies,
--Nacho
Quote:Original post by Lode
So you're in a function of an object of class CSomeStateGmSt, and then you delete the object?
AFAIK, that's similar to doing "delete this", and then in the rest of the Update function you may not access any of the object's data members or virtual functions anymore, but apart from that the function will execute just fine (since you can't access the datamembers, you won't be able to do much anymore, so unless your call to the deleting function is at the end of Update, it's going to give problems).
Ahh, that's what I needed to know. Thanks Lode.
Quote:Original post by LodeI don't really understand why you'd create and delete singletons all the time for a stack of states though. How exactly does that work?
Yes, OrangyTang made me realize it's a pretty bad approach, so I'm gonna change it.
@ jyk
Your suggestion has been very helpful. Although I prefer to leave the messaging system for my next project, I think I can add some kind of return code such as GM_ST_POP_ST [grin] to my game states and check for that in the game's main loop in order to avoid pushing and popping of states inside the update function.
Thank you all for your replies,
--Nacho
Quote:Original post by Ignacio LiverottiQuote:Original post by OrangyTangOuch. Seriously, buy a vowel.
I originally used lots of vowels, but when I started having statements such as the following in my code
*** Source Snippet Removed ***
I decided that having an acronym list would be a much better idea [lol]
Here's the real problem: all of your bad habits reinforce each other.
You wouldn't feel so compelled to use Hungarian notation and other such warts if you could write simpler, more obvious expressions. This, in turn, is possible if you (a) don't have to keep re-navigating the singleton maze to get at the relevant objects and (b) type out the rest of the names of things in full. I know from seeing enough other code that "lp" means "long pointer" (but seriously, it's been forever in computer years since it was meaningful to distinguish near and far pointers or WTF-ever), but I can't begin to guess what SPM stands for. The fact that you have to abbreviate this information in order to cram it in is actually a big warning sign that said information does not belong there. The fact that a variable is a pointer should be obvious from the fact that you are dereferencing it. (Actually, in C++, you can "dereference" objects that implement the appropriate operator overload, but the whole point of this technique is that the user should not *care* if it's a pointer or not.).
Further, you appear to avoid object creation for paranoid performance reasons (seriously: just create the new game state object fresh), and violate the Law of Demeter (again relating to the complex expressions). You should be going into a specific state, to ask it to ask the manager to return the new state and then set the state to that. Instead, you ask the current state to just do its thing, and said "thing" involves returning the new state by creating it.
It's actually very likely that you don't need a manager at all.
Quote:They are being created and deleted on demand, notice that GetInstancePtr allocates a new instance of the game state and Shutdown frees it.
... Oh. That's not obvious at all, though. Also, don't make up your own names for these kinds of things: C++ has the concept of constructors and destructors for a reason.
tl;dr:
// The "Run and Return Successor" variant of the State/Strategy pattern// (they're basically the same)// Also, pay attention to how *clean* I keep variable names.// You could also use std::auto_ptr here but I doubt it really improves much...GameState* current = new NewGame();while (current) { // The body of this loop is functionally equivalent to your example statement. // Assuming I'm understanding you correctly. GameState* next = current->update(); delete current; current = next;}// This of course assumes things like:class GameState { virtual GameState* update(); // always returns a new object or null private: // These things should never be copied GameState& operator=(const GameState&); GameState(const GameState&);};GameState* NewGame::update() { return new WorldMap();}
Author
Quote:Original post by Zahlman[...]but I can't begin to guess what SPM stands for.
SPM is the name of the name of the project.
Quote:Original post by ZahlmanThe fact that you have to abbreviate this information in order to cram it in is actually a big warning sign that said information does not belong there.
My previous version of the project used pretty long variable names such as "CSPMGameStateScheduleMission" or "CSPMGameStateCreateNewProgram". As a matter of fact, my classmates laugh at my tendency to write long and descriptive names for variables but, honestly, I found that for this particular project this convention was quite cumbersome to work with, so I decided to shorten the names and mantain a txt file with the acronyms list.
Your advice about information not belonging to certain places is very useful, so I think I'm going to revise the program architecture in order to eliminate dependencies.
Quote:Original post by ZahlmanInstead, you ask the current state to just do its thing, and said "thing" involves returning the new state by creating it.
It's actually very likely that you don't need a manager at all.
I don't agree with this point. I borrowed the manager concept from an article in the Ogre wiki and it proved to be, at least for me, a very tidy approach to coding that allows me to progresively add functionality to my game in a very simple manner. I also don't like the idea of returning a new state, but that's just my preference and I'm sure you've got a valid reason for doing that. I'll check the "Run and Return Succesor" pattern you suggest in your code snippet at the end of your post.
Quote:Original post by ZahlmanQuote:They are being created and deleted on demand, notice that GetInstancePtr allocates a new instance of the game state and Shutdown frees it.
... Oh. That's not obvious at all, though.
Maybe I didn't emphasize it enough, but I stated that in the first paragraph of my first post.
Quote:Original post by ZahlmanAlso, don't make up your own names for these kinds of things: C++ has the concept of constructors and destructors for a reason.
Yes, I agree with that. I realised that the singleton approach here is not the way to go so I'm going to change it.
Thanks for answering to my post, rating++
--Nacho
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
