From 40dc9b0d234cf960c5c48fce223166bce720a1db Mon Sep 17 00:00:00 2001 From: Ihar Hubchyk Date: Thu, 25 Jan 2024 21:10:09 +0800 Subject: [PATCH 1/7] Change the logic to handle the future map loading relates to #6845 --- src/fheroes2/editor/editor_interface.cpp | 48 ++++++++++++++------ src/fheroes2/editor/editor_interface.h | 2 +- src/fheroes2/editor/history_manager.cpp | 6 +++ src/fheroes2/editor/history_manager.h | 7 +++ src/fheroes2/maps/map_format_helper.cpp | 25 ++++++++--- src/fheroes2/maps/map_format_helper.h | 3 +- src/fheroes2/maps/maps_tiles_helper.cpp | 57 +++++++++++++++++++----- src/fheroes2/maps/maps_tiles_helper.h | 2 +- src/fheroes2/world/world_loadmap.cpp | 10 +++++ 9 files changed, 125 insertions(+), 35 deletions(-) diff --git a/src/fheroes2/editor/editor_interface.cpp b/src/fheroes2/editor/editor_interface.cpp index bae86453c70..fa2f5d4facc 100644 --- a/src/fheroes2/editor/editor_interface.cpp +++ b/src/fheroes2/editor/editor_interface.cpp @@ -990,9 +990,12 @@ namespace Interface return; } - const fheroes2::ActionCreator action( _historyManager, _mapFormat ); + fheroes2::ActionCreator action( _historyManager, _mapFormat ); - setObjectOnTile( tile, Maps::ObjectGroup::LANDSCAPE_TOWN_BASEMENTS, basementId ); + if ( !setObjectOnTile( tile, Maps::ObjectGroup::LANDSCAPE_TOWN_BASEMENTS, basementId ) ) { + action.abort(); + return; + } // Since the whole object consists of multiple "objects" we have to put the same ID for all of them. // Every time an object is being placed on a map the counter is going to be increased by 1. @@ -1002,17 +1005,26 @@ namespace Interface Maps::setLastObjectUID( objectId ); - setObjectOnTile( tile, groupType, type ); + if ( !setObjectOnTile( tile, groupType, type ) ) { + action.abort(); + return; + } // Add flags. assert( tile.GetIndex() > 0 && tile.GetIndex() < world.w() * world.h() - 1 ); Maps::setLastObjectUID( objectId ); - setObjectOnTile( world.GetTiles( tile.GetIndex() - 1 ), Maps::ObjectGroup::LANDSCAPE_FLAGS, color * 2 ); + if ( !setObjectOnTile( world.GetTiles( tile.GetIndex() - 1 ), Maps::ObjectGroup::LANDSCAPE_FLAGS, color * 2 ) ) { + action.abort(); + return; + } Maps::setLastObjectUID( objectId ); - setObjectOnTile( world.GetTiles( tile.GetIndex() + 1 ), Maps::ObjectGroup::LANDSCAPE_FLAGS, color * 2 + 1 ); + if ( !setObjectOnTile( world.GetTiles( tile.GetIndex() + 1 ), Maps::ObjectGroup::LANDSCAPE_FLAGS, color * 2 + 1 ) ) { + action.abort(); + return; + } } else if ( groupType == Maps::ObjectGroup::ADVENTURE_MINES ) { int32_t type = -1; @@ -1042,9 +1054,12 @@ namespace Interface return; } - const fheroes2::ActionCreator action( _historyManager, _mapFormat ); + fheroes2::ActionCreator action( _historyManager, _mapFormat ); - setObjectOnTile( tile, groupType, type ); + if ( !setObjectOnTile( tile, groupType, type ) ) { + action.abort(); + return; + } // TODO: Place owner flag according to the color state. } @@ -1125,26 +1140,33 @@ namespace Interface } } - void EditorInterface::setObjectOnTile( Maps::Tiles & tile, const Maps::ObjectGroup groupType, const int32_t objectIndex ) + bool EditorInterface::setObjectOnTile( Maps::Tiles & tile, const Maps::ObjectGroup groupType, const int32_t objectIndex ) { const auto & objectInfo = getObjectInfo( groupType, objectIndex ); if ( objectInfo.empty() ) { // Check your logic as you are trying to insert an empty object! assert( 0 ); - return; + return false; + } + + _redraw |= mapUpdateFlags; + + if ( !Maps::setObjectOnTile( tile, objectInfo, true ) ) { + return false; } - Maps::setObjectOnTile( tile, objectInfo, true ); Maps::addObjectToMap( _mapFormat, tile.GetIndex(), groupType, static_cast( objectIndex ) ); - _redraw |= mapUpdateFlags; + return true; } void EditorInterface::setObjectOnTileAsAction( Maps::Tiles & tile, const Maps::ObjectGroup groupType, const int32_t objectIndex ) { - const fheroes2::ActionCreator action( _historyManager, _mapFormat ); + fheroes2::ActionCreator action( _historyManager, _mapFormat ); - setObjectOnTile( tile, groupType, objectIndex ); + if ( !setObjectOnTile( tile, groupType, objectIndex ) ) { + action.abort(); + } } bool EditorInterface::loadMap( const std::string & filePath ) diff --git a/src/fheroes2/editor/editor_interface.h b/src/fheroes2/editor/editor_interface.h index fb1d88bdb7c..1a293d9b67d 100644 --- a/src/fheroes2/editor/editor_interface.h +++ b/src/fheroes2/editor/editor_interface.h @@ -134,7 +134,7 @@ namespace Interface // Do nothing. } - void setObjectOnTile( Maps::Tiles & tile, const Maps::ObjectGroup groupType, const int32_t objectIndex ); + bool setObjectOnTile( Maps::Tiles & tile, const Maps::ObjectGroup groupType, const int32_t objectIndex ); void setObjectOnTileAsAction( Maps::Tiles & tile, const Maps::ObjectGroup groupType, const int32_t objectIndex ); diff --git a/src/fheroes2/editor/history_manager.cpp b/src/fheroes2/editor/history_manager.cpp index d9df03d8e44..ed4647758e6 100644 --- a/src/fheroes2/editor/history_manager.cpp +++ b/src/fheroes2/editor/history_manager.cpp @@ -112,6 +112,7 @@ namespace fheroes2 ActionCreator::~ActionCreator() { + auto * action = dynamic_cast( _action.get() ); if ( action == nullptr ) { // How is it even possible? @@ -120,6 +121,11 @@ namespace fheroes2 } try { + if ( _aborted ) { + action->undo(); + return; + } + if ( action->prepare() ) { _manager.add( std::move( _action ) ); } diff --git a/src/fheroes2/editor/history_manager.h b/src/fheroes2/editor/history_manager.h index d3e0a2b63db..43a42aa5065 100644 --- a/src/fheroes2/editor/history_manager.h +++ b/src/fheroes2/editor/history_manager.h @@ -58,10 +58,17 @@ namespace fheroes2 ActionCreator & operator=( const ActionCreator & ) = delete; + void abort() + { + _aborted = true; + } + private: HistoryManager & _manager; std::unique_ptr _action; + + bool _aborted{ false }; }; class HistoryManager diff --git a/src/fheroes2/maps/map_format_helper.cpp b/src/fheroes2/maps/map_format_helper.cpp index 1e74fcac7ae..2f500b8a7fd 100644 --- a/src/fheroes2/maps/map_format_helper.cpp +++ b/src/fheroes2/maps/map_format_helper.cpp @@ -61,6 +61,17 @@ namespace Maps { world.generateForEditor( map.size ); + if ( !readAllTiles( map ) ) { + return false; + } + + world.updatePassabilities(); + + return true; + } + + bool readAllTiles( const Map_Format::MapFormat & map ) + { assert( static_cast( world.w() ) * world.h() == map.tiles.size() ); // We must clear all tiles before writing something on them. @@ -90,11 +101,11 @@ namespace Maps for ( const auto & info : sortedObjects ) { assert( info.info != nullptr ); - readTileObject( world.GetTiles( info.tileIndex ), *info.info ); + if ( !readTileObject( world.GetTiles( info.tileIndex ), *info.info ) ) { + return false; + } } - world.updatePassabilities(); - return true; } @@ -120,26 +131,26 @@ namespace Maps tile.setTerrain( info.terrainIndex, info.terrainFlag & 2, info.terrainFlag & 1 ); } - void readTileObject( Tiles & tile, const Map_Format::ObjectInfo & object ) + bool readTileObject( Tiles & tile, const Map_Format::ObjectInfo & object ) { const auto & objectInfos = getObjectsByGroup( object.group ); if ( object.index >= objectInfos.size() ) { // This is a bad map format! assert( 0 ); - return; + return false; } // Object UID is set through global object UID counter. Therefore, we need to update it before running the operation. if ( object.id == 0 ) { // This object UID is not set! assert( 0 ); - return; + return false; } setLastObjectUID( object.id - 1 ); // We don't update map passabilities as it is a very expensive process. // Let's do it once everything is being loaded. - setObjectOnTile( tile, objectInfos[object.index], false ); + return setObjectOnTile( tile, objectInfos[object.index], false ); } void writeTile( const Tiles & tile, Map_Format::TileInfo & info ) diff --git a/src/fheroes2/maps/map_format_helper.h b/src/fheroes2/maps/map_format_helper.h index e729218da82..2994416cf00 100644 --- a/src/fheroes2/maps/map_format_helper.h +++ b/src/fheroes2/maps/map_format_helper.h @@ -36,11 +36,12 @@ namespace Maps enum class ObjectGroup : int32_t; bool readMapInEditor( const Map_Format::MapFormat & map ); + bool readAllTiles( const Map_Format::MapFormat & map ); bool saveMapInEditor( Map_Format::MapFormat & map ); void readTileTerrain( Tiles & tile, const Map_Format::TileInfo & info ); - void readTileObject( Tiles & tile, const Map_Format::ObjectInfo & object ); + bool readTileObject( Tiles & tile, const Map_Format::ObjectInfo & object ); void writeTile( const Tiles & tile, Map_Format::TileInfo & info ); diff --git a/src/fheroes2/maps/maps_tiles_helper.cpp b/src/fheroes2/maps/maps_tiles_helper.cpp index b6ed2cfe06a..6d1535b2386 100644 --- a/src/fheroes2/maps/maps_tiles_helper.cpp +++ b/src/fheroes2/maps/maps_tiles_helper.cpp @@ -1203,19 +1203,40 @@ namespace } } - void placeObjectOnTile( const Maps::Tiles & tile, const Maps::ObjectInfo & info ) + bool placeObjectOnTile( const Maps::Tiles & tile, const Maps::ObjectInfo & info ) { assert( !info.empty() ); - const uint32_t uid = Maps::getNewObjectUID(); + // Verify that the object is allowed to be placed. const fheroes2::Point mainTilePos = tile.GetCenter(); for ( const auto & partInfo : info.groundLevelParts ) { const fheroes2::Point pos = mainTilePos + partInfo.tileOffset; const bool isMainObject = ( partInfo.layerType != Maps::SHADOW_LAYER && partInfo.layerType != Maps::TERRAIN_LAYER ); - if ( !Maps::isValidAbsPoint( pos.x, pos.y ) ) { + if ( !Maps::isValidAbsPoint( pos.x, pos.y ) && isMainObject ) { // Only shadow and terrain layer object parts are allowed not to be placed. + assert( 0 ); + return false; + } + } + + for ( const auto & partInfo : info.topLevelParts ) { + const fheroes2::Point pos = mainTilePos + partInfo.tileOffset; + if ( !Maps::isValidAbsPoint( pos.x, pos.y ) ) { + // This shouldn't happen as the object must be verified before placement. + assert( 0 ); + return false; + } + } + + const uint32_t uid = Maps::getNewObjectUID(); + + for ( const auto & partInfo : info.groundLevelParts ) { + const fheroes2::Point pos = mainTilePos + partInfo.tileOffset; + const bool isMainObject = ( partInfo.layerType != Maps::SHADOW_LAYER && partInfo.layerType != Maps::TERRAIN_LAYER ); + + if ( !Maps::isValidAbsPoint( pos.x, pos.y ) ) { assert( !isMainObject ); continue; } @@ -1273,6 +1294,8 @@ namespace currentTile.SetObject( partInfo.objectType ); } } + + return true; } } @@ -3208,7 +3231,7 @@ namespace Maps return needRedraw; } - void setObjectOnTile( Tiles & tile, const ObjectInfo & info, const bool updateMapPassabilities ) + bool setObjectOnTile( Tiles & tile, const ObjectInfo & info, const bool updateMapPassabilities ) { assert( !info.empty() ); @@ -3217,21 +3240,27 @@ namespace Maps setMonsterOnTile( tile, static_cast( info.metadata[0] ), 0 ); // Since setMonsterOnTile() function interprets 0 as a random number of monsters it is important to set the correct value. setMonsterCountOnTile( tile, 0 ); - return; + return true; case MP2::OBJ_RESOURCE: // Setting just 1 resource is enough. It doesn't matter as we are not saving this value into the map format. - placeObjectOnTile( tile, info ); + if ( !placeObjectOnTile( tile, info ) ) { + return false; + } setResourceOnTile( tile, static_cast( info.metadata[0] ), 1 ); - return; + return true; case MP2::OBJ_ARTIFACT: - placeObjectOnTile( tile, info ); + if ( !placeObjectOnTile( tile, info ) ) { + return false; + } // The artifact ID is stored in metadata[0]. It is used by the other engine functions. tile.metadata()[0] = info.metadata[0]; - return; + return true; case MP2::OBJ_ALCHEMIST_LAB: case MP2::OBJ_MINE: case MP2::OBJ_SAWMILL: - placeObjectOnTile( tile, info ); + if ( !placeObjectOnTile( tile, info ) ) { + return false; + } // Set resource type and income per day. tile.metadata()[0] = info.metadata[0]; tile.metadata()[1] = info.metadata[1]; @@ -3239,15 +3268,19 @@ namespace Maps if ( updateMapPassabilities ) { world.updatePassabilities(); } - return; + return true;; default: break; } - placeObjectOnTile( tile, info ); + if ( !placeObjectOnTile( tile, info ) ) { + return false; + } if ( updateMapPassabilities ) { world.updatePassabilities(); } + + return true; } } diff --git a/src/fheroes2/maps/maps_tiles_helper.h b/src/fheroes2/maps/maps_tiles_helper.h index 6cda6fcfa29..02920426dc1 100644 --- a/src/fheroes2/maps/maps_tiles_helper.h +++ b/src/fheroes2/maps/maps_tiles_helper.h @@ -188,5 +188,5 @@ namespace Maps bool eraseObjectsOnTiles( const int32_t startTileId, const int32_t endTileId, const uint32_t objectTypesToErase ); bool eraseOjects( Tiles & tile, const uint32_t objectTypesToErase ); - void setObjectOnTile( Tiles & tile, const ObjectInfo & info, const bool updateMapPassabilities ); + bool setObjectOnTile( Tiles & tile, const ObjectInfo & info, const bool updateMapPassabilities ); } diff --git a/src/fheroes2/world/world_loadmap.cpp b/src/fheroes2/world/world_loadmap.cpp index 2e900c4c1a5..9c467bf3132 100644 --- a/src/fheroes2/world/world_loadmap.cpp +++ b/src/fheroes2/world/world_loadmap.cpp @@ -43,6 +43,7 @@ #include "heroes.h" #include "kingdom.h" #include "logging.h" +#include "map_format_helper.h" #include "map_format_info.h" #include "maps.h" #include "maps_fileinfo.h" @@ -672,6 +673,15 @@ bool World::loadResurrectionMap( const std::string & filename ) return false; } + width = map.size; + height = map.size; + + vec_tiles.resize( width * height ); + + if ( !Maps::readAllTiles( map ) ) { + return false; + } + // TODO: return true once we add logic for loading an entire map. return false; } From 0aef380562526358ccf1fa14fed4c01db5ad9b67 Mon Sep 17 00:00:00 2001 From: Ihar Hubchyk Date: Thu, 25 Jan 2024 21:11:25 +0800 Subject: [PATCH 2/7] Fix code style --- src/fheroes2/editor/history_manager.cpp | 1 - src/fheroes2/maps/maps_tiles_helper.cpp | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/fheroes2/editor/history_manager.cpp b/src/fheroes2/editor/history_manager.cpp index ed4647758e6..8178afb805e 100644 --- a/src/fheroes2/editor/history_manager.cpp +++ b/src/fheroes2/editor/history_manager.cpp @@ -112,7 +112,6 @@ namespace fheroes2 ActionCreator::~ActionCreator() { - auto * action = dynamic_cast( _action.get() ); if ( action == nullptr ) { // How is it even possible? diff --git a/src/fheroes2/maps/maps_tiles_helper.cpp b/src/fheroes2/maps/maps_tiles_helper.cpp index 6d1535b2386..e0126a652d0 100644 --- a/src/fheroes2/maps/maps_tiles_helper.cpp +++ b/src/fheroes2/maps/maps_tiles_helper.cpp @@ -3268,7 +3268,7 @@ namespace Maps if ( updateMapPassabilities ) { world.updatePassabilities(); } - return true;; + return true; default: break; } From 1a8d8e20d6f7680044ca9e90dcc170c643eefc46 Mon Sep 17 00:00:00 2001 From: Ihar Hubchyk Date: Thu, 25 Jan 2024 21:12:42 +0800 Subject: [PATCH 3/7] Update copyright year --- src/fheroes2/editor/history_manager.cpp | 2 +- src/fheroes2/editor/history_manager.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/fheroes2/editor/history_manager.cpp b/src/fheroes2/editor/history_manager.cpp index 8178afb805e..271ad3b6fb5 100644 --- a/src/fheroes2/editor/history_manager.cpp +++ b/src/fheroes2/editor/history_manager.cpp @@ -1,6 +1,6 @@ /*************************************************************************** * fheroes2: https://github.com/ihhub/fheroes2 * - * Copyright (C) 2023 * + * Copyright (C) 2023 - 2024 * * * * This program is free software; you can redistribute it and/or modify * * it under the terms of the GNU General Public License as published by * diff --git a/src/fheroes2/editor/history_manager.h b/src/fheroes2/editor/history_manager.h index 43a42aa5065..e7ce19927ab 100644 --- a/src/fheroes2/editor/history_manager.h +++ b/src/fheroes2/editor/history_manager.h @@ -1,6 +1,6 @@ /*************************************************************************** * fheroes2: https://github.com/ihhub/fheroes2 * - * Copyright (C) 2023 * + * Copyright (C) 2023 - 2024 * * * * This program is free software; you can redistribute it and/or modify * * it under the terms of the GNU General Public License as published by * From 7dd336a48798876092e56dfc101fe7bdcc3e98d8 Mon Sep 17 00:00:00 2001 From: Ihar Hubchyk Date: Thu, 25 Jan 2024 21:15:48 +0800 Subject: [PATCH 4/7] Comply with clang-tidy --- src/fheroes2/world/world_loadmap.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fheroes2/world/world_loadmap.cpp b/src/fheroes2/world/world_loadmap.cpp index 9c467bf3132..c8e018fe8fe 100644 --- a/src/fheroes2/world/world_loadmap.cpp +++ b/src/fheroes2/world/world_loadmap.cpp @@ -676,7 +676,7 @@ bool World::loadResurrectionMap( const std::string & filename ) width = map.size; height = map.size; - vec_tiles.resize( width * height ); + vec_tiles.resize( static_cast( width ) * height ); if ( !Maps::readAllTiles( map ) ) { return false; From 71c3c7be7aea7d2e7a7dd9d2bb2759521b30af09 Mon Sep 17 00:00:00 2001 From: Ihar Hubchyk Date: Thu, 25 Jan 2024 21:28:14 +0800 Subject: [PATCH 5/7] Add more logic --- src/fheroes2/world/world_loadmap.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/fheroes2/world/world_loadmap.cpp b/src/fheroes2/world/world_loadmap.cpp index c8e018fe8fe..a4d71111064 100644 --- a/src/fheroes2/world/world_loadmap.cpp +++ b/src/fheroes2/world/world_loadmap.cpp @@ -682,8 +682,16 @@ bool World::loadResurrectionMap( const std::string & filename ) return false; } - // TODO: return true once we add logic for loading an entire map. - return false; + // Clear artifact flags to correctly generate random artifacts. + fheroes2::ResetArtifactStats(); + + if ( !ProcessNewMap( filename, false ) ) { + return false; + } + + DEBUG_LOG( DBG_GAME, DBG_INFO, "Loading of FH2 map is completed." ) + + return true; } bool World::ProcessNewMap( const std::string & filename, const bool checkPoLObjects ) From d114bccec1cd3e41a7a355cd3e43a129102a3da9 Mon Sep 17 00:00:00 2001 From: Ihar Hubchyk Date: Fri, 26 Jan 2024 22:54:39 +0800 Subject: [PATCH 6/7] Address comments --- src/fheroes2/editor/editor_interface.cpp | 38 ++++++++++++++++-------- src/fheroes2/editor/history_manager.cpp | 9 ++---- src/fheroes2/editor/history_manager.h | 15 +++++----- 3 files changed, 35 insertions(+), 27 deletions(-) diff --git a/src/fheroes2/editor/editor_interface.cpp b/src/fheroes2/editor/editor_interface.cpp index fa2f5d4facc..ad63bc54a54 100644 --- a/src/fheroes2/editor/editor_interface.cpp +++ b/src/fheroes2/editor/editor_interface.cpp @@ -472,16 +472,20 @@ namespace Interface if ( isCursorOverGamearea && _editorPanel.getBrushArea().width == 0 ) { if ( _editorPanel.isTerrainEdit() ) { // Fill the selected area in terrain edit mode. - const fheroes2::ActionCreator action( _historyManager, _mapFormat ); + fheroes2::ActionCreator action( _historyManager, _mapFormat ); const int groundId = _editorPanel.selectedGroundType(); Maps::setTerrainOnTiles( _selectedTile, _tileUnderCursor, groundId ); + + action.commit(); } else if ( _editorPanel.isEraseMode() ) { // Erase objects in the selected area. - const fheroes2::ActionCreator action( _historyManager, _mapFormat ); + fheroes2::ActionCreator action( _historyManager, _mapFormat ); Maps::eraseObjectsOnTiles( _selectedTile, _tileUnderCursor, _editorPanel.getEraseTypes() ); + + action.commit(); } } @@ -667,7 +671,7 @@ namespace Interface const int groundId = _editorPanel.selectedGroundType(); - const fheroes2::ActionCreator action( _historyManager, _mapFormat ); + fheroes2::ActionCreator action( _historyManager, _mapFormat ); if ( brushSize.width > 0 ) { const fheroes2::Point indices = getBrushAreaIndicies( brushSize, tileIndex ); @@ -684,37 +688,47 @@ namespace Interface } _redraw |= mapUpdateFlags; + + action.commit(); } else if ( _editorPanel.isRoadDraw() ) { - const fheroes2::ActionCreator action( _historyManager, _mapFormat ); + fheroes2::ActionCreator action( _historyManager, _mapFormat ); if ( Maps::updateRoadOnTile( tile, true ) ) { _redraw |= mapUpdateFlags; + + action.commit(); } } else if ( _editorPanel.isStreamDraw() ) { - const fheroes2::ActionCreator action( _historyManager, _mapFormat ); + fheroes2::ActionCreator action( _historyManager, _mapFormat ); if ( Maps::updateStreamOnTile( tile, true ) ) { _redraw |= mapUpdateFlags; + + action.commit(); } } else if ( _editorPanel.isEraseMode() ) { const fheroes2::Rect brushSize = _editorPanel.getBrushArea(); assert( brushSize.width == brushSize.height ); - const fheroes2::ActionCreator action( _historyManager, _mapFormat ); + fheroes2::ActionCreator action( _historyManager, _mapFormat ); if ( brushSize.width > 1 ) { const fheroes2::Point indices = getBrushAreaIndicies( brushSize, tileIndex ); if ( Maps::eraseObjectsOnTiles( indices.x, indices.y, _editorPanel.getEraseTypes() ) ) { _redraw |= mapUpdateFlags; + + action.commit(); } } else { if ( Maps::eraseOjects( tile, _editorPanel.getEraseTypes() ) ) { _redraw |= mapUpdateFlags; + + action.commit(); } if ( brushSize.width == 0 ) { @@ -993,7 +1007,6 @@ namespace Interface fheroes2::ActionCreator action( _historyManager, _mapFormat ); if ( !setObjectOnTile( tile, Maps::ObjectGroup::LANDSCAPE_TOWN_BASEMENTS, basementId ) ) { - action.abort(); return; } @@ -1006,7 +1019,6 @@ namespace Interface Maps::setLastObjectUID( objectId ); if ( !setObjectOnTile( tile, groupType, type ) ) { - action.abort(); return; } @@ -1015,16 +1027,16 @@ namespace Interface Maps::setLastObjectUID( objectId ); if ( !setObjectOnTile( world.GetTiles( tile.GetIndex() - 1 ), Maps::ObjectGroup::LANDSCAPE_FLAGS, color * 2 ) ) { - action.abort(); return; } Maps::setLastObjectUID( objectId ); if ( !setObjectOnTile( world.GetTiles( tile.GetIndex() + 1 ), Maps::ObjectGroup::LANDSCAPE_FLAGS, color * 2 + 1 ) ) { - action.abort(); return; } + + action.commit(); } else if ( groupType == Maps::ObjectGroup::ADVENTURE_MINES ) { int32_t type = -1; @@ -1057,11 +1069,11 @@ namespace Interface fheroes2::ActionCreator action( _historyManager, _mapFormat ); if ( !setObjectOnTile( tile, groupType, type ) ) { - action.abort(); return; } // TODO: Place owner flag according to the color state. + action.commit(); } else if ( groupType == Maps::ObjectGroup::ADVENTURE_DWELLINGS ) { const auto & objectInfo = getObjectInfo( groupType, _editorPanel.getSelectedObjectType() ); @@ -1164,8 +1176,8 @@ namespace Interface { fheroes2::ActionCreator action( _historyManager, _mapFormat ); - if ( !setObjectOnTile( tile, groupType, objectIndex ) ) { - action.abort(); + if ( setObjectOnTile( tile, groupType, objectIndex ) ) { + action.commit(); } } diff --git a/src/fheroes2/editor/history_manager.cpp b/src/fheroes2/editor/history_manager.cpp index 271ad3b6fb5..490fbabfb99 100644 --- a/src/fheroes2/editor/history_manager.cpp +++ b/src/fheroes2/editor/history_manager.cpp @@ -110,21 +110,16 @@ namespace fheroes2 _action = std::make_unique( mapFormat ); } - ActionCreator::~ActionCreator() + void ActionCreator::commit() { auto * action = dynamic_cast( _action.get() ); if ( action == nullptr ) { - // How is it even possible? + // How is it even possible? Did you call this method twice? assert( 0 ); return; } try { - if ( _aborted ) { - action->undo(); - return; - } - if ( action->prepare() ) { _manager.add( std::move( _action ) ); } diff --git a/src/fheroes2/editor/history_manager.h b/src/fheroes2/editor/history_manager.h index e7ce19927ab..c8162dd2501 100644 --- a/src/fheroes2/editor/history_manager.h +++ b/src/fheroes2/editor/history_manager.h @@ -52,23 +52,24 @@ namespace fheroes2 public: explicit ActionCreator( HistoryManager & manager, Maps::Map_Format::MapFormat & mapFormat ); - ~ActionCreator(); + ~ActionCreator() + { + if ( _action ) { + // The action wasn't committed. Undo all the changes. + _action->undo(); + } + } ActionCreator( const ActionCreator & ) = delete; ActionCreator & operator=( const ActionCreator & ) = delete; - void abort() - { - _aborted = true; - } + void commit(); private: HistoryManager & _manager; std::unique_ptr _action; - - bool _aborted{ false }; }; class HistoryManager From 51903e89ef565c9b30dd3663ee0688be6556ba82 Mon Sep 17 00:00:00 2001 From: Ihar Hubchyk Date: Sat, 27 Jan 2024 10:28:38 +0800 Subject: [PATCH 7/7] Remove try..catch --- src/fheroes2/editor/history_manager.cpp | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/fheroes2/editor/history_manager.cpp b/src/fheroes2/editor/history_manager.cpp index 490fbabfb99..18beec87c98 100644 --- a/src/fheroes2/editor/history_manager.cpp +++ b/src/fheroes2/editor/history_manager.cpp @@ -119,14 +119,8 @@ namespace fheroes2 return; } - try { - if ( action->prepare() ) { - _manager.add( std::move( _action ) ); - } - } - catch ( ... ) { - // If an exception happens here then something is very wrong with the code. - assert( 0 ); + if ( action->prepare() ) { + _manager.add( std::move( _action ) ); } } }