#5108: Refactor MapResource and add some documentation.

Add map loading test passing an invalid resource path pointing to a folder, which made the MapResource crash.
Fixed the crash by throwing the correct exception type as mentioned in the docs.

radiantcore/map/MapResource.cpp

@@ -53,26 +53,44 @@ namespace
 	}
 }
 
-// Constructor
-MapResource::MapResource(const std::string& name) :
-	_extension(os::getExtension(name))
+MapResource::MapResource(const std::string& resourcePath)
 {
-	// Initialise the paths, this is all needed for realisation
-    _path = rootPath(name);
-	_name = os::getRelativePath(name, _path);
+    constructPaths(resourcePath);
 }
 
 void MapResource::rename(const std::string& fullPath)
 {
-	// Save the paths locally and split them into parts
-	_extension = os::getExtension(fullPath);
-	_path = rootPath(fullPath);
-	_name = os::getRelativePath(fullPath, _path);
+    constructPaths(fullPath);
 
 	// Rename the map root as well
     _mapRoot->setName(_name);
 }
 
+void MapResource::constructPaths(const std::string& resourcePath)
+{
+    _extension = os::getExtension(resourcePath);
+
+    // Try to find a folder part of the VFS and use that as base path
+    // Will result to an empty string if the path is outside the VFS
+    _path = rootPath(resourcePath);
+
+    // Try to create a relative path, based on the VFS directories
+    // If no relative path can be deducted, use the absolute resourcePath 
+    // in unmodified form.
+    _name = os::getRelativePath(resourcePath, _path);
+}
+
+std::string MapResource::getAbsoluteResourcePath()
+{
+    // Concatenate path+name, since they either contain base + relative:
+    //      _path == "c:/games/darkmod/"
+    //      _name == "maps/arkham.map"
+    // or an empty _path with _name holding the full path:
+    //      _path == ""
+    //      _name == "c:/some/non/vfs/folder/arkham.map"
+    return _path + _name;
+}
+
 bool MapResource::load()
 {
 	if (!_mapRoot)
@@ -101,7 +119,7 @@ void MapResource::save(const MapFormatPtr& mapFormat)
 
 	rMessage() << "Using " << format->getMapFormatName() << " format to save the resource." << std::endl;
 
-	std::string fullpath = _path + _name;
+	std::string fullpath = getAbsoluteResourcePath();
 
 	// Save a backup of the existing file (rename it to .bak) if it exists in the first place
 	if (os::fileOrDirExists(fullpath) && !saveBackup())
@@ -125,7 +143,7 @@ void MapResource::save(const MapFormatPtr& mapFormat)
 
 bool MapResource::saveBackup()
 {
-	fs::path fullpath = (_path + _name);
+	fs::path fullpath = getAbsoluteResourcePath();
 
 	if (path_is_absolute(fullpath.string().c_str()))
 	{
@@ -243,7 +261,7 @@ RootNodePtr MapResource::loadMapNode()
 	}
 
 	// Build the map path
-	std::string fullpath = _path + _name;
+	std::string fullpath = getAbsoluteResourcePath();
 
 	// Open a stream (from physical file or VFS)
 	openFileStream(fullpath, [&](std::istream& mapStream)
@@ -376,7 +394,7 @@ void MapResource::openFileStream(const std::string& path, const std::function<vo
 		if (file.failed())
 		{
 			rError() << "failure" << std::endl;
-			throw std::runtime_error(fmt::format(_("Failure opening file:\n{0}"), path));
+			throw OperationException(fmt::format(_("Failure opening file:\n{0}"), path));
 		}
 
 		std::istream stream(&file);
@@ -395,7 +413,7 @@ void MapResource::openFileStream(const std::string& path, const std::function<vo
 		if (!vfsFile)
 		{
 			rError() << "failure" << std::endl;
-			throw std::runtime_error(fmt::format(_("Failure opening file:\n{0}"), path));
+			throw OperationException(fmt::format(_("Failure opening file:\n{0}"), path));
 		}
 
 		rMessage() << "success." << std::endl;

radiantcore/map/MapResource.h

@@ -19,15 +19,22 @@ class MapResource :
 private:
     scene::IMapRootNodePtr _mapRoot;
 
+    // Contains the absolute base path (e.g. c:/games/darkmod/) if the resourcePath
+    // points to a directory which is part of the VFS search paths. 
+    // Will be an empty string if the resource is pointing to a path outside the VFS.
 	std::string _path;
+
+    // Either contains the path relative to the base path (e.g. "maps/arkham.map")
+    // or the full absolute path to the map (in case the resource path 
+    // is pointing to a path outside the VFS)
 	std::string _name;
 
 	// File extension of this resource
 	std::string _extension;
 
 public:
 	// Constructor
-	MapResource(const std::string& name);
+	MapResource(const std::string& resourcePath);
 
 	void rename(const std::string& fullPath) override;
 
@@ -43,6 +50,9 @@ class MapResource :
 						 const GraphTraversalFunc& traverse, const std::string& filename);
 
 private:
+    void constructPaths(const std::string& resourcePath);
+    std::string getAbsoluteResourcePath();
+
 	void mapSave();
 	void onMapChanged();
 

test/MapSavingLoading.cpp

@@ -275,6 +275,15 @@ TEST_F(MapLoadingTest, openNonExistentMap)
     EXPECT_FALSE(algorithm::getEntityByName(GlobalMapModule().getRoot(), "world"));
 }
 
+TEST_F(MapLoadingTest, openWithInvalidPath)
+{
+    // Pass a directory name to it
+    GlobalCommandSystem().executeCommand("OpenMap", _context.getTemporaryDataPath());
+
+    // No worldspawn in this map, it should be empty
+    EXPECT_FALSE(algorithm::getEntityByName(GlobalMapModule().getRoot(), "world"));
+}
+
 TEST_F(MapSavingTest, saveMapWithoutModification)
 {
     auto tempPath = createMapCopyInTempDataPath("altar.map", "altar_saveMapWithoutModification.map");