Wang Tiles #1582

Bdtrotte · 2017-05-29T22:00:41Z

An iterative pr for code review on the GSOC wang tiles project.

bjorn

I was a bit puzzled by the implementation of WangSet::getAllTiles. Essentially, it seems to be searching for tiles by creating all possible variations of a given WangId, when taking each 0 as a wildcard.

I would suggest a different approach, which will probably be more efficient even though it will not be using a map lookup:

Compute a mask instead of the list of wildcards. This is an unsigned value that has 0's where the values of the WangId do not matter and 1's where the values do matter (or 0xF, since all 4 bits need to be 1).
Loop over mTileIdToWangId and compare (tileWangId & mask) == wangId to see if the tileId is a match.

See other comments inline.

In general, for your project the same goes as for thabetx, in that it will probably make sense to look for several "checkpoints" at which we will merge into a work-in-progress branch, to avoid building up a huge single PR that will become difficult to review.

The first checkpoint should probably be to complete the data structures, integrating it with the Tileset class (probably as a list of WangSet instances) and implementing saving/loading of this information. Afterwards, the initial testing data to use while implementing the functionality of tiling an area can then be manually set up.

bjorn · 2017-06-04T19:05:50Z

src/libtiled/wangset.h

+     * @param edges requesting edge color (corners if false)
+     * @return
+     */
+    int getColor(int index, bool edges) const;


This method should really be split up in edgeColor(int index) and cornerColor(int index). It will be easier to read and perform slightly better.

bjorn · 2017-06-04T19:08:40Z

src/libtiled/wangset.h

+{
+public:
+    WangId(unsigned id){ mId = id; }
+    WangId() { mId = 0; }


Coding style: please use the following notation to initialize members: WangId() : mId(0) {} (similarly for the constructor taking an id).

bjorn · 2017-06-04T19:16:54Z

src/libtiled/wangset.h

+    WangId() { mId = 0; }
+
+    inline unsigned id() const { return mId; }
+    inline void setId(unsigned id) { mId = id; }


We could consider doing this instead:

operator unsigned() const { return mId; }

This way, we allow implicit conversion between WangId and unsigned in both directions (you already allow implicit conversion from unsigned to WangId by not marking the constructor as explicit).

bjorn · 2017-06-04T19:17:38Z

src/libtiled/wangset.h

+    return color;
+}
+
+void WangId::rotate(int rotations)


Missing inline (but maybe should move to cpp file anyway).

bjorn · 2017-06-04T20:26:09Z

src/libtiled/wangset.h

+     * @param wangId
+     * @return
+     */
+    QList<Tile*> getAllTiles(WangId wangId) const;


Probably should be called findMatchingTiles.

bjorn · 2017-06-04T20:28:59Z

src/libtiled/wangset.cpp

