Merge pull request #598 from GriffinRichards/rme-fixes

Region map editor fixes
huderlem · Jun 26, 2024 · 18bdbc8 · 18bdbc8
2 parents b28d408 + cab710b
commit 18bdbc8
Show file tree

Hide file tree

Showing 6 changed files with 82 additions and 49 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,7 +7,10 @@ and this project somewhat adheres to [Semantic Versioning](https://semver.org/sp
 The **"Breaking Changes"** listed below are changes that have been made in the decompilation projects (e.g. pokeemerald), which porymap requires in order to work properly. It also includes changes to the scripting API that may change the behavior of existing porymap scripts. If porymap is used with a project or API script that is not up-to-date with the breaking changes, then porymap will likely break or behave improperly.
 
 ## [Unreleased]
-Nothing, yet.
+### Fixed
+- Fix `Add Region Map...` not updating the region map settings file.
+- Fix some crashes on invalid region map tilesets.
+- Improve error reporting for invalid region map editor settings.
 
 ## [5.4.1] - 2024-03-21
 ### Fixed

diff --git a/include/mainwindow.h b/include/mainwindow.h
@@ -388,6 +388,7 @@ private slots:
 
     void initTilesetEditor();
     bool initRegionMapEditor(bool silent = false);
+    bool askToFixRegionMapEditor();
     void initShortcutsEditor();
     void initCustomScriptsEditor();
     void connectSubEditorsToShortcutsEditor();

diff --git a/include/ui/regionmapeditor.h b/include/ui/regionmapeditor.h
@@ -27,6 +27,7 @@ class RegionMapEditor : public QMainWindow
     ~RegionMapEditor();
 
     bool load(bool silent = false);
+    bool setupErrored() const { return setupError; }
 
     void onRegionMapTileSelectorSelectedTileChanged(unsigned id);
     void onRegionMapTileSelectorHoveredTileChanged(unsigned id);
@@ -41,6 +42,8 @@ class RegionMapEditor : public QMainWindow
 
     void resizeTilemap(int width, int height);
 
+    bool reconfigure();
+
     QObjectList shortcutableObjects() const;
 
 public slots:
@@ -53,9 +56,13 @@ public slots:
     RegionMap *region_map = nullptr;
     tsl::ordered_map<QString, RegionMap *> region_maps;
 
+    QString configFilepath;
+    QString mapSectionFilepath;
+
     poryjson::Json rmConfigJson;
 
     bool configSaved = false;
+    bool setupError = false;
 
     QUndoGroup history;
 

diff --git a/src/core/regionmap.cpp b/src/core/regionmap.cpp
@@ -79,14 +79,25 @@ bool RegionMap::loadTilemap(poryjson::Json tilemapJson) {
         this->palette_path = tilemapObject["palette"].string_value();
     }
 
+    QImage tilesetFile(fullPath(this->tileset_path));
+    if (tilesetFile.isNull()) {
+        logError(QString("Failed to open region map tileset file '%1'.").arg(tileset_path));
+        return false;
+    }
+
+    if (tilesetFile.width() < 8 || tilesetFile.height() < 8) {
+        logError(QString("Region map tileset file '%1' must be at least 8x8.").arg(tileset_path));
+        return false;
+    }
+
     QFile tilemapFile(fullPath(this->tilemap_path));
     if (!tilemapFile.open(QIODevice::ReadOnly)) {
-        logError(QString("Failed to open region map tilemap file %1.").arg(tilemap_path));
+        logError(QString("Failed to open region map tilemap file '%1'.").arg(tilemap_path));
         return false;
     }
 
     if (tilemapFile.size() < tilemapBytes()) {
-        logError(QString("The region map tilemap at %1 is too small.").arg(tilemap_path));
+        logError(QString("The region map tilemap at '%1' is too small.").arg(tilemap_path));
         return false;
     }
 
@@ -297,7 +308,7 @@ bool RegionMap::loadLayout(poryjson::Json layoutJson) {
                         }
                         setLayout("main", layout);
                     } else {
-                        logError("Region map layout is not readable.");
+                        logError(QString("Failed to read region map layout from '%1'.").arg(this->layout_path));
                         return false;
                     }
                 }

diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp
@@ -2960,23 +2960,43 @@ void MainWindow::on_pushButton_CreatePrefab_clicked() {
 
 bool MainWindow::initRegionMapEditor(bool silent) {
     this->regionMapEditor = new RegionMapEditor(this, this->editor->project);
-    bool success = this->regionMapEditor->load(silent);
-    if (!success) {
-        delete this->regionMapEditor;
-        this->regionMapEditor = nullptr;
-        if (!silent) {
-            QMessageBox msgBox(this);
-            QString errorMsg = QString("There was an error opening the region map data. Please see %1 for full error details.\n\n%3")
-                    .arg(getLogPath())
-                    .arg(getMostRecentError());
-            msgBox.critical(nullptr, "Error Opening Region Map Editor", errorMsg);
+    if (!this->regionMapEditor->load(silent)) {
+        // The region map editor either failed to load,
+        // or the user declined configuring their settings.
+        if (!silent && this->regionMapEditor->setupErrored()) {
+            if (this->askToFixRegionMapEditor())
+                return true;
         }
+        delete this->regionMapEditor;
         return false;
     }
 
     return true;
 }
 
+bool MainWindow::askToFixRegionMapEditor() {
+    QMessageBox msgBox;
+    msgBox.setIcon(QMessageBox::Critical);
+    msgBox.setText(QString("There was an error opening the region map data. Please see %1 for full error details.").arg(getLogPath()));
+    msgBox.setDetailedText(getMostRecentError());
+    msgBox.setStandardButtons(QMessageBox::Ok);
+    msgBox.setDefaultButton(QMessageBox::Ok);
+    auto reconfigButton = msgBox.addButton("Reconfigure", QMessageBox::ActionRole);
+    msgBox.exec();
+    if (msgBox.clickedButton() == reconfigButton) {
+        if (this->regionMapEditor->reconfigure()) {
+            // User fixed error
+            return true;
+        }
+        if (this->regionMapEditor->setupErrored()) {
+            // User's new settings still fail, show error and ask again
+            return this->askToFixRegionMapEditor();
+        }
+    }
+    // User accepted error
+    return false;
+}
+
 void MainWindow::closeSupplementaryWindows() {
     delete this->tilesetEditor;
     delete this->regionMapEditor;

diff --git a/src/ui/regionmapeditor.cpp b/src/ui/regionmapeditor.cpp
@@ -28,6 +28,8 @@ RegionMapEditor::RegionMapEditor(QWidget *parent, Project *project) :
 {
     this->ui->setupUi(this);
     this->project = project;
+    this->configFilepath = QString("%1/%2").arg(this->project->root).arg(projectConfig.getFilePath(ProjectFilePath::json_region_porymap_cfg));
+    this->mapSectionFilepath = QString("%1/%2").arg(this->project->root).arg(projectConfig.getFilePath(ProjectFilePath::json_region_map_entries));
     this->initShortcuts();
     this->restoreWindowState();
 }
@@ -110,12 +112,10 @@ void RegionMapEditor::applyUserShortcuts() {
 bool RegionMapEditor::loadRegionMapEntries() {
     this->region_map_entries.clear();
 
-    QString regionMapSectionFilepath = QString("%1/%2").arg(this->project->root).arg(projectConfig.getFilePath(ProjectFilePath::json_region_map_entries));
-
     ParseUtil parser;
     QJsonDocument sectionsDoc;
-    if (!parser.tryParseJsonFile(&sectionsDoc, regionMapSectionFilepath)) {
-        logError(QString("Failed to read map data from %1").arg(regionMapSectionFilepath));
+    if (!parser.tryParseJsonFile(&sectionsDoc, this->mapSectionFilepath)) {
+        logError(QString("Failed to read map data from %1").arg(this->mapSectionFilepath));
         return false;
     }
 
@@ -140,11 +140,9 @@ bool RegionMapEditor::loadRegionMapEntries() {
 }
 
 bool RegionMapEditor::saveRegionMapEntries() {
-    QString regionMapSectionFilepath = QString("%1/%2").arg(this->project->root).arg(projectConfig.getFilePath(ProjectFilePath::json_region_map_entries));
-
-    QFile sectionsFile(regionMapSectionFilepath);
+    QFile sectionsFile(this->mapSectionFilepath);
     if (!sectionsFile.open(QIODevice::WriteOnly)) {
-        logError(QString("Error: Could not open %1 for writing").arg(regionMapSectionFilepath));
+        logError(QString("Could not open %1 for writing").arg(this->mapSectionFilepath));
         return false;
     }
 
@@ -298,7 +296,7 @@ bool RegionMapEditor::buildConfigDialog() {
     form.addRow(addMapButton);
 
     // allow user to add region maps
-    connect(addMapButton, &QPushButton::clicked, [this, regionMapList] {
+    connect(addMapButton, &QPushButton::clicked, [this, regionMapList, &updateJsonFromList] {
         poryjson::Json resultJson = configRegionMapDialog();
         poryjson::Json::object resultObj = resultJson.object_items();
 
@@ -310,6 +308,7 @@ bool RegionMapEditor::buildConfigDialog() {
         newItem->setText(resultObj["alias"].string_value());
         newItem->setData(Qt::UserRole, resultStr);
         regionMapList->addItem(newItem);
+        updateJsonFromList();
     });
 
     QPushButton *delMapButton = new QPushButton("Delete Selected Region Map");
@@ -403,16 +402,6 @@ bool RegionMapEditor::verifyConfig(poryjson::Json cfg) {
         logError("Region map config json has no map list.");
         return false;
     }
-
-    OrderedJson::array arr = obj["region_maps"].array_items();
-
-    for (auto ref : arr) {
-        RegionMap tempMap(this->project);
-        if (!tempMap.loadMapData(ref)) {
-            return false;
-        }
-    }
-
     return true;
 }
 
@@ -476,6 +465,7 @@ bool RegionMapEditor::setup() {
         if (!newMap->loadMapData(o)) {
             delete newMap;
             // TODO: consider continue, just reporting error loading single map?
+            this->setupError = true;
             return false;
         }
 
@@ -498,26 +488,21 @@ bool RegionMapEditor::setup() {
     if (!region_maps.empty()) {
         setRegionMap(region_maps.begin()->second);
     }
+    this->setupError = false;
     return true;
 }
 
 bool RegionMapEditor::load(bool silent) {
     // check for config json file
-    QString jsonConfigFilepath = this->project->root + "/" + projectConfig.getFilePath(ProjectFilePath::json_region_porymap_cfg);
-
     bool badConfig = true;
-
-    if (QFile::exists(jsonConfigFilepath)) {
-        logInfo("Region map configuration file found.");
+    if (QFile::exists(this->configFilepath)) {
         ParseUtil parser;
         OrderedJson::object obj;
-        if (parser.tryParseOrderedJsonFile(&obj, jsonConfigFilepath)) {
+        if (parser.tryParseOrderedJsonFile(&obj, this->configFilepath)) {
             this->rmConfigJson = OrderedJson(obj);
             this->configSaved = true;
         }
         badConfig = !verifyConfig(this->rmConfigJson);
-    } else {
-        logWarn("Region Map config file not found.");
     }
 
     if (badConfig) {
@@ -533,14 +518,15 @@ bool RegionMapEditor::load(bool silent) {
         if (warning.exec() == QMessageBox::Ok) {
             // there is a separate window that allows to load multiple region maps, 
             if (!buildConfigDialog()) {
-                logError("Region map loading interrupted [user]");
+                // User canceled config set up
                 return false;
             }
         } else {
-            // do not open editor
-            logError("Region map loading interrupted [user]");
+            // User declined config set up
             return false;
         }
+    } else {
+        logInfo("Successfully loaded region map configuration file.");
     }
 
     return setup();
@@ -582,10 +568,9 @@ void RegionMapEditor::saveConfig() {
     mapsObject["region_maps"] = mapArray;
 
     OrderedJson newConfigJson(mapsObject);
-    QString filepath = QString("%1/%2").arg(this->project->root).arg(projectConfig.getFilePath(ProjectFilePath::json_region_porymap_cfg));
-    QFile file(filepath);
+    QFile file(this->configFilepath);
     if (!file.open(QIODevice::WriteOnly)) {
-        logError(QString("Error: Could not open %1 for writing").arg(filepath));
+        logError(QString("Could not open %1 for writing").arg(this->configFilepath));
         return;
     }
     OrderedJsonDoc jsonDoc(&newConfigJson);
@@ -614,6 +599,11 @@ void RegionMapEditor::on_actionSave_All_triggered() {
 }
 
 void RegionMapEditor::on_action_Configure_triggered() {
+    reconfigure();
+}
+
+bool RegionMapEditor::reconfigure() {
+    this->setupError = false;
     if (this->modified()) {
         QMessageBox warning;
         warning.setIcon(QMessageBox::Warning);
@@ -624,15 +614,16 @@ void RegionMapEditor::on_action_Configure_triggered() {
 
         if (warning.exec() == QMessageBox::Ok) {
             if (buildConfigDialog()) {
-                reload();
+                return reload();
             }
         }
     }
     else {
         if (buildConfigDialog()) {
-            reload();
+            return reload();
         }
     }
+    return false;
 }
 
 void RegionMapEditor::displayRegionMap() {