Enabling wangfilling for BucketFillTool and StampBrush #1638

Bdtrotte · 2017-07-04T19:50:00Z

and hopefully implementing a new dock/view/model for assigning tiles to a wang set

… bucket and stamp brush (only works with bucket at the moment)

… set up though

Bdtrotte · 2017-07-09T23:06:55Z

The latest changes assert a new important rule for wangIds. If the edge count of a wangSet == 1, then for a wangId to be valid, all the edges must == 0. The same applies for corners. This is because for the purposes of finding tiles with wangIds, it is much faster if half the Id is already known; without the standardization all possibilities must be checked. Furthermore, when moving from a set with no edges, to one with them, it makes sense that the previously non-existent edges be thought of as wildcards.

Bdtrotte · 2017-07-09T23:08:34Z

With painting working, and a whole lot more refined, I feel this PR is one step away from being merged (not including reviews). All thats left is to alter things to support undo commands, and cleaning up some quarks with how the UI behaves.

Bdtrotte · 2017-07-10T04:29:06Z

Undo now works when painting wangIds onto tiles. There is still the case where one removes wangIds from tiles by changing the edge/corner counts of the wangSet. Currently there is no way to retrieve this once done, in the next push this will be fixed.

Also, some more test files have been added with a real tileset to show off some of the fun!

…e restored on undo. Also can flip the wangAssignment brush

bjorn

This is some great work! It took me about 4 hours to go through it all. I made quite some comments, but I think they are mostly minor.

I could probably merge in the evening when you have time to address the comments by then, and we could talk about the next steps.

bjorn · 2017-07-11T07:04:10Z

src/libtiled/mapreader.cpp

@@ -591,6 +591,12 @@ void MapReaderPrivate::readTilesetWangSets(Tileset &tileset)
                    const QXmlStreamAttributes tileAtts = xml.attributes();
                    int tileId = tileAtts.value(QLatin1String("tileid")).toInt();
                    unsigned wangId = tileAtts.value(QLatin1String("wangid")).toUInt();