+
+WangId WangSet::getWangIdOfTile(Tile *tile) const
+{
+    if (tile->tileset() == mTileSet && mTileIdToWangId.contains(tile->id()))


These checks are not necessary, you can just return mTileIdToWangId.value(tile->id()), since when mTileIdToWangId does not contain the tile it will anyway return a default-constructed WangId.

bjorn · 2017-06-04T20:29:41Z

src/libtiled/wangset.h

+     * @brief wangIdOfTile returns the wangId of a given tileId
+     * @param tileId
+     */
+    WangId getWangIdOfTile(Tile *tile) const;


Naming style: should be just wangIdOfTile.

bjorn · 2017-06-04T20:32:35Z

src/libtiled/wangset.cpp

+    QList<Tile*> list;
+
+    //Stores the space of a wild card, followed by how many colors that space can have.
+    QVector<QPoint> wildCards;


Please use a custom struct instead of QPoint for readability sake.

bjorn · 2017-06-04T20:40:30Z

src/libtiled/wangset.cpp

+
+        for (int i = 0; i < wildCards.size(); ++i) {
+            stack.push(wildCards[i]);
+        }


Could use stack.append(wildCards) instead.

bjorn · 2017-06-04T20:50:07Z

src/libtiled/wangset.h

+    QString mName;
+    int mImageTileId;
+    QMultiMap<WangId, Tile*> mWangIdToTile;
+    QMap<int, WangId> mTileIdToWangId; //This could be stored in the tile object.


Consider using a QHash instead, which has O(1) lookup rather than the O(log(n)) of QMap. The difference is equivalent with std::map vs. std::unordered_map.

Bdtrotte · 2017-06-21T18:18:03Z

In the most recent push, I've fixed the changes requested in your first review, as well as added wangsets to Tilesets in the way you suggested (as a list). For the most part I modeled after the Terrains list, with the major exception of not tracking the id within the wangsets themselves. I don't think it will be necessary, though I've yet to look to far into how the interface will be built (which may need more certain ids?).

For now I've kept my method of finding wang tile variations as it was, as I still think despite the overhead of setting it up, in most cases it will run faster than searching through all the tiles (as most sets should be full), though this could require further investigation by me.

On that note further testing is still required, though Ill get file IO, and wang assignment working before I test too rigorously. (I did some small tests and all worked as expected).

Sorry for the delay in making progress, but it should be more steady from here on out!

bjorn

Alright, I've provided some more comments.

I'll also change the target branch to the new wip/wangtiles branch I've set up in my repository, so we can incrementally work on the project there until it is ready to be merged to master.

bjorn · 2017-06-21T20:03:15Z

src/libtiled/wangset.cpp

+typedef struct WangWildCard
+{
+    int index, colorCount;
+} WangWildCard;


This way of defining a struct was necessary in C times, to avoid needing to repeat the struct keyword whenever you would use WangWildCard. In C++, we can just write:

struct WangWildCard { int index; int colorCount; };

What a wonderful time we live in! Thanks for informing me

bjorn · 2017-06-21T20:06:44Z

src/libtiled/wangset.cpp

+
+                for (int i = 0; i < max; ++i) {
+                    idVariation |= stack[i].colorCount << stack[i].index;
+                }


Coding style: please leave out the braces for one-line bodies (also below).

bjorn · 2017-06-21T20:08:28Z

src/libtiled/wangset.h

+    inline void setId(unsigned id) { mId = id; }
+
+    /**
+     * @brief getColor returns the color of a desired edge/corner of a given wang id


Docs still need updating.

Yeah, for all the docs I just use qt's auto formatting. Though its a bit burdensome. Ill go through and update all those.

bjorn · 2017-06-21T20:09:31Z

src/libtiled/wangset.h

+    int cornerColor(int index) const;
+
+    /**
+     * @brief rotateWangId rotates the wang id 90 * rotations degrees ccw


Docs seem to mention old function name? I'd suggest to leave out the @brief and the function name.

bjorn · 2017-06-21T20:09:47Z

src/libtiled/wangset.h

+    /**
+     * @brief rotateWangId rotates the wang id 90 * rotations degrees ccw
+     * @param rotations 1-3
+     * @return


Returns nothing, so just leave out the @return.

bjorn · 2017-06-21T20:10:06Z

src/libtiled/wangset.h

+     * @param rotations 1-3
+     * @return
+     */
+    void rotate(int rotations);\


Stray \ at end of line.

bjorn · 2017-06-21T20:13:28Z

src/libtiled/wangset.h

+    WangSet *clone(Tileset *tileset) const;
+
+private:
+    Tileset *mTileSet;


Naming: "tileset" is always a single word in Tiled source code, so this should should be mTileset.

bjorn · 2017-06-21T20:14:30Z

src/libtiled/wangset.h

+
+    /**
+     * @brief wangIdOfTile returns the wangId of a given tileId
+     * @param tileId


All docs seem outdated. Also, if you don't provide any comment on a parameter, there is no need to mention it with @param.

bjorn · 2017-06-21T20:17:28Z

src/libtiled/wangset.h

+    int mEdgeColors;
+    int mCornerColors;
+    QMultiHash<WangId, Tile*> mWangIdToTile;
+    QHash<int, WangId> mTileIdToWangId; //This could be stored in the tile object.


Whether it could be stored in the tile object depends on whether we allow a tile to be part of multiple wang sets. Would limiting a tile to a single wang set be useful in some way, beyond the small possible performance improvement?

bjorn · 2017-06-21T20:22:43Z

src/libtiled/wangset.cpp

+    mEdgeColors(edgeColors),
+    mCornerColors(cornerColors)/*,
+    mWangIdToTile(QMultiHash<WangId, Tile*>()),
+    mTileIdToWangId(QHash<int, WangId>())*/


Just remove these lines.

Knew I forgot something...

MoctezumaDev · 2017-06-22T08:51:43Z

src/libtiled/wangset.cpp

-        for (int i = 0; i < wildCards.size(); ++i) {
-            stack.push(wildCards[i]);
-        }
+        stack.append(wildCards);


I checked the documentation, check the following link.

http://doc.qt.io/qt-5/qvector.html#append-2

The method to append another qvector was introduced in Qt 5.5 so there is a difference in the Qt version that you are using and the one that the Travis CI uses, I would suggest you to get Qt 5.4.2 which is the officially minimal supported to build Tiled.

You might be able to use += or << operators instead.

I guess we could also just raise the minimum Qt version to 5.5. I wanted to quickly look up if those operators would work as an alternative, but http://doc.qt.io/archives/index.html does not even host Qt 5.4 documentation anymore.

In general, it does not matter which version is used for development but I'd recommend simply using the latest version, since that is likely the version the next Tiled release is going to be made against. We can check the minimum supported version using Travis CI.

So for the time being, should I change that part of the code to comply with the older version? Or just leave it as is?

Bjorn has more a word on this than I, he is the product owner, but I would say that if += work in older versions and in newers aswell, then just use the operator.

Right, looking at the Qt 5.4 docs in Qt Creator, the operator+= does work with another QVector, so please just use that instead of append, so we can have it in a single line without raising the Qt dependency.

Bdtrotte · 2017-06-24T02:10:30Z

I've implemented file saving and loading, though I've not yet tested rigorously... I wanted to first make sure I'm doing things correctly, and that the format I've used is good, and is in line with current formatting.

bjorn · 2017-06-24T20:42:38Z

I've implemented file saving and loading, though I've not yet tested rigorously... I wanted to first make sure I'm doing things correctly, and that the format I've used is good, and is in line with current formatting.

Please change the naming style to keep the tags all lowercase. About the format in general, I found it somewhat unexpected that there is a wanginfo tag nested in the tile tags. I had rather expected all the information to be stored along with the Wang set. For making the reading easier, this would mean storing the Wang sets after the tiles.

Bdtrotte · 2017-06-25T02:59:09Z

Yes, that was my first thought for how to do it, but figured it would be easier for it to be read right after the tile is created. Also I wanted to avoid reprinting the tile Id and such for each wangset, though it probably ends up being about the same number of lines. I mostly liked that it maintains all the info for a given tile in the single block.

Did you think one way was better than the other?

bjorn · 2017-06-25T13:55:48Z

Did you think one way was better than the other?

I think keeping all the information together in the wangset is better, because it keeps the feature in general cleanly separated from the rest, both in the format and in the code.

Bdtrotte · 2017-06-27T16:27:34Z

Not yet in a "working" state, but the foundation for displaying, creating, and editing wangSets.

…els and views.

Bdtrotte · 2017-06-28T19:05:21Z

The latest commit is ready for review! It completes the support for creating and editing the basic properties of a wangset from the UI.

Features still needed for this are assigning the image for a wang set.

Next to come is of course actually assigning the wangIds to the tiles. I see this as an iterative thing; Ill get a very basic implementation working first, then build it up into a more elegant UI. This will be done over the next week or so.

Before I do that Ill get a basic implementation of the wang bucket fill working to test out, and show off those methods.

Now that this is done, I feel progress will continue more quickly!

…les) stays grouped together

