From b008e665eaf5d9a9dc223a349ac5944876ca6f43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thorbj=C3=B8rn=20Lindeijer?= Date: Sun, 2 Feb 2025 12:06:42 +0100 Subject: [PATCH] AutoMapping: Removed the "touched layers" optimization The "AutoMap While Drawing" had an optimization where it skipped entire rule maps when none of their input layers had been "touched". This was based on the assumption that its rules would not need to be re-evaluated anyway. Unfortunately, we can't make this assumption in general. The effect of certain rules might be relevant even if none of their input or output layers have been touched. --- src/tiled/automapper.cpp | 10 +--------- src/tiled/automapper.h | 11 +---------- src/tiled/automapperwrapper.cpp | 15 +-------------- src/tiled/automapperwrapper.h | 3 +-- src/tiled/automappingmanager.cpp | 29 +++++++++-------------------- src/tiled/automappingmanager.h | 8 ++++---- 6 files changed, 17 insertions(+), 59 deletions(-) diff --git a/src/tiled/automapper.cpp b/src/tiled/automapper.cpp index 08b257457e..f0c85efc2b 100644 --- a/src/tiled/automapper.cpp +++ b/src/tiled/automapper.cpp @@ -155,16 +155,11 @@ AutoMapper::AutoMapper(std::unique_ptr rulesMap) AutoMapper::~AutoMapper() = default; -QString AutoMapper::rulesMapFileName() const +const QString &AutoMapper::rulesMapFileName() const { return mRulesMap->fileName; } -bool AutoMapper::ruleLayerNameUsed(const QString &ruleLayerName) const -{ - return mRuleMapSetup.mInputLayerNames.contains(ruleLayerName); -} - template bool checkOption(const QString &propertyName, const QVariant &propertyValue, @@ -1366,9 +1361,6 @@ void AutoMapper::copyMapRegion(const Rule &rule, QPoint offset, if (!rule.options.ignoreLock && !toTileLayer->isUnlocked()) continue; - if (!context.touchedTileLayers.isEmpty()) - appendUnique(context.touchedTileLayers, toTileLayer); - for (const QRect &rect : rule.outputRegion) { copyTileRegion(tileOutput.tileLayer, rect, toTileLayer, rect.x() + offset.x(), rect.y() + offset.y(), diff --git a/src/tiled/automapper.h b/src/tiled/automapper.h index 2dfc382cf4..f4ff96a892 100644 --- a/src/tiled/automapper.h +++ b/src/tiled/automapper.h @@ -256,9 +256,6 @@ struct TILED_EDITOR_EXPORT AutoMappingContext // Clones of existing tile layers that might have been changed in AutoMapper::autoMap std::unordered_map> originalToOutputLayerMapping; - // Used to keep track of touched tile layers (only when initially non-empty) - QVector touchedTileLayers; - private: friend class AutoMapper; @@ -335,13 +332,7 @@ class TILED_EDITOR_EXPORT AutoMapper explicit AutoMapper(std::unique_ptr rulesMap); ~AutoMapper(); - QString rulesMapFileName() const; - - /** - * Checks if the passed \a ruleLayerName is used as input layer in this - * instance of AutoMapper. - */ - bool ruleLayerNameUsed(const QString &ruleLayerName) const; + const QString &rulesMapFileName() const; /** * This needs to be called directly before the autoMap call. diff --git a/src/tiled/automapperwrapper.cpp b/src/tiled/automapperwrapper.cpp index 749b5fbce8..a77e12ffdd 100644 --- a/src/tiled/automapperwrapper.cpp +++ b/src/tiled/automapperwrapper.cpp @@ -37,8 +37,7 @@ using namespace Tiled; AutoMapperWrapper::AutoMapperWrapper(MapDocument *mapDocument, const QVector &autoMappers, - const QRegion &where, - const TileLayer *touchedLayer) + const QRegion &where) : PaintTileLayer(mapDocument) { AutoMappingContext context(mapDocument); @@ -46,11 +45,6 @@ AutoMapperWrapper::AutoMapperWrapper(MapDocument *mapDocument, for (const auto autoMapper : autoMappers) autoMapper->prepareAutoMap(context); - // During "AutoMap while drawing", keep track of the touched layers, so we - // can skip any rule maps that doesn't have these layers as input entirely. - if (touchedLayer) - context.touchedTileLayers.append(touchedLayer); - // use a copy of the region, so each AutoMapper can manipulate it and the // following AutoMappers do see the impact QRegion region(where); @@ -64,13 +58,6 @@ AutoMapperWrapper::AutoMapperWrapper(MapDocument *mapDocument, if (appliedRegionPtr && (!map->infinite() && (mapRect - region).isEmpty())) appliedRegionPtr = nullptr; - if (touchedLayer) { - if (std::none_of(context.touchedTileLayers.cbegin(), - context.touchedTileLayers.cend(), - [&] (const TileLayer *tileLayer) { return autoMapper->ruleLayerNameUsed(tileLayer->name()); })) - continue; - } - autoMapper->autoMap(region, appliedRegionPtr, context); if (appliedRegionPtr) { diff --git a/src/tiled/automapperwrapper.h b/src/tiled/automapperwrapper.h index a83a76a2b5..5b6a22c856 100644 --- a/src/tiled/automapperwrapper.h +++ b/src/tiled/automapperwrapper.h @@ -41,8 +41,7 @@ class AutoMapperWrapper : public PaintTileLayer public: AutoMapperWrapper(MapDocument *mapDocument, const QVector &autoMappers, - const QRegion &where, - const TileLayer *touchedLayer = nullptr); + const QRegion &where); }; } // namespace Tiled diff --git a/src/tiled/automappingmanager.cpp b/src/tiled/automappingmanager.cpp index 0f9b6c8b4e..f1c35ad153 100644 --- a/src/tiled/automappingmanager.cpp +++ b/src/tiled/automappingmanager.cpp @@ -78,18 +78,18 @@ void AutomappingManager::autoMap() } } - autoMapInternal(region, nullptr); + autoMapInternal(region, false); } void AutomappingManager::autoMapRegion(const QRegion ®ion) { - autoMapInternal(region, nullptr); + autoMapInternal(region, false); } -void AutomappingManager::onRegionEdited(const QRegion &where, TileLayer *touchedLayer) +void AutomappingManager::onRegionEdited(const QRegion &where) { if (automappingWhileDrawing) - autoMapInternal(where, touchedLayer); + autoMapInternal(where, true); } void AutomappingManager::onMapFileNameChanged() @@ -99,7 +99,7 @@ void AutomappingManager::onMapFileNameChanged() } void AutomappingManager::autoMapInternal(const QRegion &where, - const TileLayer *touchedLayer) + bool whileDrawing) { mError.clear(); mWarning.clear(); @@ -107,17 +107,15 @@ void AutomappingManager::autoMapInternal(const QRegion &where, if (!mMapDocument) return; - const bool automatic = touchedLayer != nullptr; - // Even if no AutoMapper instance will be executed, we still want to report // any warnings or errors that might have been reported while interpreting // the rule maps. auto reportErrors = qScopeGuard([=] { if (!mWarning.isEmpty()) - emit warningsOccurred(automatic); + emit warningsOccurred(whileDrawing); if (!mError.isEmpty()) - emit errorsOccurred(automatic); + emit errorsOccurred(whileDrawing); }); if (!mLoaded) { @@ -148,17 +146,8 @@ void AutomappingManager::autoMapInternal(const QRegion &where, if (autoMappers.isEmpty()) return; - // Skip this AutoMapping run if none of the loaded rule maps actually use - // the touched layer. - if (touchedLayer) { - if (std::none_of(autoMappers.cbegin(), - autoMappers.cend(), - [=] (const AutoMapper *autoMapper) { return autoMapper->ruleLayerNameUsed(touchedLayer->name()); })) - return; - } - - AutoMapperWrapper *aw = new AutoMapperWrapper(mMapDocument, autoMappers, where, touchedLayer); - aw->setMergeable(automatic); + AutoMapperWrapper *aw = new AutoMapperWrapper(mMapDocument, autoMappers, where); + aw->setMergeable(whileDrawing); aw->setText(tr("Apply AutoMap rules")); mMapDocument->undoStack()->push(aw); diff --git a/src/tiled/automappingmanager.h b/src/tiled/automappingmanager.h index 77b6400127..c31741d300 100644 --- a/src/tiled/automappingmanager.h +++ b/src/tiled/automappingmanager.h @@ -86,7 +86,7 @@ class AutomappingManager : public QObject void warningsOccurred(bool automatic); private: - void onRegionEdited(const QRegion &where, TileLayer *touchedLayer); + void onRegionEdited(const QRegion &where); void onMapFileNameChanged(); void onFileChanged(const QString &path); @@ -99,10 +99,10 @@ class AutomappingManager : public QObject /** * Applies automapping to the region \a where. * - * If a \a touchedLayer is given, only those AutoMappers will be used which - * have a rule layer matching the \a touchedLayer. + * If \a whileDrawing is true, the changes made through AutoMapping will + * merge with the previous undo operation when possible. */ - void autoMapInternal(const QRegion &where, const TileLayer *touchedLayer); + void autoMapInternal(const QRegion &where, bool whileDrawing); /** * deletes all its data structures