+
+                    if (!wangSet->wangIdIsValid(wangId)) {
+                        xml.skipCurrentElement();


I think this is probably a case where it would be better to raise an error using xml.raiseError(...) and return, because this means the file is invalid and it is not nice to silently ignore that.

bjorn · 2017-07-11T07:13:52Z

src/libtiled/wangset.cpp

+    int previousEdgeColors = mEdgeColors;
+    mEdgeColors = newEdgeColors;
+
+    for (WangId wangId : mWangIdToWangTile.keys()) {


This allocates a temporary list of all WangIds. You could instead just iterate the other hash, right?

for (WangId wangId : mTileInfoToWangId) {

Also applies to tilesChangedOnSetCornerColors.

bjorn · 2017-07-11T07:27:19Z

src/libtiled/wangset.cpp

+void WangSet::removeWangTile(const WangTile &wangTile)
+{
+    WangId wangId = mTileInfoToWangId.value(wangTileToTileInfo(wangTile), 0);
+    mTileInfoToWangId.remove(wangTileToTileInfo(wangTile));


Can be simplified to:

WangId wangId = mTileInfoToWangId.take(wangTileToTileInfo(wangTile));

bjorn · 2017-07-11T07:43:43Z

src/libtiled/wangset.cpp

+
+        if (Tile *tile = mTileset->tileAt(tileInfo))
+            tiles.append(tile);
+    }


Wouldn't this do the same thing?

for (const WangTile &wangTile : mWangIdToWangTile) tiles.append(wangTile.tile());

With both methods there can be duplicates, but I guess that is not really a problem.

bjorn · 2017-07-11T07:48:30Z

src/libtiled/wangset.cpp

+    if (mCornerColors > 1)
+        mask |= 0xf0f0f0f0;
+
+    for (WangId usedId : mWangIdToWangTile.keys()) {


To avoid allocating a temporary list, I think this should be:

for (WangId usedId : mTileInfoToWangId) {

However, I do not fully understand why this function isn't simply doing:

return mWangIdToWangTile.contains(wangId);

Why the need for the mask, and could this be avoided?

That is originally what I was doing, but ran into some problems when there was less certainty about the wangId (I was searching for something with at least all ones in a corner only set, but the wangSet had the corners as wildcards (0)). The mask just allows that, though it should now never happen, the spaces in the wangId which don't matter can be different. Though now that I have made more clear rules about wangIds, this could be changed.

bjorn · 2017-07-11T12:04:26Z

src/tiled/wangtemplatemodel.cpp

+    int cornerEdgePermutations = edges * corners;
+
+    for (int i = 0; i < 8; ++i) {
+        int bellowPermutations = qPow(cornerEdgePermutations, i/2) * ((i&1)? edges : 1);


Typo: "below"

bjorn · 2017-07-11T12:07:10Z

src/tiled/wangtemplatemodel.cpp

+{
+    mWangSet = wangSet;
+
+    resetModel();


While sometimes it may be unavoidable, please implement this properly when possible:

beginResetModel(); mWangSet = wangSet; endResetModel();

bjorn · 2017-07-11T12:09:20Z

src/tiled/wangtemplatemodel.h

+
+    int rowCount(const QModelIndex &parent = QModelIndex()) const override;
+
+    int columnCount(const QModelIndex &parent = QModelIndex()) const override;


Since you derive from QAbstractListModel, you can remove this override because the list model already returns a column count of 1 by default.

bjorn · 2017-07-11T12:11:25Z

src/tiled/wangtemplateview.cpp

+    setSelectionMode(QAbstractItemView::SingleSelection);
+    setVerticalScrollMode(QAbstractItemView::ScrollPerPixel);
+    setItemDelegate(new WangTemplateDelegate(this, this));
+    setTabKeyNavigation(true);


Please add:

setUniformItemSizes(true);

I didn't notice an actual performance difference, but the view may be able to avoid getting the size hint very often.

You can remove the setTabKeyNavigation(true);. That is the default, but actually it won't work anyway because Tab is used to show/hide the all menu bars and dock widgets. Alternatively it could be set to false explicitly.

bjorn · 2017-07-11T12:22:29Z

src/tiled/wangtemplateview.cpp

+void WangTemplateView::setModel(QAbstractItemModel *model)
+{
+    QListView::setModel(model);
+    updateBackgroundColor();


Since updateBackgroundColor does not change color based on the model, this override can be removed. It could be called from the constructor and you should connect it to StyleHelper::styleApplied, as done in TilesetView.

Bdtrotte · 2017-07-11T19:10:02Z

There is one more change I would like to put on this pr, which is to send a signal from the TilesetView to the wangDock when it changes the current WangId used, and when it changes the used status of a certain wangId, so that the template view updates accordingly. After that it will be complete, and ready for merge.

Bdtrotte added 9 commits July 4, 2017 12:45

Wangdock added to madEditior, Wang icon added, Wangfill introduced to…

b02bb8a

… bucket and stamp brush (only works with bucket at the moment)

Got rid of that pesky file

06beda0

added test files

d163771

Stamp brush now supports wang filling

a7d8052

Added methods for setting wangId edges and corners

9148508

Added view to wangDock for displaying the wang template of a given set

a793454

improvments to wangset class, can now zoom in wangTemplateView

73b6be4

TilesetView now displays an overlay for each tile with a wangId

6083d17

Can now paint wangIds onto tilesets, and erase. Undo commands not yet…

6ea72f6

… set up though

Bdtrotte added 2 commits July 9, 2017 18:12

Changing the edge or corner count now updates the wang overlay

2695458

Now support undo commands when painting wangIds, added some test files

52d0354

Bdtrotte added 2 commits July 10, 2017 10:47

When changing edge/corner count and tiles are unassigned, they will b…

14bac44

…e restored on undo. Also can flip the wangAssignment brush

Fixed some small issues

3136c28

bjorn requested changes Jul 11, 2017

View reviewed changes

Made requested changes

7359064

Bdtrotte added 2 commits July 11, 2017 13:30

Improved communication between tilesetView and wangDock

fc593b8

made clang++ happier

213eb27

bjorn merged commit 09dc14d into mapeditor:wip/wangtiles Jul 13, 2017

Bdtrotte deleted the wip/wangtiles branch July 13, 2017 21:44

bjorn mentioned this pull request Apr 14, 2021

Terrains UI issues #3015

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabling wangfilling for BucketFillTool and StampBrush #1638

Enabling wangfilling for BucketFillTool and StampBrush #1638

Bdtrotte commented Jul 4, 2017

Bdtrotte commented Jul 9, 2017

Bdtrotte commented Jul 9, 2017

Bdtrotte commented Jul 10, 2017

bjorn left a comment

bjorn Jul 11, 2017

bjorn Jul 11, 2017

bjorn Jul 11, 2017

bjorn Jul 11, 2017

bjorn Jul 11, 2017

Bdtrotte Jul 11, 2017

bjorn Jul 11, 2017

bjorn Jul 11, 2017

bjorn Jul 11, 2017

bjorn Jul 11, 2017

bjorn Jul 11, 2017

Bdtrotte commented Jul 11, 2017


		int rowCount(const QModelIndex &parent = QModelIndex()) const override;

		int columnCount(const QModelIndex &parent = QModelIndex()) const override;

Enabling wangfilling for BucketFillTool and StampBrush #1638

Enabling wangfilling for BucketFillTool and StampBrush #1638

Conversation

Bdtrotte commented Jul 4, 2017

Bdtrotte commented Jul 9, 2017

Bdtrotte commented Jul 9, 2017

Bdtrotte commented Jul 10, 2017

bjorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bdtrotte commented Jul 11, 2017