Zireael07 · 2017-06-29T06:37:01Z

"addremocewangset"? Did you mean "remove"?

Bdtrotte · 2017-06-29T20:13:33Z

A working test of the wang fill bucket is in this pr! Bdtrotte#1

bjorn

I tried out your Wang filling bucket tool and I think it looks very promising. You have made very good progress this week!

Unfortunately, it will take until Monday before I can do a detailed review of your patches. Please take a chance tomorrow to clean up the work so far and maybe fill in some more bits of functionality (also I think you should make the test tileset part of this PR). The main goal should be to have something we can merge into the work-in-progress branch early next week. Then we can plan the next steps and you can open a new pull request to wip/wangtiles.

bjorn · 2017-06-29T21:47:08Z

src/libtiled/mapreader.cpp

+                    int tileId = tileAtts.value(QLatin1String("tileid")).toInt();
+                    int wangId = tileAtts.value(QLatin1String("wangid")).toInt();
+
+                    wangSet->addTile(tileset.tileAt(tileId), wangId);


This should be findOrCreateTile, since you can't assume the tile is already allocated. In my case this was crashing with your test tileset, since the tiles are usually allocated later, when the image is loaded.

…gId based on surrounding tiles.

… reflects this

Bdtrotte · 2017-07-02T23:45:04Z

