Removed manual reference counting of tilesets

Rely on the QSharedPointer for the lifetime and add/remove tilesets
instances from the TilesetManager directly from the Tileset class.

This fixes issues with tilesets getting loaded twice when they are used
by both a map and a template object, which happened as a result of the
manual adding of references being too late.

In general this approach should also be more reliable and prevent double
loading of tilesets in some cases.

Helps with issue #1895 when the tileset used by the template is also
part of the map.

src/libtiled/objecttemplate.cpp

@@ -30,7 +30,6 @@
 #include "objecttemplate.h"
 
 #include "objecttemplateformat.h"
-#include "tilesetmanager.h"
 
 namespace Tiled {
 
@@ -54,23 +53,23 @@ ObjectTemplate::~ObjectTemplate()
 void ObjectTemplate::setObject(const MapObject *object)
 {
     MapObject *oldObject = mObject;
+    Tileset *tileset = nullptr;
 
     if (object) {
-        if (Tileset *tileset = object->cell().tileset())
-            TilesetManager::instance()->addReference(tileset->sharedPointer());
-
+        tileset = object->cell().tileset();
         mObject = object->clone();
         mObject->markAsTemplateBase();
     } else {
         mObject = nullptr;
     }
 
-    if (oldObject) {
-        if (Tileset *tileset = oldObject->cell().tileset())
-            TilesetManager::instance()->removeReference(tileset->sharedPointer());
-
+    if (oldObject)
         delete oldObject;
-    }
+
+    if (tileset)
+        mTileset = tileset->sharedPointer();
+    else
+        mTileset.reset();
 }
 
 void ObjectTemplate::setFormat(ObjectTemplateFormat *format)

src/libtiled/objecttemplate.h

@@ -30,6 +30,7 @@
 #pragma once
 
 #include "mapobject.h"
+#include "tileset.h"
 
 #include <QPointer>
 
@@ -57,6 +58,7 @@ class TILEDSHARED_EXPORT ObjectTemplate : public Object
     QString mFileName;
     QPointer<ObjectTemplateFormat> mFormat;
     MapObject *mObject;
+    SharedTileset mTileset;
 };
 
 inline const MapObject *ObjectTemplate::object() const

src/libtiled/tileset.cpp

@@ -32,12 +32,22 @@
 #include "terrain.h"
 #include "tile.h"
 #include "tilesetformat.h"
+#include "tilesetmanager.h"
 #include "wangset.h"
 
 #include <QBitmap>
 
 using namespace Tiled;
 
+SharedTileset Tileset::create(const QString &name, int tileWidth, int tileHeight, int tileSpacing, int margin)
+{
+    SharedTileset tileset(new Tileset(name, tileWidth, tileHeight,
+                                      tileSpacing, margin));
+    tileset->mWeakPointer = tileset;
+    TilesetManager::instance()->addTileset(tileset.data());
+    return tileset;
+}
+
 Tileset::Tileset(QString name, int tileWidth, int tileHeight,
                  int tileSpacing, int margin):
     Object(TilesetType),
