Changeset 3363

Timestamp:

Jul 29, 2009, 5:24:39 PM (15 years ago)

Author:

rgrieder

Message:

Exception-safety for the Game and Core c'tors as well as load/unload-Graphics.

Location:

code/branches/resource/src/core

Files:

• Core.cc (modified) (8 diffs)
• Core.h (modified) (3 diffs)
• Game.cc (modified) (12 diffs)
• Game.h (modified) (7 diffs)

code/branches/resource/src/core/Core.cc

@@ -248 +248 @@
 
     Core::Core(const std::string& cmdLine)
-        : bDevRun_(false)
+        // Cleanup guard for identifier destruction (incl. XMLPort, configValues, consoleCommands)
+        : identifierDestroyer_(Identifier::destroyAllIdentifiers)
+        // Cleanup guard for external console commands that don't belong to an Identifier
+        , consoleCommandDestroyer_(CommandExecutor::destroyExternalCommands)
+        , bDevRun_(false)
         , bGraphicsLoaded_(false)
+        , configuration_(new CoreConfiguration()) // Don't yet create config values!
     {
         if (singletonRef_s != 0)
@@ -257 +262 @@
         }
         Core::singletonRef_s = this;
-
-        // We need the variables very soon. But don't configure them yet!
-        this->configuration_ = new CoreConfiguration();
 
         // Parse command line arguments first
@@ -276 +278 @@
         // create a signal handler (only active for linux)
         // This call is placed as soon as possible, but after the directories are set
-        this->signalHandler_ = new SignalHandler();
+        this->signalHandler_.reset(new SignalHandler());
         this->signalHandler_->doCatch(configuration_->executablePath_.string(), Core::getLogPathString() + "orxonox_crash.log");
 
@@ -295 +297 @@
 
         // Manage ini files and set the default settings file (usually orxonox.ini)
-        this->configFileManager_ = new ConfigFileManager();
+        this->configFileManager_.reset(new ConfigFileManager());
         this->configFileManager_->setFilename(ConfigFileType::Settings,
             CommandLine::getValue("settingsFile").getString());
 
         // Required as well for the config values
-        this->languageInstance_ = new Language();
+        this->languageInstance_.reset(new Language());
 
         // Do this soon after the ConfigFileManager has been created to open up the
@@ -307 +309 @@
 
         // Create the lua interface
-        this->luaBind_ = new LuaBind();
+        this->luaBind_.reset(new LuaBind());
 
         // initialise Tcl
-        this->tclBind_ = new TclBind(Core::getMediaPathString());
-        this->tclThreadManager_ = new TclThreadManager(tclBind_->getTclInterpreter());
+        this->tclBind_.reset(new TclBind(Core::getMediaPathString()));
+        this->tclThreadManager_.reset(new TclThreadManager(tclBind_->getTclInterpreter()));
 
         // create a shell
-        this->shell_ = new Shell();
+        this->shell_.reset(new Shell());
 
         // creates the class hierarchy for all classes with factories
@@ -321 +323 @@
 
     /**
-        @brief Sets the bool to true to avoid static functions accessing a deleted object.
+    @brief
+        All destruction code is handled by scoped_ptrs and SimpleScopeGuards.
     */
     Core::~Core()
     {
-        delete this->shell_;
-        delete this->tclThreadManager_;
-        delete this->tclBind_;
-        delete this->luaBind_;
-        delete this->configuration_;
-        delete this->languageInstance_;
-        delete this->configFileManager_;
-
-        // Destroy command line arguments
-        CommandLine::destroyAllArguments();
-        // Also delete external console commands that don't belong to an Identifier
-        CommandExecutor::destroyExternalCommands();
-        // Clean up class hierarchy stuff (identifiers, XMLPort, configValues, consoleCommand)
-        Identifier::destroyAllIdentifiers();
-
-        delete this->signalHandler_;
-
         // Don't assign singletonRef_s with NULL! Recreation is not supported
+        // The is quite simply because of the pre-main code that uses heap allocation
+        // And we correctly deallocate these resources in this destructor.
     }
 
@@ -351 +339 @@
 
         // Load OGRE including the render window
-        this->graphicsManager_ = new GraphicsManager();
+        scoped_ptr<GraphicsManager> graphicsManager(new GraphicsManager());
 
         // The render window width and height are used to set up the mouse movement.
         size_t windowHnd = 0;
-        Ogre::RenderWindow* renderWindow = GraphicsManager::getInstance().getRenderWindow();
-        renderWindow->getCustomAttribute("WINDOW", &windowHnd);
+        graphicsManager->getRenderWindow()->getCustomAttribute("WINDOW", &windowHnd);
 
         // Calls the InputManager which sets up the input devices.