Pending further review of everything, I believe the work so far is mergable into the work in progress branch. Currently all the base data, and core UI is set up and should be ready to be used in further expansion of the feature. My next step is going to be to focus on making the wangsets available in the map editor, and a tool bar button for activating wangfilling methods. After which I will focus back on the assignment of tiles to a wangId.

bjorn

The file qtc_Desktop__bdef64c0-debug.bg should not be committed, please remove it again.

In general, it's impressive how much code you've already written, though of course it was largely duplicating the closely related code for terrains. With all these similarities, I really hope we can eventually base the terrain tool on this one. It should be possible, given that the terrain tool is just a special case, being a WangSet with corner count equal to the number of terrain types.

I also quite liked to see the implementation of WangSet::wangIdFromSurrounding. I think it could be a little more readable if we added WangId::setCornerColor and setEdgeColor. And I think eventually we'll want to have a version that works without copying, for performance reasons. But this is fine for now.

In any case, I think I left a rather large amount of small comments. I hope you can make them tomorrow, so that we can merge this and continue with a new pull request.

Thanks for the work so far!

bjorn · 2017-07-03T15:47:58Z

src/libtiled/mapreader.cpp

@@ -564,6 +568,52 @@ void MapReaderPrivate::readTilesetTerrainTypes(Tileset &tileset)
    }
 }

+void MapReaderPrivate::readTilesetWangSet(Tileset &tileset)


Naming: should be readTilesetWangSets, since it will read multiple sets.

bjorn · 2017-07-03T15:48:58Z

src/libtiled/mapreader.cpp

+
+            WangSet *wangSet = new WangSet(&tileset, edges, corners, name, tile);
+
+            tileset.insertWangSet(wangSet);


Naming: this should be called addWangSet, since "insertion" generally is done at a certain index.

bjorn · 2017-07-03T15:49:25Z

src/libtiled/mapreader.cpp

