WangTiles: WangColor now have properties including name, color, image, and probability... #1670

Bdtrotte · 2017-07-31T23:27:52Z

Also to be in this pr:

Right click to choose a wangcolor from the map
Add/remove buttons for wangColors
Any other fixes/features before 1.1

Bdtrotte · 2017-08-05T22:24:57Z

For assignment purposes in regards to staggered maps, one can think of the assignment being rotated 45 degrees clock wise. At some point this should be reflected graphically. The wang brush now supports staggered maps, (other tools to come shortly), except when working with edges. Because edges are very dependent on which edge they are closest to, the mouse moved function must be changed to take staggered into account. This is a fairly big effort at the moment, and will be saved for later.

bjorn · 2017-08-06T10:34:21Z

For assignment purposes in regards to staggered maps, one can think of the assignment being rotated 45 degrees clock wise. At some point this should be reflected graphically.

Did you notice that the overlay used for terrain assignment is already able to handle isometric tilesets? Check out the usages of tilesetGridTransform.

Bdtrotte · 2017-08-06T14:00:01Z

Did you notice that the overlay used for terrain assignment is already able to handle isometric tilesets? Check out the usages of tilesetGridTransform.

I did not! Transforms will be very useful in supporting edges as well, thanks for pointing that out

Bdtrotte · 2017-08-07T02:07:13Z

