#5408: After some restructuring and a few paper cuts, the UndoSystem …

…is now a child of the IMapRootNode implementations.

The GlobalUndoSystem() accessor now refers to the GlobalMapModule()'s undo system, the undo system module has been transformed into the GlobalUndoSystemFactory.
I'm not entirely happy with the way the Map module is subscribing to each map resource's undo system on map change, since the root node of a MapResource is not always available.

include/imap.h

@@ -31,6 +31,9 @@ class ITargetManager;
 // see ilayer.h
 class ILayerManager;
 
+// see iundo.h
+class IUndoSystem;
+
 namespace selection
 {
 	class ISelectionSetManager;
@@ -85,6 +88,9 @@ class IMapRootNode :
 	 * Provides methods to create and assign layers in this map.
 	 */
 	virtual ILayerManager& getLayerManager() = 0;
+
+    // The UndoSystem of this map
+    virtual IUndoSystem& getUndoSystem() = 0;
 };
 typedef std::shared_ptr<IMapRootNode> IMapRootNodePtr;
 
@@ -181,6 +187,10 @@ class IMap :
     // Signal emitted when an operation affecting the map has been redone
     virtual sigc::signal<void>& signal_postRedo() = 0;
 
+    // Accessor to the undo system of the main scene. 
+    // Throws std::runtime_error if no map resource has been loaded.
+    virtual IUndoSystem& getUndoSystem() = 0;
+
 	// Caution: this is upposed to be called on startup, since it doesn't ask the user
 	// whether to save the current map. Use the "NewMap" command for regular purposes.
 	virtual void createNewMap() = 0;

include/iundo.h

@@ -4,6 +4,7 @@
 /// \brief The undo-system interface. Uses the 'memento' pattern.
 
 #include "imodule.h"
+#include "imap.h"
 #include <cstddef>
 #include <memory>
 #include <sigc++/signal.h>
@@ -53,12 +54,11 @@ class IUndoStateSaver
 	virtual void save(IUndoable& undoable) = 0;
 };
 
-const char* const MODULE_UNDOSYSTEM("UndoSystem");
-
-class IUndoSystem :
-	public RegisterableModule
+class IUndoSystem
 {
 public:
+    using Ptr = std::shared_ptr<IUndoSystem>;
+
 	// Undoable objects need to call this to get hold of a StateSaver instance
 	// which will take care of exporting and saving the state. The passed map file change 
     // tracker will be notified when the state is saved.
@@ -104,11 +104,28 @@ class IUndoSystem :
 	virtual void detachTracker(Tracker& tracker) = 0;
 };
 
-// The accessor function
-inline IUndoSystem& GlobalUndoSystem() 
+class IUndoSystemFactory :
+    public RegisterableModule
+{
+public:
+    virtual ~IUndoSystemFactory() {}
+
+    // Create a new UndoSystem instance for use in a map root node
+    virtual IUndoSystem::Ptr createUndoSystem() = 0;
+};
+
+constexpr const char* const MODULE_UNDOSYSTEM_FACTORY("UndoSystemFactory");
+
+inline IUndoSystemFactory& GlobalUndoSystemFactory()
+{
+    static module::InstanceReference<IUndoSystemFactory> _reference(MODULE_UNDOSYSTEM_FACTORY);
+    return _reference;
+}
+
+// The accessor function to the main map's undo system
+inline IUndoSystem& GlobalUndoSystem()
 {
-	static module::InstanceReference<IUndoSystem> _reference(MODULE_UNDOSYSTEM);
-	return _reference;
+    return GlobalMapModule().getUndoSystem();
 }
 
 class UndoableCommand

libs/scene/BasicRootNode.h

@@ -4,6 +4,7 @@
 #include "mapfile.h"
 #include "ilayer.h"
 #include "ientity.h"
+#include "iundo.h"
 #include "iselectiongroup.h"
 #include "iselectionset.h"
 #include "Node.h"
@@ -16,7 +17,7 @@ namespace scene
 
 // A very simple implementation of a Map Root Node
 // for use in the preview widget's scenes.
-class BasicRootNode :
+class BasicRootNode final :
     public IMapRootNode,
     public Node,
     public KeyValueStore
@@ -29,6 +30,7 @@ class BasicRootNode :
     selection::ISelectionGroupManager::Ptr _selectionGroupManager;
     selection::ISelectionSetManager::Ptr _selectionSetManager;
     ILayerManager::Ptr _layerManager;
+    IUndoSystem::Ptr _undoSystem;
     AABB _emptyAABB;
 
 public:
@@ -39,12 +41,10 @@ class BasicRootNode :
         _selectionGroupManager = GlobalSelectionGroupModule().createSelectionGroupManager();
         _selectionSetManager = GlobalSelectionSetModule().createSelectionSetManager();
         _layerManager = GlobalLayerModule().createLayerManager();