+            tileset.insertWangSet(wangSet);
+
+            while (xml.readNextStartElement()) {
+                if (xml.name() == QLatin1String("properties"))


Coding style: should have { and } because the else if part has them too (it's a rule from the Qt coding style).

bjorn · 2017-07-03T15:52:10Z

src/libtiled/mapreader.cpp

+                    unsigned wangId = tileAtts.value(QLatin1String("wangid")).toUInt();
+                    bool fH = tileAtts.value(QLatin1String("flippedhorizontally")).toInt();
+                    bool fV = tileAtts.value(QLatin1String("flippedvertically")).toInt();
+                    bool fA = tileAtts.value(QLatin1String("flippedantidiagonally")).toInt();


In the interest of avoiding really long names, I wonder if these should be "hflip", "vflip" and "dflip"?

bjorn · 2017-07-03T15:53:00Z

src/libtiled/mapreader.cpp

+                    wangSet->addWangTile(wangTile);
+
+                    xml.skipCurrentElement();
+                } else


Coding style: Should have { and } because because the previous part of the condition has them.

bjorn · 2017-07-03T21:01:26Z

src/tiled/tileseteditor.cpp

+void TilesetEditor::addWangSet()
+{
+    Tileset *tileset = currentTileset();
+    if(!tileset)


Coding style: space after if.

bjorn · 2017-07-03T21:02:27Z

src/tiled/tileseteditor.cpp

+    if(!tileset)
+        return;
+
+    //2 and 0 are default values for number of edges and corners TODO define this some where better?


This is fine for now, though we should probably add a "NewWangSetDialog" class so that the user can set up his wang set upon creation (where there would be space to explain the corner/edges count a bit and show the presets).

bjorn · 2017-07-03T21:04:22Z

src/tiled/tileseteditor.cpp

+
+void TilesetEditor::removeWangSet()
+{
+    WangSet *wangSet = mWangDock->currentWangSet();


Please do check that there is a current wang set, even if the button to trigger this action should be disabled at this point.

bjorn · 2017-07-03T21:08:50Z

src/tiled/tilesetwangsetmodel.h

+ * OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
+ * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
+ * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.


Please make sure your files use the right copyright header. This is the header used for libtiled, but this file is part of Tiled itself and should have the GPL header.

bjorn · 2017-07-03T21:09:57Z

src/tiled/tilesetwangsetmodel.h

+        WangSetRole = Qt::UserRole
+    };
+
+    TilesetWangSetModel(TilesetDocument *mapDocument,


This constructor should be marked explicit, to avoid potential implicit conversion from TilesetDocument * to TilesetWangSetModel (however unlikely to run into).

…ngTiles

bjorn

A view more adjustments needed.

bjorn · 2017-07-04T07:54:01Z

src/libtiled/wangset.cpp

+
+Cell WangTile::makeCell() const
+{
+    if(!mTile)


Please make coding style adjustments in all needed places, not just where I point them out. This is still one of 10 places where there is no space after the if.

bjorn · 2017-07-04T07:55:00Z

src/tiled/tilesetwangsetmodel.h

+
+    WangSet *wangSetAt(const QModelIndex &index) const;
+
+    void insertWangSet(WangSet *wangSet, int index);


All the other functions take index as first parameter, please be consistent and do the same here.

bjorn · 2017-07-04T07:57:12Z

src/tiled/mapdocument.h

@@ -315,6 +328,8 @@ private slots:
    MapRenderer *mRenderer;
    Layer* mCurrentLayer;
    MapObjectModel *mMapObjectModel;
+    TerrainModel *mTerrainModel;


You've removed the getter, but this member should not be added either, nor should the forward declaration and the tilesetTerrain... set of signals.

Since your WangSetModel now follows the same logic as the TerrainModel, it should also not be part of the MapDocument, nor should the tilesetWangSet... set of signals.

bjorn · 2017-07-04T07:58:34Z

src/tiled/changewangsetdata.cpp

+
+ChangeWangSetCorners::ChangeWangSetCorners(TilesetDocument *tilesetDocument,
+                                            int index,
+                                            int newValue)


Indentation off by one.

bjorn · 2017-07-04T19:20:22Z

Thanks for your work so far!

Please open a new pull request today or tomorrow for the next steps. You should decide on a work package that you estimate will take about one week, so that we can aim for another merge on Monday or Tuesday next week.

Please don't add merge commits to the pull request. It is confusing on GitHub and makes the history less clean. I had to forward the wip/wangtiles branch before doing the squashed merge to make sure the history still made sense. I don't think we will need further changes that will be made on master in the meantime, but if we do then I will do a merge in between pull requests. You should base further changes on the wip/wangtiles branch in my repository, and rebase any commits you may have already done onto it.

I think the general set up for the wang tiles is really promising, so I'm really looking forward to see the rest of the UI and the tools materialize!

Bdtrotte added 2 commits May 28, 2017 15:09

Created WangSet class, as well as some functions for wangIds

904e8bc

Moved wangId into a class of its own

cb8d9ae

bjorn requested changes Jun 4, 2017

View reviewed changes

Bdtrotte added 3 commits June 10, 2017 22:21

Merge branch 'master' into WangTiles

e93cc74

Merge branch 'master' of https://github.com/bjorn/tiled into WangTiles

c329a97

Refined WangSet class, and added them into Tileset

4757fb6

bjorn requested changes Jun 21, 2017

View reviewed changes

bjorn changed the base branch from master to wip/wangtiles June 21, 2017 20:26

Made requested changes

31689c6

MoctezumaDev reviewed Jun 22, 2017

View reviewed changes

Enabled file saving and loading. (Needs testing)

e10151e

Fixed some issues with saving/loading

c7e67d0

Style update

0ef5add

Foundation for adding UI support to wangTile assignment

74dbae3

Bdtrotte added 3 commits June 27, 2017 09:48

merged upstream

400d47c

Added a couple Undo commands, and made a bit more progress on the mod…

ff6c624

…els and views.

WangSets are now able to be created and edited from the ui

3916cd6

Bdtrotte added 2 commits June 28, 2017 14:12

Added new files to tiled.pro

acae98a

adjusted reading/writting so that all the wang set info (including ti…

68bd9af

…les) stays grouped together

Bdtrotte added 2 commits June 29, 2017 10:11

Cleaned up a few things

52ae862

remoced references to 'addremocewangset

4398cea

bjorn requested changes Jun 29, 2017

View reviewed changes

Bdtrotte added 3 commits June 30, 2017 17:14

Now can assign wangsets an image, added functions for getting the wan…

7f58709

…gId based on surrounding tiles.

Beging to store info about tile rotation and inversion

bb6b98a

Wangsets now store info about rotation and flipping, mapreader/writer…

0969d46

… reflects this

bjorn requested changes Jul 3, 2017

View reviewed changes

Bdtrotte added 4 commits July 3, 2017 21:28

Merge branch 'wip/wangtiles' into WangTiles

8b959a9

Made requested changes

1df48f5

Merge branch 'WangTiles' of https://github.com/Bdtrotte/tiled into Wa…

25f8494

…ngTiles

fixed issue in 'wangIdFromSurrounding'

888ea7b

bjorn requested changes Jul 4, 2017

View reviewed changes

Bdtrotte and others added 2 commits July 4, 2017 10:50

Made requested changes!

825774a

Removed file that should not be in version control

0d3f353

bjorn merged commit 9e174f4 into mapeditor:wip/wangtiles Jul 4, 2017

Bdtrotte deleted the WangTiles branch July 4, 2017 19:15


		WangSet *wangSet = new WangSet(&tileset, edges, corners, name, tile);

		tileset.insertWangSet(wangSet);


		WangSet *wangSetAt(const QModelIndex &index) const;

		void insertWangSet(WangSet *wangSet, int index);

Wang Tiles #1582

Wang Tiles #1582

Conversation

Bdtrotte commented May 29, 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

Bdtrotte commented Jun 21, 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

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 Jun 24, 2017

bjorn commented Jun 24, 2017

Bdtrotte commented Jun 25, 2017

bjorn commented Jun 25, 2017

Bdtrotte commented Jun 27, 2017

Bdtrotte commented Jun 28, 2017

Zireael07 commented Jun 29, 2017

Bdtrotte commented Jun 29, 2017

bjorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

bjorn commented Jul 4, 2017