-        inputManager_ = new InputManager(windowHnd);
+        scoped_ptr<InputManager> inputManager(new InputManager(windowHnd));
 
         // load the CEGUI interface
-        guiManager_ = new GUIManager(renderWindow);
+        guiManager_.reset(new GUIManager(graphicsManager->getRenderWindow()));
+
+        // Dismiss scoped pointers
+        graphicsManager_.swap(graphicsManager);
+        inputManager_.swap(inputManager);
 
         bGraphicsLoaded_ = true;
@@ -372 +363 @@
             return;
 
-        delete this->guiManager_;
-        delete this->inputManager_;
-        delete graphicsManager_;
+        this->guiManager_.reset();;
+        this->inputManager_.reset();;
+        this->graphicsManager_.reset();
 
         bGraphicsLoaded_ = false;

code/branches/resource/src/core/Core.h

@@ -43 +43 @@
 
 #include <cassert>
+#include <boost/scoped_ptr.hpp>
 #include "util/OutputHandler.h"
+#include "util/ScopeGuard.h"
 
 namespace orxonox
 {
     class CoreConfiguration;
+    using boost::scoped_ptr;
 
     /**
@@ -58 +61 @@
     class _CoreExport Core
     {
+        typedef Loki::ScopeGuardImpl0<void (*)()> SimpleScopeGuard;
+
         public:
             /**
@@ -112 +117 @@
             void setThreadAffinity(int limitToCPU);
 
-            // Singletons
-            ConfigFileManager*    configFileManager_;
-            Language*             languageInstance_;
-            LuaBind*              luaBind_;
-            Shell*                shell_;
-            SignalHandler*        signalHandler_;
-            TclBind*              tclBind_;
-            TclThreadManager*     tclThreadManager_;
+            // Mind the order for the destruction!
+            scoped_ptr<SignalHandler>     signalHandler_;
+            SimpleScopeGuard              identifierDestroyer_;
+            SimpleScopeGuard              consoleCommandDestroyer_;
+            scoped_ptr<ConfigFileManager> configFileManager_;
+            scoped_ptr<Language>          languageInstance_;
+            scoped_ptr<CoreConfiguration> configuration_;
+            scoped_ptr<LuaBind>           luaBind_;
+            scoped_ptr<TclBind>           tclBind_;
+            scoped_ptr<TclThreadManager>  tclThreadManager_;
+            scoped_ptr<Shell>             shell_;
             // graphical
-            InputManager*         inputManager_;        //!< Interface to OIS
-            GUIManager*           guiManager_;          //!< Interface to GUI
-            GraphicsManager*      graphicsManager_;     //!< Interface to OGRE
+            scoped_ptr<GraphicsManager>   graphicsManager_;     //!< Interface to OGRE
+            scoped_ptr<InputManager>      inputManager_;        //!< Interface to OIS
+            scoped_ptr<GUIManager>        guiManager_;          //!< Interface to GUI
 
-            bool                  bDevRun_;             //!< True for runs in the build directory (not installed)
-            bool                  bGraphicsLoaded_;
-            CoreConfiguration*    configuration_;
+            bool                          bDevRun_;             //!< True for runs in the build directory (not installed)
+            bool                          bGraphicsLoaded_;
 
             static Core* singletonRef_s;

code/branches/resource/src/core/Game.cc

@@ -40 +40 @@
 #include "util/Debug.h"
 #include "util/Exception.h"
+#include "util/ScopeGuard.h"
 #include "util/Sleep.h"
 #include "util/SubString.h"
@@ -133 +134 @@
 
         // Set up a basic clock to keep time
-        this->gameClock_ = new Clock();
+        this->gameClock_.reset(new Clock());
 
         // Create the Core
-        this->core_ = new Core(cmdLine);
+        this->core_.reset(new Core(cmdLine));
 
         // After the core has been created, we can safely instantiate the GameStates that don't require graphics
@@ -153 +154 @@
 
         // Do this after the Core creation!
-        this->configuration_ = new GameConfiguration();
+        this->configuration_.reset(new GameConfiguration());
     }
 
     /**
     @brief
+        All destruction code is handled by scoped_ptrs and SimpleScopeGuards.
     */
     Game::~Game()
     {
-        // Destroy the configuration helper class instance
-        delete this->configuration_;
-
-        // Destroy the GameStates (note that the nodes still point to them, but doesn't matter)
-        for (std::map<std::string, GameState*>::const_iterator it = constructedStates_.begin();
-            it != constructedStates_.end(); ++it)
-            delete it->second;
-
-        // Destroy the Core and with it almost everything
-        delete this->core_;
-        delete this->gameClock_;
-
-        // Take care of the GameStateFactories
-        GameStateFactory::destroyFactories();
-
         // Don't assign singletonRef_s with NULL! Recreation is not supported
     }
@@ -283 +270 @@
     {
         // Note: The first element is the empty root state, which doesn't need ticking
-        for (std::vector<GameState*>::const_iterator it = this->loadedStates_.begin() + 1;
+        for (GameStateVector::const_iterator it = this->loadedStates_.begin() + 1;
             it != this->loadedStates_.end(); ++it)
         {
@@ -457 +444 @@
     }
 
-    GameState* Game::getState(const std::string& name)
-    {
-        std::map<std::string, GameState*>::const_iterator it = constructedStates_.find(name);
+    shared_ptr<GameState> Game::getState(const std::string& name)
+    {
+        GameStateMap::const_iterator it = constructedStates_.find(name);
         if (it != constructedStates_.end())
             return it->second;
@@ -469 +456 @@
             else
                 COUT(1) << "Error: Could not find GameState '" << name << "'." << std::endl;
-            return 0;
+            return shared_ptr<GameState>();
         }
     }
@@ -528 +515 @@
         {
             core_->loadGraphics();
+            Loki::ScopeGuard graphicsUnloader = Loki::MakeObjGuard(*this, &Game::unloadGraphics);
             GameMode::bShowsGraphics_s = true;
 
@@ -536 +524 @@
                 if (it->second.bGraphicsMode)
                 {
+                    // Game state loading failure is serious --> don't catch
+                    shared_ptr<GameState> gameState = GameStateFactory::fabricate(it->second);
                     if (!constructedStates_.insert(std::make_pair(
-                        it->second.stateName, GameStateFactory::fabricate(it->second))).second)
+                        it->second.stateName, gameState)).second)
                         assert(false); // GameState was already created!
                 }
             }
