Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from Mar 21, 2017

Conversation

@Bdtrotte
Copy link
Contributor

commented Mar 16, 2017

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

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)

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Mar 17, 2017

Contributor

Code style: Add a space between if and (

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Mar 17, 2017

Contributor

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

if (mapDocument()->map()->orientation() == 4 && stamp->height() > 1) {
{
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.

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Mar 17, 2017

Contributor

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

if (rowOfTop < 0)
    rowOfTop *= -1;
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)

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Mar 17, 2017

Contributor

Space between if and (


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

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Mar 17, 2017

Contributor

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

This comment has been minimized.

Copy link
@Bdtrotte

Bdtrotte Mar 17, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Mar 17, 2017

Contributor

Oh, I got confused by that commend :/

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

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Mar 17, 2017

Contributor

Space between if and (

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Mar 17, 2017

Contributor

Move the curly bracket here too.

if ((mCaptureTopCorner+1)%2 == mapDocument()->map()->staggerIndex()) {
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())

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Mar 17, 2017

Contributor

Here too.

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

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Mar 17, 2017

Contributor

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

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

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Mar 17, 2017

Contributor

Here.

shiftMode = -1;
}

if(shiftMode != -1)

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Mar 17, 2017

Contributor

Here too.

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)

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Mar 17, 2017

Contributor

Add a space between for and ( too :)

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Mar 17, 2017

Contributor

Here as well.

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

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Mar 17, 2017

Contributor

Here as well.

This comment has been minimized.

Copy link
@Bdtrotte

Bdtrotte Mar 17, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Mar 17, 2017

Contributor

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

This comment has been minimized.

Copy link
@ketanhwr

ketanhwr Mar 17, 2017

Contributor

Btw, move the curly brackets up here as well.

@bjorn

This comment has been minimized.

Copy link
Owner

commented Mar 20, 2017

@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
Copy link
Owner

left a comment

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.

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

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

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)
 ^~~~~~~~~~
@@ -250,6 +251,8 @@ void StampBrush::endCapture()

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

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

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.

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) {

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

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.

//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)
{

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

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

@@ -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());

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

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.

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

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

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.

}

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

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

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());
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)

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

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

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

if (mCaptureTopCorner == -1)

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 20, 2017

Owner

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

This comment has been minimized.

Copy link
Owner

commented Mar 20, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2017

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
Copy link
Owner

left a comment

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.

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());

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

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

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

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

Coding style: please put a space after if

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) {

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

Coding style: space after for

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 comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

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

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

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

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.

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

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

QHash<TileLayer*, TileLayer*> shiftedCopies;

...

qDeleteAll(shiftedCopies);

This comment has been minimized.

Copy link
@Bdtrotte

Bdtrotte Mar 21, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

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.

This comment has been minimized.

Copy link
@Bdtrotte

Bdtrotte Mar 21, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

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.

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

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

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.

int rowOfTop = p.y() - stamp->height() / 2;

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

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

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.


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

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

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

This comment has been minimized.

Copy link
@Bdtrotte

Bdtrotte Mar 21, 2017

Author Contributor

I have done this. Good idea.

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

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

I believe this whole condition could be written as:

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

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));

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

Coding style: please put spaces after commas.

tileLayer->width(), tileLayer->height());

//gets if the relative stagger should be the same as the base layer
int stagIndexOffSet;

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

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

Bdtrotte added 2 commits Mar 21, 2017
@Bdtrotte

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2017

Made the latest batch of requested improvements and style updates!


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

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

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.

This comment has been minimized.

Copy link
@Bdtrotte

Bdtrotte Mar 21, 2017

Author Contributor

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?

This comment has been minimized.

Copy link
@Bdtrotte

Bdtrotte Mar 21, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

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;

This comment has been minimized.

Copy link
@Bdtrotte

Bdtrotte Mar 21, 2017

Author Contributor

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

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

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

Coding style: please remove these brackets again now.

bool topIsOdd = (p.y() - stamp->height() / 2) & 1;

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

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

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

Bdtrotte added 2 commits Mar 21, 2017
@bjorn
Copy link
Owner

left a comment

Last request for changes, I'm positive!

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

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

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

No need to add this line.

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

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 21, 2017

Owner

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

@Bdtrotte

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2017

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

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bjorn

This comment has been minimized.

Copy link
Owner

commented Mar 21, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2017

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 :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.