WangTiles: Better filling methods, wang brush #1649

Bdtrotte · 2017-07-13T22:24:09Z

A work in progress ppull request for the GSOC Wang Tiles project:

In this PR I will commit, first improvements to how wang filling and brushing is done. Next will be a major project to add in a new tool, the wang brush. This tool will function very closely to how the current terrain brush works.

Along with this there are sure to be a host of improvements to the general architecture of wangTiles.

speed up filling a large area.

… of a given wangId based on wildcard slots.

…ndIds

bjorn

First of all, please push the updated testing files (with the wang Ids in hex format)!

Some feedback on the code below.

bjorn · 2017-07-17T12:22:53Z

src/tiled/wangfiller.cpp

@@ -133,7 +133,7 @@ TileLayer *WangFiller::fillRegion(const TileLayer &back,
                    if (lookForward) {
                        for (int i = 0; i < 8; ++i) {
                            QPoint p = currentPoint + adjacentPoints[i];
-                            if (!fillRegion.contains(p))
+                            if (!fillRegion.contains(p) || !tileLayer->cellAt(p - QPoint(tileLayer->x(), tileLayer->y())).isEmpty())


QPoint(tileLayer->x(), tileLayer->y()) is tileLayer->position()

bjorn · 2017-07-17T12:23:18Z

src/tiled/wangfiller.cpp

+                                if (wangTiles.isEmpty())
+                                    fill = true;
+                                else
+                                    fill = false;


That would be:

fill = wangTiles.isEmpty();

bjorn · 2017-07-17T12:29:28Z

src/libtiled/wangset.cpp

+    if (index & 1)
+        return cornerColor(index / 2);
+    else
+        return edgeColor(index / 2);


If you need indexColor, I guess it will make more sense to implement cornerColor and edgeColor using indexColor, rather than the other way around.

bjorn · 2017-07-17T12:48:10Z

src/libtiled/wangset.cpp

@@ -134,6 +187,71 @@ void WangId::flipVertically()
    rotate(2);
 }

+WangIdVariationIterator::WangIdVariationIterator(int edgeColors, int cornerColors, WangId wangId)


This is an interesting approach, but I think it only becomes really useful if it can be used to avoid the allocation of the QVector returned from WangId::variations. For this, WangId::variations would need to return a WangIdVariations instance which provides begin and end members returning this iterator. In addition, the iterator needs to be changed such that it is not valid to dereference the one returned by end. Then, you can write this and it will not allocate the actual list of variations:

for (WangId id : wangId.variations(mEdgeColors, mCornerColors))

As currently implemented I rather fail to see the point of using an iterator.

That said, I still worry that there is a performance issue here when the color count goes above about 4, even when we avoid the allocation of the vector.

The point of it is to make "WildWangIdUsed" a lot faster. That function is pretty important, so Im still working on speeding it up more.

In regards to the iterator itself, I know it isn't proper with end being valid, but the problem is in the 15 x 15 case, all unsigned ints are valid, so I'm not sure there is a way to make it invalid to dereference. So I'm not sure I can make it proper? Though I see your point it would be nice to avoid the allocation... Though currently, the variations function is unused (it is in the findMatchingWangTiles function, but that can be changed to use the iterator as is), so It could be removed all together. Ill work on a more elegant solution though,

So the solution I've put together implements a proper iterator to replace to old messy one. It works for the end case by using the biggest wangId for given input + 1 to the underlying id. This works great for all sets, other than those 15x15, where this value will be zero, and immediately fail. As a work around, everywhere this iterator is used must be wrapped with an if testing if it is the 15x15 case, and if so takes on slightly different behavior.

Of course if this is actually run, that would mean 15^8 loops, which may not be ideal...

In that case, it will be better for it to fail immediately, so maybe you don't need that if...

bjorn · 2017-07-17T12:52:32Z

src/libtiled/wangset.cpp

+
+unsigned WangSet::completeSetSize() const
+{
+    return qPow(mEdgeColors, 4) * qPow(mCornerColors, 4);


I guess this would be faster when writing like the little more verbose:

return mEdgeColors * mEdgeColors * mEdgeColors * mEdgeColors * mCornerColors * mCornerColors * mCornerColors * mCornerColors;

I think it makes little sense to return unsigned rather than int, btw. We can't go out of int range here anyway due to limitations on the color counts, right? So all it does it add a need for casting int to unsigned elsewhere.

We can't go out of int range here anyway due to limitations on the color counts, right?

I think we can. Max color is 15, so max return is 15^8 = 2562890625 > 0x7fffffff

To avoid casting, I the new function for unique keys will return unsigned as well.

bjorn · 2017-07-17T13:12:04Z

src/libtiled/wangset.cpp

+{
+    if (isEmpty())
+        return false;
+    return (unsigned) mWangIdToWangTile.uniqueKeys().size() == completeSetSize();


If this check happens often (looks like it will, because it is passed to findFittingCell), it would make sense to cache this information and calculate it when wang Ids are changing. Also, the mWangIdToWangTile.uniqueKeys().size() would be faster when implemented like:

template <class Key, class T> static int uniqueKeyCount(const QHash<Key, T> &hash) const { int count = 0; auto i = hash.begin(); auto i_end = hash.end(); if (i != i_end) { for (;;) { ++count; const Key &key = i.key(); do { if (++i == i_end) return count; } while (key == i.key()); } } return count; } bool WangSet::isComplete() const { if (mWangIdToWangTile.size() < completeSetSize()) return false; // can't possibly be complete return uniqueKeyCount(mWangIdToWangTile) == completeSetSize()) }

That is to avoid the allocation of the list and in most cases also the whole iteration (the uniqueKeyCount implementation is based on QHash::uniqueKeys, but I didn't compile-check it).

I also realized since it should be possible to assign a wangId with 0s (wildcards) this has to check for the count of unique keys with no wild cards. So I'll write a specific function for this in WangSet based on your suggestion and that.

bjorn · 2017-07-17T13:36:50Z

src/tiled/wangfiller.cpp

+    for (int i = 0; i < wangIds.size(); ++i) {
+        QPoint point(i % tileLayer->width() + tileLayer->x(),
+                     i / tileLayer->width() + tileLayer->y());
+        if (fillRegion.contains(point))


Since you only care for points within the fill region, you should really iterate over fillRegion.rects(). Otherwise all these contains checks could be quite a performance hit.

While iterating just the rectangles you could even go one step further, since really you only care for points at the borders of these rectangles. You don't need to iterate the possibly large area in the middle because those wangIds will always be 0.

bjorn · 2017-07-17T13:38:34Z

src/tiled/wangfiller.cpp

+                                        boundingRect.x(),
+                                        boundingRect.y(),
+                                        boundingRect.width(),
+                                        boundingRect.height());


Indentation is off by one.

bjorn · 2017-07-17T13:44:22Z

src/tiled/wangfiller.cpp

+                int currentIndex = (currentPoint.y() - tileLayer->y()) * tileLayer->width() + (currentPoint.x() - tileLayer->x());
+                QList<WangTile> wangTiles = mWangSet->findMatchingWangTiles(wangIds[currentIndex]);
+
+                while(!wangTiles.isEmpty()) {


Coding style: missing space after while.

bjorn · 2017-07-18T08:08:02Z

src/tiled/wangdock.cpp

+    if (mWangTemplateView->isVisible())
+        setColorView(true);
+    else
+        setTemplateView(true);


This way of switching appears to behave a little strange (with the height changing upon switching). I would suggest to use a QStackedWidget (or QStackedLayout if possible) to switch between the two views.

bjorn · 2017-07-21T19:26:04Z

I think we'll need to sort by tile ID when saving the wang tiles, because their order is essentially randomized every time the file is saved currently due to using a QHash. I think using a QHash is still better for performance than a QMap, but upon saving we'll need to get the keys, sort them and then use that to write out the file.

Bdtrotte · 2017-07-21T19:40:50Z

At this point this PR should be ready for final review and merge! Also completes the major mile stones of Wang Tiles in whole. Next to come will be a host of features and improvements for wang tiles to polish off the feature and get it totally ready for merge into master.

bjorn

Two small comments. Unfortunately, I no longer had time for a complete review and in testing I could not figure out how to make the Wang Brush do something. I selected a color but when hovering or clicking the map nothing appeared to happen, apart from a paint command being added to the undo history.

I'll try to get around to more reviewing as soon as possible.

bjorn · 2017-07-25T15:33:38Z

src/tiled/wangbrush.cpp

+        tilePos.setY(floor(tilePosF.y()));
+
+        QPointF tileLocalPoint(modf(tilePosF.x(), nullptr),
+                              modf(tilePosF.y(), nullptr));


This is crashing for me. Reading the documentation for modf, I don't see anything about it being valid to pass in a null pointer.

Interesting... I've had no problems on mine... I'll pass a local int to be safe.

And fix the spacing!

bjorn · 2017-07-25T15:34:07Z

src/tiled/wangbrush.cpp

+            switch (mEdgeDir) {
+            case 0:
+                if (tileLocalPoint.y() < EDGE_DEAD_ZONE)
+                    return;


My compiler is warning about the fall-through here. Is it intensional?

It was not, forgot my breaks

…ments with wangBrush

bjorn

Nice work! It really starts to take up shape with the addition of the wang brush!

I've made a lot of small to medium inline comments. In addition, one thing I missed was the ability to capture the current color from the map using right mouse button, similar to how it works for the stamp and terrain tools.

But I think after you've processed the feedback this can be merged and we can move on to some polishing and other things on your list (do you have it written down somewhere? I don't remember everything you listed on IRC) in preparation of merging to master early next week (I'd say the target for this is 7th or 9th of August). After that we'd have three weeks for remaining tasks as well as hearing feedback from early users.

bjorn · 2017-07-30T09:20:53Z

src/libtiled/wangset.cpp

+    if (wangId
+            && !mWangIdToWangTile.contains(wangId)
+            && ((mEdgeColors > 1 && !wangId.hasEdgeWildCards())
+                || (mCornerColors > 1 && !wangId.hasCornerWildCards())))


This condition still needs the same correction as the one in addWangTile for the case where we have both edges and corners.

bjorn · 2017-07-30T09:32:00Z

src/libtiled/wangset.cpp

-        }
-    }
+    for (WangId id : wangId.variations(mEdgeColors, mCornerColors))
+        list.append(mWangIdToWangTile.values(id));


While this is nice and short, it is not the most efficient since it allocates a QList on the heap every iteration. See the documentation of QMultiHash for an example showing a more efficient approach using find and iterating while checking the key.

bjorn · 2017-07-30T09:50:11Z

src/tiled/bucketfilltool.cpp

-                    mRandomCellPicker.add(cell, tile->probability());
+    if (mIsWangFill && mWangFiller->wangSet()) {
+        const SharedTileset &tileset = mWangFiller->wangSet()->tileset()->sharedPointer();
+        if (!mMissingTilesets.contains(tileset)


This contains check is superfluous, because mMissingTilesets was just cleared.

bjorn · 2017-07-30T10:01:10Z

src/tiled/mapdocument.cpp

@@ -605,6 +605,9 @@ void MapDocument::toggleOtherLayers(Layer *layer)
 */
 void MapDocument::insertTileset(int index, const SharedTileset &tileset)
 {
+    if (mMap->tilesets().contains(tileset))
+        return;


Like I said on IRC, this isn't the right fix for that problem. If insertTileset is silently doing nothing, then AddTileset::undo will do the wrong thing. So this is just moving the problem to the undo action. The proper fix is to not even try adding tilesets to the map that are already there (no AddTileset command should be constructed).

bjorn · 2017-07-30T10:03:44Z

src/tiled/stampbrush.cpp

+    mMissingTilesets.clear();
+
+    const SharedTileset &tileset = wangSet->tileset()->sharedPointer();
+    if (!mMissingTilesets.contains(tileset)


contains check superfluous here as well.

bjorn · 2017-07-30T13:16:40Z

src/tiled/wangfiller.cpp

+
+                    if (fill) {
+                        tileLayer->setCell(currentPoint.x() - tileLayer->x(),
+                                          currentPoint.y() - tileLayer->y(),


Indentation off by one.

bjorn · 2017-07-30T13:20:29Z

src/tiled/wangfiller.cpp

+        if (!fillRegion.contains(adjacentPoint) && back.contains(adjacentPoint))
+            surroundingCells[i] = back.cellAt(adjacentPoint);
+        else
+            surroundingCells[i] = Cell();


This assignment shouldn't be necessary.

bjorn · 2017-07-30T13:23:31Z

tests/wangtiles/grassAndWater.tsx

@@ -0,0 +1,32 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<tileset name="GrassAndWater" tilewidth="64" tileheight="64" tilecount="24" columns="4">
+ <image source="C:/Users/topha/Desktop/grass_and_water.png" width="256" height="384"/>


Please make sure there is a relative reference here.

bjorn · 2017-07-30T13:35:19Z

src/tiled/stampbrush.cpp

+
+    mMissingTilesets.clear();
+
+    const SharedTileset &tileset = wangSet->tileset()->sharedPointer();


This is crashing when the wangSet is null (happens to me when I create a new map, for example).

bjorn · 2017-07-30T13:41:37Z

src/tiled/wangfiller.h

+     * */
+    TileLayer *fillRegion(const TileLayer &back,
+                    const QRegion &fillRegion,
+                    bool lookForward = true) const;


Indentation is off.

Bdtrotte added 4 commits July 13, 2017 15:18

Merged in new work

a1825c0

Some general improvements. Added fillRegion method to WangFiller to

aaeba35

speed up filling a large area.

Added WangIdVariations iterator to more quickly go through variations…

e59ed67

… of a given wangId based on wildcard slots.

Added WangColorModel in preporation for WangBrush, and painting on wa…

9df29b0

…ndIds

bjorn requested changes Jul 17, 2017

View reviewed changes

Bdtrotte added 3 commits July 17, 2017 07:36

Added updated test files

7e5b0b3

Made requested changes

431d408

Added button to switch between template and color view

f08140d

bjorn reviewed Jul 18, 2017

View reviewed changes

Bdtrotte added 4 commits July 18, 2017 09:53

Color and template now linked. Other requested changes

0a780e2

Wang Brush wokring!

3c1703f

Can now paint on individual edges/corners when assigning

be44afa

Some fixes, and added wangbrush.cpp/h to .pro

c8afa67

Added icon for wangBrush

f16514b

Bdtrotte added 2 commits July 21, 2017 13:30

included cmath

35be0d7

Using QVectorIterator

1422355

bjorn requested changes Jul 25, 2017

View reviewed changes

Wang methods with stamp and fill tool add tileset to map. Some improv…

95ac080

…ments with wangBrush

bjorn requested changes Jul 30, 2017

View reviewed changes

Made requested changes

f4776b9

bjorn merged commit f4776b9 into mapeditor:wip/wangtiles Jul 31, 2017

Bdtrotte deleted the wip/wangtiles branch July 31, 2017 22:51

Bdtrotte restored the wip/wangtiles branch July 31, 2017 23:03

Bdtrotte deleted the wip/wangtiles branch July 31, 2017 23:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WangTiles: Better filling methods, wang brush #1649

WangTiles: Better filling methods, wang brush #1649

Bdtrotte commented Jul 13, 2017

bjorn left a comment

bjorn Jul 17, 2017

bjorn Jul 17, 2017

bjorn Jul 17, 2017

bjorn Jul 17, 2017

Bdtrotte Jul 17, 2017

Bdtrotte Jul 17, 2017 •

edited

Loading

bjorn Jul 17, 2017

bjorn Jul 17, 2017

Bdtrotte Jul 17, 2017

bjorn Jul 17, 2017

Bdtrotte Jul 17, 2017

bjorn Jul 17, 2017

bjorn Jul 17, 2017

bjorn Jul 17, 2017

bjorn Jul 18, 2017

bjorn commented Jul 21, 2017

Bdtrotte commented Jul 21, 2017

bjorn left a comment

bjorn Jul 25, 2017

Bdtrotte Jul 25, 2017 •

edited

Loading

bjorn Jul 25, 2017

Bdtrotte Jul 25, 2017

bjorn left a comment

bjorn Jul 30, 2017

bjorn Jul 30, 2017

bjorn Jul 30, 2017

bjorn Jul 30, 2017

bjorn Jul 30, 2017

bjorn Jul 30, 2017

bjorn Jul 30, 2017

bjorn Jul 30, 2017

bjorn Jul 30, 2017

bjorn Jul 30, 2017


		mMissingTilesets.clear();

		const SharedTileset &tileset = wangSet->tileset()->sharedPointer();

WangTiles: Better filling methods, wang brush #1649

WangTiles: Better filling methods, wang brush #1649

Conversation

Bdtrotte commented Jul 13, 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

Bdtrotte Jul 17, 2017 • edited Loading

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

bjorn commented Jul 21, 2017

Bdtrotte commented Jul 21, 2017

bjorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bdtrotte Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Bdtrotte Jul 17, 2017 •

edited

Loading

Bdtrotte Jul 25, 2017 •

edited

Loading