+            graphicsUnloader.Dismiss();
         }
     }
@@ -549 +540 @@
         {
             // Destroy all the GameStates that require graphics
-            for (std::map<std::string, GameState*>::iterator it = constructedStates_.begin(); it != constructedStates_.end();)
+            for (GameStateMap::iterator it = constructedStates_.begin(); it != constructedStates_.end();)
             {
                 if (it->second->getInfo().bGraphicsMode)
-                {
-                    delete it->second;
                     constructedStates_.erase(it++);
-                }
                 else
                     ++it;
@@ -577 +565 @@
     {
         this->bChangingState_ = true;
+        LOKI_ON_BLOCK_EXIT_OBJ(*this, &Game::resetChangingState);
+
         // If state requires graphics, load it
-        if (gameStateDeclarations_s[name].bGraphicsMode)
+        Loki::ScopeGuard graphicsUnloader = Loki::MakeObjGuard(*this, &Game::unloadGraphics);
+        if (gameStateDeclarations_s[name].bGraphicsMode && !GameMode::showsGraphics())
             this->loadGraphics();
-        GameState* state = this->getState(name);
+        else
+            graphicsUnloader.Dismiss();
+
+        shared_ptr<GameState> state = this->getState(name);
         state->activate();
         if (!this->loadedStates_.empty())
@@ -586 +580 @@
         this->loadedStates_.push_back(state);
         state->activity_.topState = true;
-        this->bChangingState_ = false;
+
+        graphicsUnloader.Dismiss();
     }
 
     void Game::unloadState(const std::string& name)
     {
-        GameState* state = this->getState(name);
         this->bChangingState_ = true;
-        state->activity_.topState = false;
-        this->loadedStates_.pop_back();
-        if (!this->loadedStates_.empty())
-            this->loadedStates_.back()->activity_.topState = true;
         try
         {
+            shared_ptr<GameState> state = this->getState(name);
+            state->activity_.topState = false;
+            this->loadedStates_.pop_back();
+            if (!this->loadedStates_.empty())
+                this->loadedStates_.back()->activity_.topState = true;
             state->deactivate();
-            // Check if graphis is still required
-            bool graphicsRequired = false;
-            for (unsigned i = 0; i < loadedStates_.size(); ++i)
-                graphicsRequired |= loadedStates_[i]->getInfo().bGraphicsMode;
-            if (!graphicsRequired)
-                this->unloadGraphics();
         }
         catch (const std::exception& ex)
@@ -612 +601 @@
             COUT(2) << "         There might be potential resource leaks involved! To avoid this, improve exception-safety." << std::endl;
         }
+        // Check if graphics is still required
+        bool graphicsRequired = false;
+        for (unsigned i = 0; i < loadedStates_.size(); ++i)
+            graphicsRequired |= loadedStates_[i]->getInfo().bGraphicsMode;
+        if (!graphicsRequired)
+            this->unloadGraphics();
         this->bChangingState_ = false;
     }
 
-    std::map<std::string, Game::GameStateFactory*> Game::GameStateFactory::factories_s;
-
-    /*static*/ GameState* Game::GameStateFactory::fabricate(const GameStateInfo& info)
-    {
-        std::map<std::string, GameStateFactory*>::const_iterator it = factories_s.find(info.className);
+    std::map<std::string, shared_ptr<Game::GameStateFactory> > Game::GameStateFactory::factories_s;
+
+    /*static*/ shared_ptr<GameState> Game::GameStateFactory::fabricate(const GameStateInfo& info)
+    {
+        std::map<std::string, shared_ptr<Game::GameStateFactory> >::const_iterator it = factories_s.find(info.className);
         assert(it != factories_s.end());
         return it->second->fabricateInternal(info);
     }
-
-    /*static*/ void Game::GameStateFactory::destroyFactories()
-    {
-        for (std::map<std::string, GameStateFactory*>::const_iterator it = factories_s.begin(); it != factories_s.end(); ++it)
-            delete it->second;
-        factories_s.clear();
-    }
 }

code/branches/resource/src/core/Game.h

@@ -44 +44 @@
 #include <vector>
 #include <boost/shared_ptr.hpp>
+#include <boost/scoped_ptr.hpp>
 #include <boost/preprocessor/cat.hpp>
 
@@ -59 +60 @@
 {
     class GameConfiguration;
+    using boost::scoped_ptr;
+    using boost::shared_ptr;
 
     //! Helper object required before GameStates are being constructed
@@ -75 +78 @@
     class _CoreExport Game
     {
+        typedef std::vector<shared_ptr<GameState> > GameStateVector;
+        typedef std::map<std::string, shared_ptr<GameState> > GameStateMap;
         typedef boost::shared_ptr<GameStateTreeNode> GameStateTreeNodePtr;
     public:
@@ -81 +86 @@
 
         void setStateHierarchy(const std::string& str);
-        GameState* getState(const std::string& name);
+        shared_ptr<GameState> getState(const std::string& name);
 
         void run();
@@ -106 +111 @@
         public:
             virtual ~GameStateFactory() { }
-            static GameState* fabricate(const GameStateInfo& info);
+            static shared_ptr<GameState> fabricate(const GameStateInfo& info);
             template <class T>
             static void createFactory(const std::string& className)
-                { factories_s[className] = new TemplateGameStateFactory<T>(); }
-            static void destroyFactories();
+                { factories_s[className].reset(new TemplateGameStateFactory<T>()); }
         private:
-            virtual GameState* fabricateInternal(const GameStateInfo& info) = 0;
-            static std::map<std::string, GameStateFactory*> factories_s;
+            virtual shared_ptr<GameState> fabricateInternal(const GameStateInfo& info) = 0;
+            static std::map<std::string, shared_ptr<GameStateFactory> > factories_s;
         };
         template <class T>
@@ -119 +123 @@
         {
         public:
-            GameState* fabricateInternal(const GameStateInfo& info)
-                { return new T(info); }
+            shared_ptr<GameState> fabricateInternal(const GameStateInfo& info)
+                { return shared_ptr<GameState>(new T(info)); }
         };
 
@@ -144 +148 @@
         void updateFPSLimiter();
 
-        std::map<std::string, GameState*>  constructedStates_;
-        std::vector<GameState*>            loadedStates_;
+        // ScopeGuard helper function
+        void resetChangingState() { this->bChangingState_ = false; }
+
+        scoped_ptr<Clock>                  gameClock_;
+        scoped_ptr<Core>                   core_;
+        scoped_ptr<GameConfiguration>      configuration_;
+
+        GameStateMap                       constructedStates_;
+        GameStateVector                    loadedStates_;
         GameStateTreeNodePtr               rootStateNode_;
         GameStateTreeNodePtr               loadedTopStateNode_;
-        std::vector<GameStateTreeNodePtr > requestedStateNodes_;
-
-        Core*                              core_;
-        Clock*                             gameClock_;
-        GameConfiguration*                 configuration_;
+        std::vector<GameStateTreeNodePtr>  requestedStateNodes_;
 
         bool                               bChangingState_;