I have not tested the results of the change of the last commit at length. Map rendering and such seems fine. The terrain brush may behave a little out of sorts on staggered maps, though should likely be updated to properly follow the corner the mouse is closest to (which I'm pretty sure it did not do prior in staggered maps.) In fact this could probably be done in the AbstractTileTool for BetweenTiles mode.

bjorn

Can you do a rebase and squash the "removed stray file" commit into its parent? That way, we don't have the adding and removing of this file in the history. You may also want to add some .gitignore rule to avoid adding these files in the future.

bjorn · 2017-08-10T07:17:01Z

src/libtiled/staggeredrenderer.cpp


-    return referencePoint;
+    QPointF newRel = HexagonalRenderer::tileToScreenCoords(referencePoint.x(), referencePoint.y());


Should not be necessary to qualify this with HexagonalRenderer::.

bjorn · 2017-08-10T07:23:19Z

src/libtiled/staggeredrenderer.cpp

+    newRel = QPointF(x - newRel.x(), y - newRel.y());
+    QPointF tileLocal = newRel - QPointF(p.tileWidth / 2, 0);
+
+    //Undose the adjustment done above.


I guess it would be nicer to not modify x and y and copy them into alignedX and alignedY for example before modifying them.

bjorn

Unfortunately not a full review, I think I got about halfway through.

bjorn · 2017-08-10T13:49:02Z

src/libtiled/mapreader.cpp

+                    int index = wangColorAtts.value(QLatin1String("index")).toInt();
+                    int r = wangColorAtts.value(QLatin1String("r")).toInt();
+                    int g = wangColorAtts.value(QLatin1String("g")).toInt();
+                    int b = wangColorAtts.value(QLatin1String("b")).toInt();


Please store color the same way as it is done for other color attributes, so in hex format like "#RRGGBB".

bjorn · 2017-08-10T13:50:06Z

src/libtiled/mapreader.cpp

+                    int r = wangColorAtts.value(QLatin1String("r")).toInt();
+                    int g = wangColorAtts.value(QLatin1String("g")).toInt();
+                    int b = wangColorAtts.value(QLatin1String("b")).toInt();
+                    int imageId = wangColorAtts.value(QLatin1String("imageTile")).toInt();


Please keep the names for elements and attributes all lower-case, so that would be imagetile. However, in this case, I would match the name with that used on the terrain element, and simply call this the tile.

bjorn · 2017-08-10T13:54:26Z

src/libtiled/wangset.h

+    void removeWangColorAt(int color, bool isEdge);
+
+    WangColor *wangColorOfEdge(int index) const;
+    WangColor *wangColorOfCorner(int index) const;


Could these be just called edgeColorAt and cornerColorAt?

bjorn · 2017-08-10T13:59:05Z

src/libtiled/wangset.cpp

+
+    if (n == 1) {
+        mEdgeColors = 1;
+        mEdges.clear();


This will not delete the WangColor instances (nor will the below call to removeLast). Who owns the instances?

The wangSet does, bit of an oversight on my part!

Ill use QSharedPointer I think

bjorn · 2017-08-10T14:02:06Z

src/libtiled/wangset.cpp

+
+    colors.insert(wangColor->colorIndex(), wangColor);
+
+    for (int i = wangColor->colorIndex() + 1; i <= (wangColor->isEdge()? mEdgeColors : mCornerColors); ++i)


If for almost every other line you need to check if the color is edge or corner, please split it up into two functions. Same goes for removeWangColorAt.

bjorn · 2017-08-10T14:20:10Z

src/tiled/tiled.qrc

@@ -52,6 +52,8 @@
        <file>images/16x16/selection-subtract.png</file>
        <file>images/16x16/selection-intersect.png</file>
        <file>images/22x22/add.png</file>
+		<file>images/22x22/add-corner.png</file>
+		<file>images/22x22/add-edge.png</file>


Please avoid using tabs here.

bjorn · 2017-08-10T14:24:17Z

src/tiled/tilesetwangsetmodel.cpp

+
+    if ((isEdge? wangSet->edgeColors() : wangSet->cornerColors()) == 2) {
+        if (isEdge)
+            wangSet->setEdgeColors(1);


Please try to get rid of this special logic, or explain me why it's a good idea. :-)

bjorn · 2017-08-10T14:52:48Z

src/tiled/wangbrush.cpp

+
+        QString extraInfo;
+        if (!static_cast<WangBrushItem*>(brushItem())->isValid())
+            extraInfo = QLatin1String(" (Missing wang tile transition)");


This should be a translatable string (except for the leading space, which shouldn't be part of the translatable string).

bjorn · 2017-08-10T14:56:42Z

src/tiled/wangbrush.cpp

+        int newColor = 0;
+
+        if (mBrushMode == PaintVertex)
+            newColor = wangId.cornerColor(3);


To avoid hardcoded numbers like 3 I think we should probably have an enumeration going over the colors and edges, so that we can write something like wangId.cornerColor(WangId::TopLeft).

bjorn · 2017-08-10T15:38:41Z

src/tiled/changewangcolordata.h

+class ChangeWangColorProbability : public QUndoCommand
+{
+public:
+        ChangeWangColorProbability(float newProbability,


Indentation is off by one level here.

bjorn · 2017-08-10T16:09:14Z

src/tiled/wangfiller.cpp

@@ -84,20 +131,25 @@ Cell WangFiller::findFittingCell(const TileLayer &back,
                    break;
                }
            }
+            delete adjacentPoints;


Since getSurroundingPoints uses malloc, this should be free.

Or, preferably, use a static array in getSurroundingPoints.

… WangBrush edges and vertex mode works on staggered maps as expected

…ileset

bjorn

I think it's shaping up nicely, but I had some comments on the code that I'd like to be addressed before merging.

bjorn · 2017-08-14T09:53:17Z

src/tiled/tiled.qrc

-		<file>images/22x22/add-corner.png</file>
-		<file>images/22x22/add-edge.png</file>
+	    <file>images/22x22/add-corner.png</file>
+	    <file>images/22x22/add-edge.png</file>


You only changed one tab to 4 spaces, but there is still a tab remaining.

bjorn · 2017-08-15T13:17:31Z

src/libtiled/mapreader.cpp

+                           || xml.name() == QLatin1String("wangcornercolor")) {
+                    const QXmlStreamAttributes wangColorAtts = xml.attributes();
+                    QString name = wangColorAtts.value(QLatin1String("name")).toString();
+                    int index = wangColorAtts.value(QLatin1String("index")).toInt();


I think we shouldn't store the index, since it's superfluous. We could even drop the edges amd corners attributes from wangset now, and just append the edge and corner colors we find inside.

bjorn · 2017-08-15T13:24:07Z

src/libtiled/wangset.h

-    int mEdgeColors;
-    int mCornerColors;
+    QList<QSharedPointer<WangColor>> mEdgeColors;
+    QList<QSharedPointer<WangColor>> mCornerColors;


Please use QVector, since a QSharedPointer does not fit in a pointer.

bjorn · 2017-08-15T13:24:54Z

src/tiled/abstracttiletool.h

@@ -45,7 +45,8 @@ class AbstractTileTool : public AbstractTool
    AbstractTileTool(const QString &name,
                     const QIcon &icon,
                     const QKeySequence &shortcut,
-                     QObject *parent = nullptr);
+                     QObject *parent = nullptr,
+                     BrushItem *brushItem = nullptr);


Please keep the parent as the last parameter.

bjorn · 2017-08-15T13:36:44Z

src/libtiled/mapreader.cpp

+                        wc = wangSet->cornerColorAt(index).data();
+
+                    wc->setName(name);
+                    wc->setColor(color);


Please either avoid setting invalid colors, or update the example files to use color attributes. :-)

bjorn · 2017-08-15T14:07:03Z

src/tiled/wangdock.cpp

-    if (!mTemplateAndColorView->isVisible()) {
-        mSwitchTemplateViewButton->setEnabled(true);
-        mTemplateAndColorView->setVisible(true);
+    if (!mTemplateAndColorWidget->isVisible()) {


Can also be removed, right?

bjorn · 2017-08-15T14:07:56Z

src/tiled/wangdock.cpp


    retranslateUi();
 }

 void WangDock::hideTemplateColorView()
 {
-    if (!mTemplateAndColorView->isVisible())
+    if (!mTemplateAndColorWidget->isVisible())


Should also be superfluous now.

bjorn · 2017-08-15T14:12:48Z

src/tiled/wangfiller.cpp

@@ -47,26 +50,68 @@ void WangFiller::setWangSet(WangSet *wangSet)
    mWangSet = wangSet;
 }

+static QPoint *getSurroundingPoints(QPoint point, StaggeredRenderer *staggeredRenderer, Map::StaggerAxis staggerAxis)
+{
+    QPoint *adjacentPoints = (QPoint*)malloc(sizeof(QPoint) * 8);


There's still the problem with malloc / delete mismatch. Actually, I suggested using a static array but I think it's not going to work because the function is called multiple times. But you could allocate these arrays on the stack and pass them into the function:

static void getSurroundingPoints(QPoint point, StaggeredRenderer *staggeredRenderer, Map::StaggerAxis staggerAxis, QPoint *points) QPoint adjacentPoints[8]; getSurroundingPoints(point, staggeredRenderer, staggerAxis, adjacentPoints);

bjorn · 2017-08-15T14:21:21Z

src/tiled/wangfiller.h

-                         QPoint point) const;
+                         QPoint point,
+                         StaggeredRenderer *staggeredRenderer = nullptr,
+                         Map::StaggerAxis staggerAxis = Map::StaggerX) const;


Instead of adding these parameters to each WangFiller method, passing them around on each call, consider adding them to the WangFiller constructor. And to avoid having to keep a WangFiller instance up-to-date, just allocate it on the stack when you're going to call its methods. This is similar to how TilePainter is used.

bjorn · 2017-08-15T14:31:35Z

tests/staggeredrenderer/test_staggeredrenderer.cpp

@@ -89,11 +89,11 @@ void test_StaggeredRenderer::screenToTileCoords_data()
    QTest::addColumn<QPointF>("screenCoords");
    QTest::addColumn<QPointF>("tileCoords");

-    QTest::newRow("10,16") << QPointF(10, 16) << QPointF(0, 0);
-    QTest::newRow("5,5")   << QPointF(5, 5)   << QPointF(-1, -1);
+    QTest::newRow("10,16") << QPointF(10, 16) << QPointF(0, 1);


I think these values are very hard to understand, since they rely on rounding the internal fraction in a coordinate space where rounding makes no sense at all. Please keep these values the same, and change the test_StaggeredRenderer::screenToTileCoords to truncate instead of round.

bjorn

Some minor comments.

bjorn · 2017-08-15T20:46:43Z

src/libtiled/wangset.cpp

 }

 int WangSet::edgeColorCount() const
 {
    int size = mEdgeColors.size();

    if (size > 1)
-        return size - 1;
+        return size;


Let's shorten this to:

return qMax(1, mEdgeColors.size());

bjorn · 2017-08-15T20:46:51Z

src/libtiled/mapreader.cpp

@@ -614,22 +612,18 @@ void MapReaderPrivate::readTilesetWangSets(Tileset &tileset)
                } else if (xml.name() == QLatin1String("wangedgecolor")
                           || xml.name() == QLatin1String("wangcornercolor")) {
                    const QXmlStreamAttributes wangColorAtts = xml.attributes();
-                    QString name = wangColorAtts.value(QLatin1String("name")).toString();
-                    int index = wangColorAtts.value(QLatin1String("index")).toInt();
+                    QString name = wangColorAtts.value(QLatin1String("name")).toString();\


bjorn · 2017-08-15T20:48:36Z

src/tiled/stampbrush.cpp

-                                                     mapDocument()->map()->staggerAxis());
+            WangFiller wangFiller(mWangSet,
+                                  dynamic_cast<StaggeredRenderer *>(mapDocument()->renderer()),
+                                  mapDocument()->map()->staggerAxis());


Please move the wangFiller out of the loop.

bjorn requested changes Aug 10, 2017

View reviewed changes

bjorn reviewed Aug 10, 2017

View reviewed changes

Bdtrotte added 10 commits August 10, 2017 13:14

WangColor now have properties: name, color, image, and probability

8713919

Refinment to wangDock. Added add/remove buttons for wangColors

e900a32

Added icons for adding edges/corners

0eb6c68

Added graphic for when wangbrush can't paint

e8a0bed

Status info now updates for wangBrush. Some code cleanup

87f09b5

Wangbrush works on staggered maps (exept for edge sets)

5bfaf08

WangFiller now supports staggered maps -> stamp and bucket do as well

f5d0282

TilesetView wangset graphics support isometric

ca5cb57

StaggeredRenderer::screenToTileCoords now supports sub-tile values ->…

fa1a8e9

… WangBrush edges and vertex mode works on staggered maps as expected

Fixed bug where tiled crashed when using a wangset from a different t…

2817fed

…ileset

Bdtrotte force-pushed the wip/wangtiles branch 4 times, most recently from fbfb52a to 126681e Compare August 10, 2017 23:05

Some renames, and other requested changes

e11f2d9

Bdtrotte force-pushed the wip/wangtiles branch from 126681e to e11f2d9 Compare August 10, 2017 23:27

bjorn requested changes Aug 15, 2017

View reviewed changes

Bdtrotte added 3 commits August 15, 2017 10:02

Made requested changes

508f2b2

Added outline to wang template view to show selected tile

d8acf51

Initialized mWangSet in stampBrush and bucketFill

5f72b4a

bjorn requested changes Aug 15, 2017

View reviewed changes

some minor changes

5f8d1a9

bjorn merged commit 5f8d1a9 into mapeditor:wip/wangtiles Aug 15, 2017

Bdtrotte deleted the wip/wangtiles branch August 15, 2017 21:21


		return referencePoint;
		QPointF newRel = HexagonalRenderer::tileToScreenCoords(referencePoint.x(), referencePoint.y());


		colors.insert(wangColor->colorIndex(), wangColor);

		for (int i = wangColor->colorIndex() + 1; i <= (wangColor->isEdge()? mEdgeColors : mCornerColors); ++i)

WangTiles: WangColor now have properties including name, color, image, and probability... #1670

WangTiles: WangColor now have properties including name, color, image, and probability... #1670

Conversation

Bdtrotte commented Jul 31, 2017

Bdtrotte commented Aug 5, 2017

bjorn commented Aug 6, 2017

Bdtrotte commented Aug 6, 2017

Bdtrotte commented Aug 7, 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

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

Choose a reason for hiding this comment

bjorn Aug 10, 2017 • edited Loading

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

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

bjorn Aug 10, 2017 •

edited

Loading