+        _undoSystem = GlobalUndoSystemFactory().createUndoSystem();
     }
 
-    virtual ~BasicRootNode()
-    {}
-
-    virtual void setName(const std::string& name) override
+    void setName(const std::string& name) override
     {
         _name = name;
     }
@@ -79,6 +79,11 @@ class BasicRootNode :
         return *_layerManager;
     }
 
+    IUndoSystem& getUndoSystem() override
+    {
+        return *_undoSystem;
+    }
+
     const AABB& localAABB() const override
     {
         return _emptyAABB;

radiant/ui/einspector/EntityInspector.cpp

@@ -505,10 +505,10 @@ const StringSet& EntityInspector::getDependencies() const
         MODULE_XMLREGISTRY,
         MODULE_GROUPDIALOG,
         MODULE_SELECTIONSYSTEM,
-        MODULE_UNDOSYSTEM,
         MODULE_GAMEMANAGER,
         MODULE_COMMANDSYSTEM,
-        MODULE_MAINFRAME
+        MODULE_MAINFRAME,
+        MODULE_MAP,
     };
 
     return _dependencies;

radiantcore/CMakeLists.txt

@@ -292,6 +292,7 @@ add_library(radiantcore MODULE
             shaders/textures/TextureManipulator.cpp
             skins/Doom3SkinCache.cpp
             undo/UndoSystem.cpp
+            undo/UndoSystemFactory.cpp
             versioncontrol/VersionControlManager.cpp
             vfs/DeflatedInputStream.cpp
             vfs/DirectoryArchive.cpp

radiantcore/brush/BrushModule.cpp

@@ -100,7 +100,6 @@ const StringSet& BrushModuleImpl::getDependencies() const {
 		_dependencies.insert(MODULE_GAMEMANAGER);
 		_dependencies.insert(MODULE_XMLREGISTRY);
 		_dependencies.insert(MODULE_PREFERENCESYSTEM);
-		_dependencies.insert(MODULE_UNDOSYSTEM);
 	}
 
 	return _dependencies;

radiantcore/map/Map.cpp

@@ -84,6 +84,25 @@ void Map::clearMapResource()
 
     // Rename the map to "unnamed" in any case to avoid overwriting the failed map
     setMapName(_(MAP_UNNAMED_STRING));
+
+    connectToUndoSystem();
+}
+
+void Map::connectToUndoSystem()
+{
+    _postUndoListener.disconnect();
+    _postRedoListener.disconnect();
+
+    if (!_resource->getRootNode()) return;
+
+    _postUndoListener = _resource->getRootNode()->getUndoSystem().signal_postUndo().connect([this]()
+    {
+        _mapPostUndoSignal.emit();
+    });
+    _postRedoListener = _resource->getRootNode()->getUndoSystem().signal_postRedo().connect([this]()
+    {
+        _mapPostRedoSignal.emit();
+    });
 }
 
 void Map::loadMapResourceFromPath(const std::string& path)
@@ -113,10 +132,7 @@ void Map::loadMapResourceFromLocation(const MapLocation& location)
         GlobalMapResourceManager().createFromArchiveFile(location.path, location.archiveRelativePath) :
         GlobalMapResourceManager().createFromPath(location.path);
 
-    if (!_resource)
-    {
-        return;
-    }
+    assert(_resource);
 
     try
     {
@@ -133,6 +149,8 @@ void Map::loadMapResourceFromLocation(const MapLocation& location)
         clearMapResource();
     }
 
+    connectToUndoSystem();
+
     // Take the new node and insert it as map root
     GlobalSceneGraph().setRoot(_resource->getRootNode());
 
@@ -452,6 +470,18 @@ sigc::signal<void>& Map::signal_postRedo()
     return _mapPostRedoSignal;
 }
 
+IUndoSystem& Map::getUndoSystem()
+{
+    const auto& rootNode = _resource->getRootNode();
+
+    if (!rootNode)
+    {
+        throw std::runtime_error("No map resource loaded");
+    }
+
+    return rootNode->getUndoSystem();
+}
+
 // move the view to a certain position
 void Map::focusViews(const Vector3& point, const Vector3& angles)
 {
@@ -774,6 +804,8 @@ bool Map::saveAs()
         return false;
     }
 
+    connectToUndoSystem();
+
     // Resource save was successful, notify about this name change
     rename(fileInfo.fullPath);
 
@@ -924,6 +956,34 @@ void Map::registerCommands()
           cmd::ARGTYPE_INT | cmd::ARGTYPE_OPTIONAL,
           cmd::ARGTYPE_INT | cmd::ARGTYPE_OPTIONAL,
           cmd::ARGTYPE_INT | cmd::ARGTYPE_OPTIONAL });
