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

Staggered flipping now works as expected, as well, Hexagonal rotation now works in 60 degree intervals. #1515

Merged
merged 20 commits into from Apr 5, 2017

Conversation

@Bdtrotte
Copy link
Contributor

commented Mar 29, 2017

Addresses #1476. Adds a hex class, which stores info of the cube coordinates of a hex tile, and implements methods for rotation, and converting to staggered coordinates.

@Bdtrotte

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2017

I think I've done something wrong, as it's showing commits far beyond the context of this pr. Ill work to fix this...

@Bdtrotte

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2017

Alright, should be up to date now

@bjorn

This comment has been minimized.

Copy link
Owner

commented Mar 29, 2017

Alright, should be up to date now

Well, you merged my branch with yours, but you should rather rebase your work and in the process you can remove all the commits that have already been merged (and squashed as cdc71f4).

@bjorn
Copy link
Owner

left a comment

I've focused on coding style in this review since I don't have time to try it out right now. The change looks promising!

As before, I have only mentioned each coding style problem once, but I found many cases where the code was in a different style. Please check your patch against each rule. And please try to remember them! :-)

Btw, some rules may be arbitrary, but if I may highlight one in particular, the spaces after commas (and around operators) one:

Hex newCenter(newWidth/2,newHeight/2,staggerIndex,staggerAxis);

vs.

Hex newCenter(newWidth / 2, newHeight / 2, staggerIndex, staggerAxis);

The second is simply more readable, isn't it? It's a good habit to use everywhere, unless for some inexplicable reason you come to work on a project that explicitly tells you to do otherwise.

* This file is part of Tiled.
*
* This program is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License as published by the Free

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 29, 2017

Owner

Note that the libtiled part is BSD licensed. Please copy the header from another libtiled source file.

Hex::Hex(int x, int y, int z):
mX(x),
mY(y),
mZ(z) { }

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 29, 2017

Owner

Coding style: Please put the { } on its own line.

Map::StaggerIndex staggerIndex,
Map::StaggerAxis staggerAxis)
{
Hex(point.x(),point.y(),staggerIndex,staggerAxis);

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 29, 2017

Owner

Coding style: Please put spaces after commas.

Coding wise, I think I know what you're trying to do here, but C++ doesn't work like that. You're just creating a temporary Hex instance on the stack that will be gone when the constructor is done. You could implement this as follows though:

*this = Hex(point.x(), point.y(), staggerIndex, staggerAxis);

But instead you could also define a Hex::setStaggered(int col, int row, ...) method and call that from these constructors.

This comment has been minimized.

Copy link
@Bdtrotte

Bdtrotte Mar 30, 2017

Author Contributor

Thanks for this. I was thinking logically what I was doing wasn't quite right, but thought I would get an error if it was wrong. But I see now exactly what it was doing.

instead you could also define a Hex::setStaggered(int col, int row, ...)

Good advice! Implementing this way.

}

QPoint Hex::toStagger(Map::StaggerIndex staggerIndex,
Map::StaggerAxis staggerAxis)

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 29, 2017

Owner

Should be marked const and I'd call it toStaggered.


Hex::Hex(int col, int row,
Map::StaggerIndex staggerIndex,
Map::StaggerAxis staggerAxis)

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 29, 2017

Owner

