Stamp brush now works with the stagger in hex mode as expected #1496

Bdtrotte · 2017-03-16T22:05:47Z

Previously when using the stamp brush tool when in hexagonal mode, moving down rows would cause a stamp to behave strangely (not accounting for the stagger). This has been patched over by shifting every other row of the stamp over in the appropriate direction when needed.

This is my first contribution, and major edit of tiled, so feedback on how anything could be done better would be greatly appreciated!

Ben

ketanhwr · 2017-03-17T04:02:22Z

src/tiled/stampbrush.cpp

+            TileLayer *stamp = variation.tileLayer()->copy(variation.tileLayer()->bounds());
+
+            //If using a hexagonal map, this takes into acount staggering
+            if(mapDocument()->map()->orientation() == 4 && stamp->height()>1)


Code style: Add a space between if and (

Also I forgot to mention, but move the curly bracket to this line itself.

if (mapDocument()->map()->orientation() == 4 && stamp->height() > 1) {

ketanhwr · 2017-03-17T04:03:38Z

src/tiled/stampbrush.cpp

+            {
+                int shiftMode;//-1 do not shift, 0 shift even rows (relatived to the stamp), 1 shift odd rows
+                int rowOfTop = p.y()-stamp->height()/2;
+                if(rowOfTop<0)rowOfTop*=-1; //Only used to check if even or odd, and being negative can mess this up.


Always add spaces to make the code more readable. Also, add the statement rowOfTop*=-1 in the next line :)

if (rowOfTop < 0) rowOfTop *= -1;

ketanhwr · 2017-03-17T04:03:57Z

src/tiled/stampbrush.cpp

+                int rowOfTop = p.y()-stamp->height()/2;
+                if(rowOfTop<0)rowOfTop*=-1; //Only used to check if even or odd, and being negative can mess this up.
+
+                if(mCaptureTopCorner == -1)


Space between if and (

ketanhwr · 2017-03-17T04:04:31Z

src/tiled/stampbrush.cpp

+
+                if(mCaptureTopCorner == -1)
+                    shiftMode = 1;
+                else


Why have you added an else construct when you don't have to execute any statements under it?

that else should pair up with the if under it (separated by a comment). Though i will clean it up for readability!

Oh, I got confused by that commend :/

ketanhwr · 2017-03-17T04:04:55Z

src/tiled/stampbrush.cpp

+                    shiftMode = 1;
+                else
+                //if what the stamp captured starts by going left
+                if((mCaptureTopCorner+1)%2 == mapDocument()->map()->staggerIndex())


Space between if and (

Move the curly bracket here too.

if ((mCaptureTopCorner+1)%2 == mapDocument()->map()->staggerIndex()) {

ketanhwr · 2017-03-17T04:05:11Z

src/tiled/stampbrush.cpp

+                if((mCaptureTopCorner+1)%2 == mapDocument()->map()->staggerIndex())
+                {
+                    //if where the top is now will go down the correct way
+                    if((rowOfTop+1)%2 != mapDocument()->map()->staggerIndex())


ketanhwr · 2017-03-17T04:05:43Z

src/tiled/stampbrush.cpp

+                    else
+                        shiftMode = -1;
+                }
+                else//if going right


Code style: Shift the else to previous line like this:

} else {

ketanhwr · 2017-03-17T04:05:49Z

src/tiled/stampbrush.cpp

+                else//if going right
+                {
+                    //if where the top is now will go down the correct way
+                    if((rowOfTop+1)%2 == mapDocument()->map()->staggerIndex())


ketanhwr · 2017-03-17T04:05:56Z

src/tiled/stampbrush.cpp

+                        shiftMode = -1;
+                }
+
+                if(shiftMode != -1)


ketanhwr · 2017-03-17T04:06:13Z

src/tiled/stampbrush.cpp

+                    QRect boundBox = stamp->bounds();
+                    //Actually does the shift of the tiles. Maybe this could be done as
+                    //a method of TileLayer?
+                    for(int i = shiftMode; i <= boundBox.bottom(); i+=2)


Add a space between for and ( too :)

Here as well.

for () {

ketanhwr · 2017-03-17T04:06:22Z

src/tiled/stampbrush.cpp

+                    //a method of TileLayer?
+                    for(int i = shiftMode; i <= boundBox.bottom(); i+=2)
+                    {
+                        for(int j = boundBox.right()-1; j >= 0; --j)


Here as well.

haha,DoyouthinkIshouldaddsomespaces? Ill get on cleaning up the style! Thanks for pointing it all out.
Ben

It's just a style everyone should follow in order to maintain readability and consistency :)

Btw, move the curly brackets up here as well.

bjorn · 2017-03-20T13:36:11Z

@ketanhwr When you do a review, please use the "Start a review" button instead of "Add a single comment". It's something I had to get used to after they added the feature and I still sometimes forget, but it makes the whole review process a lot less spammy in terms of e-mail notifications. As a bonus, it means you can tweak all your comments until the time when you feel ready to submit your review. But thanks for stepping in to comment!

bjorn

I think it's a good start and in trying it out I really felt the potential. A great quality of life improvement for editing of staggered maps! (note that the same logic applies to isometric staggered maps, not just hexagonal staggered maps).

Implementation wise, I had some comments both on details and on the general approach.

bjorn · 2017-03-20T13:38:43Z

src/tiled/stampbrush.cpp

@@ -45,6 +45,7 @@ StampBrush::StampBrush(QObject *parent)
                       parent)
    , mBrushBehavior(Free)
    , mIsRandom(false)
+    , mCaptureTopCorner(-1)


Please heed the compiler warnings:

src/tiled/stampbrush.h: In constructor ‘Tiled::Internal::StampBrush::StampBrush(QObject*)’: src/tiled/stampbrush.h:138:10: warning: ‘Tiled::Internal::StampBrush::mIsRandom’ will be initialized after [-Wreorder] bool mIsRandom; ^~~~~~~~~ src/tiled/stampbrush.h:106:9: warning: ‘int Tiled::Internal::StampBrush::mCaptureTopCorner’ [-Wreorder] int mCaptureTopCorner; ^~~~~~~~~~~~~~~~~ src/tiled/stampbrush.cpp:40:1: warning: when initialized here [-Wreorder] StampBrush::StampBrush(QObject *parent) ^~~~~~~~~~

bjorn · 2017-03-20T13:40:05Z

src/tiled/stampbrush.cpp

@@ -250,6 +251,8 @@ void StampBrush::endCapture()

    // Intersect with the layer and translate to layer coordinates
    QRect captured = capturedArea();
+    mCaptureTopCorner = captured.top();


I think remembering only the top of the captured area will not be enough, because the staggered axis may be either the X or the Y axis (it's a map setting). By storing just the top, you can only handle a staggering Y axis.

bjorn · 2017-03-20T13:44:42Z

src/tiled/stampbrush.cpp

+            TileLayer *stamp = variation.tileLayer()->copy(variation.tileLayer()->bounds());
+
+            //If using a hexagonal map, this takes into acount staggering
+            if (mapDocument()->map()->orientation() == 4 && stamp->height()>1) {


Please don't hardcode a number like 4. That should be Map::Hexagonal.

Coding style: use spaces for readability: stamp->height() > 1

Also, when the staggering axis is the X axis, this needs to check the width of course.

bjorn · 2017-03-20T13:47:02Z

src/tiled/stampbrush.cpp

+                    //Actually does the shift of the tiles. Maybe this could be done as
+                    //a method of TileLayer?
+                    for (int i = shiftMode; i <= boundBox.bottom(); i+=2)
+                    {


Coding style: the { for if, for, etc. should be on the same line.

bjorn · 2017-03-20T13:53:22Z

src/tiled/stampbrush.cpp

@@ -385,7 +388,47 @@ void StampBrush::drawPreviewLayer(const QVector<QPoint> &list)
            const TileStampVariation variation = mStamp.randomVariation();
            mapDocument()->unifyTilesets(variation.map, mMissingTilesets);

-            TileLayer *stamp = variation.tileLayer();
+            TileLayer *stamp = variation.tileLayer()->copy(variation.tileLayer()->bounds());


Please don't copy the layer when it isn't necessary (so it should only be done once you have determined the map is hexagonal and you need to shift some rows). Also, if you copy, you need to make sure the copy is deleted. I think the easiest way will be to define a:

QScopedPointer<TileLayer*> copy;

And to set it along with setting the stamp variable to the copy, when you make it.

bjorn · 2017-03-20T13:56:19Z

src/tiled/stampbrush.cpp

+                        for (int j = boundBox.right()-1; j >= 0; --j)
+                        {
+                            stamp->setCell(j+1,i,stamp->cellAt(j,i));
+                            stamp->setCell(j,i,Cell());


Why use j and i instead of the more common x and y? Also, this last assignment does not need to be in the inner loop.

bjorn · 2017-03-20T13:59:19Z

src/tiled/stampbrush.cpp

+                }
+
+                if (shiftMode != -1) {
+                    stamp->resize(QSize(stamp->width()+1,stamp->height()),QPoint());


Coding style: please use spaces around operators and after commas to increase readability (applies to whole patch):

stamp->resize(QSize(stamp->width() + 1, stamp->height()), QPoint());

bjorn · 2017-03-20T14:01:11Z

src/tiled/stampbrush.cpp

+                    QRect boundBox = stamp->bounds();
+                    //Actually does the shift of the tiles. Maybe this could be done as
+                    //a method of TileLayer?
+                    for (int i = shiftMode; i <= boundBox.bottom(); i+=2)


I see no advantage to using <= boundBox.bottom() here instead of just < stamp->height().

bjorn · 2017-03-20T14:08:52Z

src/tiled/stampbrush.cpp

+                if (rowOfTop<0)
+                    rowOfTop*=-1; //Only used to check if even or odd, and being negative can mess this up.
+
+                if (mCaptureTopCorner == -1)


It's interesting how this breaks the shift adjustment for captures made starting from -1, just outside the map.

In any case, you will need to associate this data with the tile layer instance rather than storing it as a member of the stamp brush. In fact, the tile layer already has a member for exactly that, which is the staggerIndex. All you should need to do, is to make sure this property gets set correctly when the stamp is captured, and to account for it when the stamp is used at odd indexes (or at even indexes, when the stamp stagger index != the map's stagger index).

bjorn · 2017-03-20T14:26:55Z

In any case, you will need to associate this data with the tile layer instance rather than storing it as a member of the stamp brush. In fact, the tile layer already has a member for exactly that, which is the staggerIndex. All you should need to do, is to make sure this property gets set correctly when the stamp is captured, and to account for it when the stamp is used at odd indexes (or at even indexes, when the stamp stagger index != the map's stagger index).

Sorry, I completely forgot that the staggerIndex is stored on the map rather than on the tile layer. Thankfully, at some point I changed the stamp to always store a map with a single tile layer, in order to keep track of exactly this kind of information. So you should be able to use that.

Bdtrotte · 2017-03-21T01:01:11Z

Made all the changes, now works with isometric and x-axis stagger nicely! Feel much better about this version. Hopefully there are not too many style issues this time...

bjorn

Yep, that's much better! But it can still be improved in terms of performance, code style and readability.

Note that I won't comment on the same coding style issue repeatedly. I expect you to take each missed coding style issue into account for the whole patch.

bjorn · 2017-03-21T07:25:42Z

src/tiled/stampbrush.cpp

+    if (tileLayer->map()->staggerAxis() == Map::StaggerX)
+        stagIndexOffSet = captured.x() % 2;
+    else
+        stagIndexOffSet = captured.y() % 2;

    captured &= QRect(tileLayer->x(), tileLayer->y(),
                      tileLayer->width(), tileLayer->height());


Don't forget to remove these lines, since you moved the code up.

bjorn · 2017-03-21T07:29:14Z

src/tiled/stampbrush.cpp

            preview->merge(op.pos - bounds.topLeft(), op.stamp);
+            if(op.isCopy)


Coding style: please put a space after if

bjorn · 2017-03-21T07:30:39Z

src/tiled/stampbrush.cpp

+                        stamp->resize(QSize(stamp->width() + 1,stamp->height()),QPoint());
+
+                        for (int y = (variation.map->staggerIndex()+1)&1; y < stamp->height(); y += 2) {
+                            for(int x = stamp->width() - 2; x >= 0; --x) {


Coding style: space after for

bjorn · 2017-03-21T07:32:16Z

src/tiled/stampbrush.cpp

+                        for (int y = (variation.map->staggerIndex()+1)&1; y < stamp->height(); y += 2) {
+                            for(int x = stamp->width() - 2; x >= 0; --x) {
+                                stamp->setCell(x+1,y,stamp->cellAt(x,y));
+                                stamp->setCell(x,y,Cell());


This line can still be moved out of the inner loop, since it only needs to be done for the left-most cell.

bjorn · 2017-03-21T07:38:44Z

src/tiled/stampbrush.cpp

+                    && ((mapDocument()->map()->staggerAxis() == Map::StaggerY)?
+                         stamp->height() > 1 : stamp->width() > 1)) {
+                stamp = variation.tileLayer()->copy(variation.tileLayer()->bounds());
+                copyFlag = true;


At this point you still don't know whether it is actually necessary to create a copy. Also, you only ever need a single adjusted copy, since there is only a single possible adjustment depending on the context. So I think you can really just store it in a QScopedPointer<TileLayer*> copy (or maybe call it shiftedCopy) and create it on-demand.

Oops, I totally forgot about the randomVariation() part. You could still do a cache like:

QHash<TileLayer*, TileLayer*> shiftedCopies; ... qDeleteAll(shiftedCopies);

Is there a disadvantage to how I'm doing it now with the copy flag? As we already have to loop through all the operations so I thought deleting the copies there with delete would be fastest. Ill go ahead and implement with the QHash and push a new commit, but I'm curious of the difference.

It is only a performance concern. You're making a copy for each point where the stamp is to be drawn. With a somewhat large stamp in combination with a large amount of points (so either drawing a long line or large circle), this consumes unnecessary memory and CPU cycles.

Even though this could count as "premature optimization", I think the more performant code where only a single copy is made per variation (and there is usually only one variation) is trivial to write and then I think why not just do it.

Yep, you're totally right. I'll work on fixing that. Do you think there is a clean way of not having to make to copy but once when the stamp is captured (or a map setting is changed)? I just couldn't think where to store this.

I don't think that is possible in an easy way. So at that point, the optimization becomes too much of an engineering effort and it's not worth doing it unless we notice an actual performance issue with the easy approach.

bjorn · 2017-03-21T07:41:30Z

src/tiled/stampbrush.cpp

+                    if ((variation.map->staggerIndex() == mapDocument()->map()->staggerIndex()
+                            && (rowOfTop&1) == 1)
+                               ||
+                           (variation.map->staggerIndex() != mapDocument()->map()->staggerIndex()


You're doing mapDocument()->map()->staggerIndex() so often that it really makes sense to introduce a local mapStaggerIndex (and a stampStaggerIndex) to make the code shorter and more readable.

bjorn · 2017-03-21T07:44:27Z

src/tiled/stampbrush.cpp

+                    int rowOfTop = p.y() - stamp->height() / 2;
+
+                    if ((variation.map->staggerIndex() == mapDocument()->map()->staggerIndex()
+                            && (rowOfTop&1) == 1)


Coding style: please put spaces around &.

Also, since you're not using rowOfTop for any other computation, you can change that local variable to bool oddOffset for example.

bjorn · 2017-03-21T07:47:39Z

src/tiled/stampbrush.cpp

+
+            //if staggered map, makes sure stamp stays the same
+            if ((mapDocument()->map()->orientation() == Map::Hexagonal
+                    || mapDocument()->map()->orientation() == Map::Staggered)


Feel free to introduce a bool Map::isStaggered() const method to make this code more readable.

I have done this. Good idea.

bjorn · 2017-03-21T07:53:35Z

src/tiled/stampbrush.cpp

+                            && (colOfLeft&1) == 1)
+                               ||
+                           (variation.map->staggerIndex() != mapDocument()->map()->staggerIndex()
+                            && (colOfLeft&1) == 0)) {


I believe this whole condition could be written as:

if ((stampStaggerIndex == mapStaggerIndex) == oddOffset) {

bjorn · 2017-03-21T07:54:19Z

src/tiled/stampbrush.cpp

+
+                        for (int x = (variation.map->staggerIndex() + 1)&1; x < stamp->width(); x += 2) {
+                            for(int y = stamp->height() - 2; y >= 0; --y) {
+                                stamp->setCell(x,y+1,stamp->cellAt(x,y));


Coding style: please put spaces after commas.

bjorn · 2017-03-21T08:01:21Z

src/tiled/stampbrush.cpp

+                      tileLayer->width(), tileLayer->height());
+
+    //gets if the relative stagger should be the same as the base layer
+    int stagIndexOffSet;


One more comment: there is no point in shortening "stagger" here, so just call it staggerIndexOffset.

Bdtrotte · 2017-03-21T15:58:31Z

Made the latest batch of requested improvements and style updates!

bjorn · 2017-03-21T20:12:21Z

src/tiled/stampbrush.cpp

+
+                    if ((stampStaggerIndex == mapStaggerIndex) == topIsOdd) {
+                        stamp = variation.tileLayer()->copy(variation.tileLayer()->bounds());
+                        shiftedCopies.insert(stamp, stamp);


Before you go and make the copy and insert it, you should of course check whether it isn't already there (meaning as well, that you should use the original stamp as the key, and not the copy). If a shifted copy was already made for this stamp, just assign that to stamp and don't do the shifting again.

Could it be easier to just use a bool flag to see if we've made the copy? As we are only storing one copy. Or is there an advantage to the QHash?

Or for that matter just checking is a TileLayer * == nullptr and if so make the copy, and if not set stamp = copy

No, it's not easier with a bool flag, because you need a bool flag for each variation. This also means we're storing potentially multiple copies (one for each variation that ends up being used). Hence we need the QHash. You see if you've made a copy by doing:

TileLayer *shiftedStamp = shiftedCopies.value(stamp); if (!shiftedStamp) { shiftedStamp = static_cast<TileLayer*>(stamp->clone()); shiftedCopies.insert(stamp, shiftedStamp); // do the shifting } stamp = shiftedStamp;

Forgot to take into account the variations! Sorry, still learning tiled and qt as I go!

bjorn · 2017-03-21T20:12:47Z

src/tiled/stampbrush.cpp

            preview->merge(op.pos - bounds.topLeft(), op.stamp);
+        }


Coding style: please remove these brackets again now.

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

src/tiled/stampbrush.cpp

+                    bool topIsOdd = (p.y() - stamp->height() / 2) & 1;
+
+                    if ((stampStaggerIndex == mapStaggerIndex) == topIsOdd) {
+                        stamp = variation.tileLayer()->copy(variation.tileLayer()->bounds());


Just use stamp instead of variation.tileLayer(). It's the same thing after all.

bjorn

Last request for changes, I'm positive!

bjorn · 2017-03-21T21:03:17Z

src/tiled/stampbrush.cpp

@@ -250,9 +250,17 @@ void StampBrush::endCapture()

    // Intersect with the layer and translate to layer coordinates
    QRect captured = capturedArea();
+


No need to add this line.

bjorn · 2017-03-21T21:03:53Z

src/tiled/stampbrush.cpp

+    if (tileLayer->map()->staggerAxis() == Map::StaggerX)
+        staggerIndexOffSet = captured.x() % 2;
+    else
+        staggerIndexOffSet = captured.y() % 2;


Please move this bit of code down to after if (captured.isValid()) {, since you don't need the variable yet here.

Bdtrotte · 2017-03-21T21:24:41Z

Thank you for being so patient as I get used to the style guidelines, qt, and tiled in general!

bjorn · 2017-03-21T22:17:45Z

Thank you for being so patient as I get used to the style guidelines, qt, and tiled in general!

No problem! You also need to get used to my rigorous review process, and to look carefully at your code to see if it could be written better, shorter, faster, etc. :-)

Now that we got this in, it occurred to me that of course it could probably have been done without copying and modifying the stamp at all. Instead, there could be some kind of specialized version of TileLayer::merge that can apply the shifting as it merges. I'll leave that as a possible exercise to explore later...

Bdtrotte · 2017-03-21T22:25:34Z

Rigorous review is great! Lots of good feed back that made it a lot better.

Instead, there could be some kind of specialized version of TileLayer::merge that can apply the shifting as it merges. I'll leave that as a possible exercise to explore later...

Haha, probably right! Ill take a breather to focus on rotation for a bit. Which speaking of, passing the stagger index to the stamp's map solved a lot of the problems I was having with rotation, and it now works perfectly! There was, though, some oddities with flipping (which I think is just due to that process not accounting for the stagger) which I need to work out. As well go over the code with a very fine comb to get through review a little easier :).

Stamp brush now works with the stager as expected

1ecedf6

ketanhwr reviewed Mar 17, 2017

View reviewed changes

src/tiled/stampbrush.cpp Outdated

shiftMode = -1;

}

if(shiftMode != -1)

Copy link

Contributor

ketanhwr Mar 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too.

ketanhwr reviewed Mar 17, 2017

View reviewed changes

Style Updates

e7a79e6

Bdtrotte mentioned this pull request Mar 20, 2017

Rotating of hexagonal tile stamps #1476

Closed

bjorn requested changes Mar 20, 2017

View reviewed changes

Bdtrotte added 2 commits March 20, 2017 11:07

Slight cleanup. Set up to move changes to new branch

02fb06d

Made requested changes, and general inprovments, style updates

94a76db

bjorn requested changes Mar 21, 2017

View reviewed changes

bjorn reviewed Mar 21, 2017

View reviewed changes

Bdtrotte added 2 commits March 21, 2017 08:52

General inprovments and style updates

2d9d6c8

to merge with upsream master

9814c37

bjorn requested changes Mar 21, 2017

View reviewed changes

Bdtrotte added 2 commits March 21, 2017 13:51

No longer makes unnecessary copies!

b2145f1

Added spaces for style

4707b66

bjorn requested changes Mar 21, 2017

View reviewed changes

Last changes!

7970270

bjorn merged commit cdc71f4 into mapeditor:master Mar 21, 2017

		preview->merge(op.pos - bounds.topLeft(), op.stamp);
		if(op.isCopy)

		@@ -250,9 +250,17 @@ void StampBrush::endCapture()

		// Intersect with the layer and translate to layer coordinates
		QRect captured = capturedArea();

Stamp brush now works with the stagger in hex mode as expected #1496

Stamp brush now works with the stagger in hex mode as expected #1496

Conversation

Bdtrotte commented Mar 16, 2017

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

bjorn commented Mar 20, 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

bjorn commented Mar 20, 2017

Bdtrotte commented Mar 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

bjorn Mar 21, 2017 • edited

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 Mar 21, 2017 • edited

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 Mar 21, 2017

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

Bdtrotte commented Mar 21, 2017

bjorn commented Mar 21, 2017

Bdtrotte commented Mar 21, 2017

bjorn Mar 21, 2017 •

edited

bjorn Mar 21, 2017 •

edited