+
+    // Add undo commands
+    GlobalCommandSystem().addCommand("Undo", std::bind(&Map::undoCmd, this, std::placeholders::_1));
+    GlobalCommandSystem().addCommand("Redo", std::bind(&Map::redoCmd, this, std::placeholders::_1));
+}
+
+void Map::undoCmd(const cmd::ArgumentList& args)
+{
+    try
+    {
+        getUndoSystem().undo();
+    }
+    catch (const std::runtime_error& err)
+    {
+        throw cmd::ExecutionNotPossible(err.what());
+    }
+}
+
+void Map::redoCmd(const cmd::ArgumentList& args)
+{
+    try
+    {
+        getUndoSystem().redo();
+    }
+    catch (const std::runtime_error& err)
+    {
+        throw cmd::ExecutionNotPossible(err.what());
+    }
 }
 
 // Static command targets
@@ -1352,18 +1412,15 @@ const std::string& Map::getName() const
 
 const StringSet& Map::getDependencies() const
 {
-    static StringSet _dependencies;
-
-    if (_dependencies.empty())
-	{
-		_dependencies.insert(MODULE_GAMEMANAGER);
-		_dependencies.insert(MODULE_SCENEGRAPH);
-		_dependencies.insert(MODULE_MAPINFOFILEMANAGER);
-		_dependencies.insert(MODULE_FILETYPES);
-		_dependencies.insert(MODULE_MAPRESOURCEMANAGER);
-        _dependencies.insert(MODULE_COMMANDSYSTEM);
-        _dependencies.insert(MODULE_UNDOSYSTEM);
-    }
+    static StringSet _dependencies
+    {
+        MODULE_GAMEMANAGER,
+        MODULE_SCENEGRAPH,
+        MODULE_MAPINFOFILEMANAGER,
+        MODULE_FILETYPES,
+        MODULE_MAPRESOURCEMANAGER,
+        MODULE_COMMANDSYSTEM
+    };
 
     return _dependencies;
 }
@@ -1404,16 +1461,6 @@ void Map::initialiseModule(const IApplicationContext& ctx)
         radiant::IMessage::Type::ApplicationShutdownRequest,
         radiant::TypeListener<radiant::ApplicationShutdownRequest>(
             sigc::mem_fun(this, &Map::handleShutdownRequest)));
-
-    // Prepare to dispatch the undo/redo signals
-    _postUndoListener = GlobalUndoSystem().signal_postUndo().connect([this]()
-    {
-        _mapPostUndoSignal.emit();
-    });
-    _postRedoListener = GlobalUndoSystem().signal_postRedo().connect([this]()
-    {
-        _mapPostRedoSignal.emit();
-    });
 }
 
 void Map::shutdownModule()

radiantcore/map/Map.h

@@ -195,6 +195,8 @@ class Map :
     sigc::signal<void>& signal_postUndo() override;
     sigc::signal<void>& signal_postRedo() override;
 
+    IUndoSystem& getUndoSystem() override;
+
 	// greebo: Creates a new, empty map file.
 	void createNewMap() override;
 
@@ -282,13 +284,17 @@ class Map :
 	void emitMapEvent(MapEvent ev);
 
 	void clearMapResource();
+	void connectToUndoSystem();
 
     void cleanupMergeOperation();
 
     /** greebo: Focus the XYViews and the Camera to the given point/angle.
      */
     void focusViews(const Vector3& point, const Vector3& angles);
     void focusViewCmd(const cmd::ArgumentList& args);
+
+    void undoCmd(const cmd::ArgumentList& args);
+    void redoCmd(const cmd::ArgumentList& args);
 };
 
 } // namespace map

radiantcore/map/RootNode.cpp

@@ -11,8 +11,6 @@ RootNode::RootNode(const std::string& name) :
 	// Apply root status to this node
 	setIsRoot(true);
 
-	GlobalUndoSystem().attachTracker(*this);
-
 	// Create a new namespace
 	_namespace = GlobalNamespaceFactory().createNamespace();
 	assert(_namespace);
@@ -28,11 +26,16 @@ RootNode::RootNode(const std::string& name) :
 
 	_layerManager = GlobalLayerModule().createLayerManager();
 	assert(_layerManager);
+
+    _undoSystem = GlobalUndoSystemFactory().createUndoSystem();
+    assert(_undoSystem);
+
+    _undoSystem->attachTracker(*this);
 }
 
 RootNode::~RootNode()
 {
-	GlobalUndoSystem().detachTracker(*this);
+    _undoSystem->detachTracker(*this);
 
 	// Remove all child nodes to trigger their destruction
 	removeAllChildNodes();
@@ -68,6 +71,11 @@ scene::ILayerManager& RootNode::getLayerManager()
 	return *_layerManager;
 }
 
+IUndoSystem& RootNode::getUndoSystem()
+{
+    return *_undoSystem;
+}
+
 std::string RootNode::name() const 
 {
 	return _name;