Coding style: please align wrapped lines with the (

Hex operator *(const float& f);
Hex operator /(const float& f);
void operator +=(const Hex& h);
void operator -=(const Hex& h);

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 29, 2017

Owner

Although rarely used, these operators usually return a Hex& (return *this;).

QRect filledRect = region().boundingRect();

if(staggerAxis == Map::StaggerY)
{

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 29, 2017

Owner

Coding style: { on the same line and space after if.

@@ -266,7 +268,7 @@ class TILEDSHARED_EXPORT TileLayer : public Layer
* are rotated within the layer, and the tiles themselves are rotated. The
* dimensions of the tile layer are swapped.
*/
void rotateHexagonal(RotateDirection direction);
void rotateHexagonal(RotateDirection direction, Tiled::Map::StaggerIndex staggerIndex, Tiled::Map::StaggerAxis staggerAxis);

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 29, 2017

Owner

You're in the Tiled namespace here, so you don't need the Tiled:: qualification.

if (staggerAxis == Map::StaggerY)
{
if ((direction == FlipVertically && !(layer->height() & 1)) || direction == FlipHorizontally)
variation.map->setStaggerIndex(static_cast<Map::StaggerIndex>((staggerIndex + 1) & 1));

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 29, 2017

Owner

A little shorter would be:

variation.map->setStaggerIndex(static_cast<Map::StaggerIndex>(1 - staggerIndex));

Though for readability I might try to use:

static Map::StaggerIndex invert(Map::StaggerIndex staggerIndex)
{
    return static_cast<Map::StaggerIndex>(1 - staggerIndex);
}
Map::StaggerIndex staggerIndex = variation.map->staggerIndex();
Map::StaggerAxis staggerAxis = variation.map->staggerAxis();

if (variation.map->orientation() == Map::Staggered || variation.map->orientation() == Map::Hexagonal) {

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 29, 2017

Owner

Remember that we added map->isStaggered().

This comment has been minimized.

Copy link
@Bdtrotte

Bdtrotte Mar 30, 2017

Author Contributor

I forgot to use the function I added... Resolved now!

@Bdtrotte

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2017

Well, you merged my branch with yours, but you should rather rebase your work and in the process you can remove all the commits that have already been merged (and squashed as cdc71f4).

Would it be best to close this pr and start a new one with everything cleaned up? Or just continue working in this one?

@Bdtrotte

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2017

Updated all items from the review. I absolutely appreciate good style, just have to get used to being consistently good! As well constantly learning a lot about c++, so hopefully less coding errors as we go.

@bjorn

This comment has been minimized.

Copy link
Owner

commented Mar 30, 2017

Would it be best to close this pr and start a new one with everything cleaned up? Or just continue working in this one?

There is no reason to close this PR, please keep updating this one.

Bdtrotte added 2 commits Mar 30, 2017
@Bdtrotte

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2017

I believe I have fixed all the style issues. Is there anything else you think should be changed within the code?

@bjorn
Copy link
Owner

left a comment

It seems to work well and it's awesome!

I've added a few more comments on the implementation.

@@ -31,6 +31,8 @@

#include "tiled_global.h"

#include "hex.h"

This comment has been minimized.

Copy link
@bjorn

bjorn Apr 4, 2017

Owner

You don't need the hex.h include here.


if (variation.map->isStaggered()) {
if (staggerAxis == Map::StaggerY)
{

This comment has been minimized.

Copy link
@bjorn

bjorn Apr 4, 2017

Owner

Coding style: { should be on the same line.

@@ -232,6 +238,22 @@ TileStamp TileStamp::flipped(FlipDirection direction) const

for (const TileStampVariation &variation : flipped.variations()) {
TileLayer *layer = variation.tileLayer();

Map::StaggerIndex staggerIndex = variation.map->staggerIndex();
Map::StaggerAxis staggerAxis = variation.map->staggerAxis();

This comment has been minimized.

Copy link
@bjorn

bjorn Apr 4, 2017

Owner

You can move these variables inside the if (variation.map->isStaggered()) { condition below.


if (staggerAxis == Map::StaggerY) {
if (filledRect.y() & 1)
filledRect.adjust(0, -1, 0, 0);

This comment has been minimized.

Copy link
@bjorn

bjorn Apr 4, 2017

Owner

Would it be possible to change the stagger index instead of keeping an empty row of data around?

This comment has been minimized.

Copy link
@Bdtrotte

Bdtrotte Apr 4, 2017

Author Contributor

This was admittedly a bit of a lazy solution. Though I believe we don't have the inherent ability to change the stagger index from a TileLayer. The solution could be to omit the staggerIndex and staggerAxis parameters, and in place pass the map reference. I was thinking it would be preferable to keep them separate, but I'll go ahead and implement that way if you think I'd be better.

{
int newWidth = mHeight;
int newHeight = mWidth;
Hex botRight(mWidth, mHeight, staggerIndex, staggerAxis);

This comment has been minimized.

Copy link
@bjorn

bjorn Apr 4, 2017

Owner

Naming style: Please just use bottomRight.

topRight.rotate(RotateRight);

int newWidth = topRight.toStaggered(staggerIndex, staggerAxis).x() * 2 + 2;
int newHeight = botRight.toStaggered(staggerIndex, staggerAxis).y() * 2 + 2;

This comment has been minimized.

Copy link
@bjorn

bjorn Apr 4, 2017

Owner

It is not immediately obvious to me why this works (like, why the new area is both minimal but also large enough). Could you share a few words about it?

This comment has been minimized.

Copy link
@Bdtrotte

Bdtrotte Apr 4, 2017

Author Contributor

If a rectangle is rotated 60 degrees clockwise around the center, then the top right corner will be the furthest right point, the bottom left will be the furthest left by the same amount, and top left will be the highest, bottom right the lowest. Similar logic applies for anti-clockwise rotation, and the sizes will be the same, so one direction can be chosen, regardless of intended rotation direction. So we take the two corners, transform them around the center, rotate, and take the respective largest axis. This value is of the center, so it is doubled to get the maximum new width/height. The two is added because of potential integer rounding in regards to the center.

Of course a problem is that sometimes the corners will be empty, so the enlargement is unnecessary. I don't believe there is an easy way to account for this, thus I enlarge, then trim it down later.

In short: This process produces the smallest rectangle which contains the original rotated by 60 degrees.

Hex operator + (Hex h) const;
Hex operator -(Hex h) const;
Hex operator *(float f) const;
Hex operator /(float f) const;

This comment has been minimized.

Copy link
@bjorn

bjorn Apr 4, 2017

Owner

You're not actually using the * and / operators and using them would risk violating the rule that hex cube coordinates always adhere to x + y + z = 0 (due to integer truncation). I would suggest you leave those out.

Map::StaggerIndex staggerIndex,
Map::StaggerAxis staggerAxis);

Hex operator + (Hex h) const;

This comment has been minimized.

Copy link
@bjorn

bjorn Apr 4, 2017

Owner

Stray space after the + (it'd be fine, but you don't have it on any of the other operators).


namespace Tiled {

class Hex

This comment has been minimized.

Copy link
@bjorn

bjorn Apr 4, 2017

Owner

Please add a comment like:

/**
 * Represents a hex position in cube coordinates.
 */
@bjorn
Copy link
Owner

left a comment

Actually you had convinced me that it was not a good idea to change the stagger index, because indeed that's on the map and not the layer, so we're just lucky it works here because at the moment the rotation is only invoked from a map that has only this one tile layer.

But since for our current use-case this works fine and is nicer than adding an empty row, let's leave it for now.

I noticed only one now outdated comment which is still to be updated, then this patch can be merged!

@@ -266,7 +267,7 @@ class TILEDSHARED_EXPORT TileLayer : public Layer
* are rotated within the layer, and the tiles themselves are rotated. The
* dimensions of the tile layer are swapped.

This comment has been minimized.

Copy link
@bjorn

bjorn Apr 5, 2017

Owner

Please still adjust this comment, and make a note of the map parameter and that the stagger index may be changed.

@bjorn bjorn merged commit c170b55 into bjorn:master Apr 5, 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 Apr 5, 2017

Many thanks for this great enhancement!

@Bdtrotte Bdtrotte deleted the Bdtrotte:hexRotate branch Apr 5, 2017

@bjorn

This comment has been minimized.

Copy link
Owner

commented Apr 5, 2017

@Bdtrotte Please read up on Git, in particular how to update your fork without deleting and re-creating it. :-)

@Bdtrotte

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2017

Of course! Will do. It was just pretty messy, so what quicker way to clean up..!

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