@@ -62,6 +72,7 @@ Tileset::Tileset(QString name, int tileWidth, int tileHeight,
 
 Tileset::~Tileset()
 {
+    TilesetManager::instance()->removeTileset(this);
     qDeleteAll(mTiles);
     qDeleteAll(mTerrainTypes);
     qDeleteAll(mWangSets);
@@ -151,9 +162,14 @@ void Tileset::setTransparentColor(const QColor &c)
  */
 void Tileset::setImageReference(const ImageReference &reference)
 {
+    const QUrl oldImageSource = mImageReference.source;
+
     mImageReference = reference;
     mExpectedColumnCount = columnCountForWidth(mImageReference.size.width());
     mExpectedRowCount = rowCountForHeight(mImageReference.size.height());
+
+    if (mImageReference.source != oldImageSource)
+        TilesetManager::instance()->tilesetImageSourceChanged(*this, oldImageSource);
 }
 
 /**
@@ -172,8 +188,13 @@ void Tileset::setImageReference(const ImageReference &reference)
  */
 bool Tileset::loadFromImage(const QImage &image, const QUrl &source)
 {
+    const QUrl oldImageSource = mImageReference.source;
+
     mImageReference.source = source;
 
+    if (mImageReference.source != oldImageSource)
+        TilesetManager::instance()->tilesetImageSourceChanged(*this, oldImageSource);
+
     if (image.isNull()) {
         mImageReference.status = LoadingError;
         return false;
@@ -309,7 +330,11 @@ SharedTileset Tileset::findSimilarTileset(const QVector<SharedTileset> &tilesets
  */
 void Tileset::setImageSource(const QUrl &imageSource)
 {
-    mImageReference.source = imageSource;
+    if (mImageReference.source != imageSource) {
+        const QUrl oldImageSource = mImageReference.source;
+        mImageReference.source = imageSource;
+        TilesetManager::instance()->tilesetImageSourceChanged(*this, oldImageSource);
+    }
 }
 
 /**

src/libtiled/tileset.h

@@ -89,13 +89,7 @@ class TILEDSHARED_EXPORT Tileset : public Object
                                 int tileWidth,
                                 int tileHeight,
                                 int tileSpacing = 0,
-                                int margin = 0)
-    {
-        SharedTileset tileset(new Tileset(name, tileWidth, tileHeight,
-                                          tileSpacing, margin));
-        tileset->mWeakPointer = tileset;
-        return tileset;
-    }
+                                int margin = 0);
 
 private:
     /**

src/libtiled/tilesetmanager.cpp

@@ -36,6 +36,8 @@
 
 #include <QImage>
 
+#include "qtcompat_p.h"
+
 namespace Tiled {
 
 TilesetManager *TilesetManager::mInstance;
@@ -63,8 +65,7 @@ TilesetManager::TilesetManager():
 
 TilesetManager::~TilesetManager()
 {
-    // Since all MapDocuments should be deleted first, we assert that there are
-    // no remaining tileset references.
+    // Assert that there are no remaining tileset instances
     Q_ASSERT(mTilesets.isEmpty());
 }
 
@@ -111,13 +112,9 @@ SharedTileset TilesetManager::loadTileset(const QString &fileName, QString *erro
  */
 SharedTileset TilesetManager::findTileset(const QString &fileName) const
 {
-    QMapIterator<SharedTileset, int> it(mTilesets);
-
-    while (it.hasNext()) {
-        const SharedTileset &tileset = it.next().key();
+    for (Tileset *tileset : mTilesets)
         if (tileset->fileName() == fileName)
-            return tileset;
-    }
+            return tileset->sharedPointer();
 
     return SharedTileset();
 }
@@ -126,57 +123,29 @@ SharedTileset TilesetManager::findTileset(const QString &fileName) const
  * Adds a tileset reference. This will make sure the tileset is watched for
  * changes and can be found using findTileset().
  */
-void TilesetManager::addReference(const SharedTileset &tileset)
+void TilesetManager::addTileset(Tileset *tileset)
 {
-    if (mTilesets.contains(tileset)) {
-        mTilesets[tileset]++;
-    } else {
-        mTilesets.insert(tileset, 1);
-        if (tileset->imageSource().isLocalFile())
-            mWatcher->addPath(tileset->imageSource().toLocalFile());
-    }
+    Q_ASSERT(!mTilesets.contains(tileset));
+    mTilesets.append(tileset);
 }
 
 /**
  * Removes a tileset reference. When the last reference has been removed,
  * the tileset is no longer watched for changes.
  */
-void TilesetManager::removeReference(const SharedTileset &tileset)
-{
-    Q_ASSERT(mTilesets.value(tileset) > 0);
-    mTilesets[tileset]--;
-
-    if (mTilesets.value(tileset) == 0) {
-        mTilesets.remove(tileset);
-        if (tileset->imageSource().isLocalFile())
-            mWatcher->removePath(tileset->imageSource().toLocalFile());
-    }
-}
-
-/**
- * Convenience method to add references to multiple tilesets.
- * @see addReference
- */
-void TilesetManager::addReferences(const QVector<SharedTileset> &tilesets)
+void TilesetManager::removeTileset(Tileset *tileset)
 {
-    for (const SharedTileset &tileset : tilesets)
-        addReference(tileset);
-}
+    Q_ASSERT(mTilesets.contains(tileset));
+    mTilesets.removeOne(tileset);
 
-/**
- * Convenience method to remove references from multiple tilesets.
- * @see removeReference
- */
-void TilesetManager::removeReferences(const QVector<SharedTileset> &tilesets)
-{
-    for (const SharedTileset &tileset : tilesets)
-        removeReference(tileset);
+    if (tileset->imageSource().isLocalFile())
+        mWatcher->removePath(tileset->imageSource().toLocalFile());
 }
 
 /**
  * Forces a tileset to reload.
  */
-void TilesetManager::reloadImages(const SharedTileset &tileset)
+void TilesetManager::reloadImages(Tileset *tileset)
 {
     if (!mTilesets.contains(tileset))
         return;
@@ -187,10 +156,10 @@ void TilesetManager::reloadImages(const SharedTileset &tileset)
             if (tile->imageSource().isLocalFile())
                 tile->setImage(QPixmap(tile->imageSource().toLocalFile()));
         }
-        emit tilesetImagesChanged(tileset.data());
+        emit tilesetImagesChanged(tileset);
     } else {
         if (tileset->loadImage())
-            emit tilesetImagesChanged(tileset.data());
+            emit tilesetImagesChanged(tileset);
     }
 }
 
@@ -224,9 +193,7 @@ bool TilesetManager::animateTiles() const
 void TilesetManager::tilesetImageSourceChanged(const Tileset &tileset,
                                                const QUrl &oldImageSource)
 {
-    Q_ASSERT(mTilesets.contains(tileset.sharedPointer()));
-    Q_ASSERT(!oldImageSource.isEmpty());
-    Q_ASSERT(!tileset.imageSource().isEmpty());
+    Q_ASSERT(mTilesets.contains(const_cast<Tileset*>(&tileset)));
 
     if (oldImageSource.isLocalFile())
         mWatcher->removePath(oldImageSource.toLocalFile());
@@ -251,15 +218,11 @@ void TilesetManager::fileChanged(const QString &path)
 
 void TilesetManager::fileChangedTimeout()
 {
-    QMapIterator<SharedTileset, int> it(mTilesets);
-
-    while (it.hasNext()) {
-        const SharedTileset &tileset = it.next().key();
-
+    for (Tileset *tileset : qAsConst(mTilesets)) {
         const QString fileName = tileset->imageSource().toLocalFile();
         if (mChangedFiles.contains(fileName))
             if (tileset->loadFromImage(fileName))
-                emit tilesetImagesChanged(tileset.data());
+                emit tilesetImagesChanged(tileset);
     }
 
     mChangedFiles.clear();
@@ -274,17 +237,14 @@ void TilesetManager::resetTileAnimations()
     // TODO: This could be more optimal by keeping track of the list of
     // actually animated tiles
 
-    QMapIterator<SharedTileset, int> it(mTilesets);
-
-    while (it.hasNext()) {
-        const SharedTileset &tileset = it.next().key();
+    for (Tileset *tileset : qAsConst(mTilesets)) {
         bool imageChanged = false;
 
         for (Tile *tile : tileset->tiles())
             imageChanged |= tile->resetAnimation();
 
         if (imageChanged)
-            emit repaintTileset(tileset.data());
+            emit repaintTileset(tileset);
     }
 }
 
@@ -293,17 +253,14 @@ void TilesetManager::advanceTileAnimations(int ms)
     // TODO: This could be more optimal by keeping track of the list of
     // actually animated tiles
 
-    QMapIterator<SharedTileset, int> it(mTilesets);
-
-    while (it.hasNext()) {
-        const SharedTileset &tileset = it.next().key();
+    for (Tileset *tileset : qAsConst(mTilesets)) {
         bool imageChanged = false;
 
         for (Tile *tile : tileset->tiles())
             imageChanged |= tile->advanceAnimation(ms);
 
         if (imageChanged)
-            emit repaintTileset(tileset.data());
+            emit repaintTileset(tileset);
     }
 }
 

src/libtiled/tilesetmanager.h

@@ -33,7 +33,6 @@
 
 #include <QObject>
 #include <QList>
-#include <QMap>
 #include <QString>
 #include <QSet>
 #include <QTimer>
@@ -59,13 +58,11 @@ class TILEDSHARED_EXPORT TilesetManager : public QObject
     SharedTileset loadTileset(const QString &fileName, QString *error = nullptr);
     SharedTileset findTileset(const QString &fileName) const;
 
-    void addReference(const SharedTileset &tileset);
-    void removeReference(const SharedTileset &tileset);
+    // Only meant to be used by the Tileset class
+    void addTileset(Tileset *tileset);
+    void removeTileset(Tileset *tileset);
 
-    void addReferences(const QVector<SharedTileset> &tilesets);
-    void removeReferences(const QVector<SharedTileset> &tilesets);
-
-    void reloadImages(const SharedTileset &tileset);
+    void reloadImages(Tileset *tileset);
 
     void setReloadTilesetsOnChange(bool enabled);
     bool reloadTilesetsOnChange() const;
@@ -99,14 +96,14 @@ private slots:
     Q_DISABLE_COPY(TilesetManager)
 
     TilesetManager();
-    ~TilesetManager();
+    ~TilesetManager() override;
 
     static TilesetManager *mInstance;
 
     /**
-     * Stores the tilesets and maps them to the number of references.
+     * The list of loaded tilesets (weak references).
      */
-    QMap<SharedTileset, int> mTilesets;
+    QList<Tileset*> mTilesets;
     FileSystemWatcher *mWatcher;
     TileAnimationDriver *mAnimationDriver;
     QSet<QString> mChangedFiles;

src/tiled/addremovetileset.cpp

@@ -22,7 +22,6 @@
 
 #include "map.h"
 #include "mapdocument.h"
-#include "tilesetmanager.h"
 
 using namespace Tiled;
 using namespace Tiled::Internal;
@@ -36,13 +35,10 @@ AddRemoveTileset::AddRemoveTileset(MapDocument *mapDocument,
     , mTileset(tileset)
     , mIndex(index)
 {
-    // Make sure the tileset manager keeps watching this tileset
-    TilesetManager::instance()->addReference(mTileset);
 }
 
 AddRemoveTileset::~AddRemoveTileset()
 {
-    TilesetManager::instance()->removeReference(mTileset);
 }
 
 void AddRemoveTileset::removeTileset()