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

Support rotate hexagonal tiles 60 degrees #1447

Closed
wants to merge 9 commits into from

Conversation

Projects
None yet
2 participants
@nicolaichuk
Copy link
Contributor

commented Feb 16, 2017

@nicolaichuk nicolaichuk changed the title support Rotate hexagonal tiles 60 degrees Support rotate hexagonal tiles 60 degrees Feb 16, 2017

@bjorn
Copy link
Owner

left a comment

I've tried it out and I think it works well! I did have a few comments on the implementation, which I think could be easier to understand.

Thanks for working on this nice feature!

}
}
const Map* const globalMap = map();
const Map::Orientation currentOrientation = (globalMap ? globalMap->orientation() : Map::Unknown);

This comment has been minimized.

Copy link
@bjorn

bjorn Feb 21, 2017

Owner

Since we can't rely on the tile layer being part of a map in general, and because of the duplicated implementation, I think it would make sense to split this up into flip and flipHexagonal, rotate and rotateHexagonal. We can decide which version to call in tilestamp.cpp, where a null check on the map is not needed.

@@ -38,6 +38,7 @@ using namespace Tiled;
const int FlippedHorizontallyFlag = 0x80000000;
const int FlippedVerticallyFlag = 0x40000000;
const int FlippedAntiDiagonallyFlag = 0x20000000;
const int FlippedLeftHexAntiDiagonallyFlag = 0x10000000;

This comment has been minimized.

Copy link
@bjorn

bjorn Feb 21, 2017

Owner

I don't entirely understand the name or behavior of this flag.

FlippedAntiDiagonally meant basically swapping the x/y axis, which was equivalent to flipping vertically and then rotating by 90 degrees. In hexagonal mode, this has changed to mean flipping vertically and then rotating by 60 degrees clockwise (which can no longer be described as an anti-diagonal flip).

FlippedLeftHexAntiDiagonallyFlag seems to mean to flip vertically and then rotating by 60 degrees counter-clockwise (which I guess is where the Left comes from).

What I'm wondering about is since these flags are not exactly diagonal flips, whether it is useful in any way to still implement them like that. Wouldn't it be simpler if we instead had:

const int Rotated60Flag = 0x20000000;   // a duplicate of FlippedAntiDiagonallyFlag for readability
const int Rotated120Flag = 0x10000000;

This way things should be easier to understand:

  • 60: Rotated60
  • 120: Rotated120
  • 180: FlippedHorizontally | FlippedVertically (or Rotated60 | Rotated120 of course)
  • 240: Rotated60 | FlippedHorizontally | FlippedVertically
  • 300: Rotated120 | FlippedHorizontally | FlippedVertically

I hope you don't mind to adjust the patch accordingly.

This comment has been minimized.

Copy link
@bjorn

bjorn Feb 21, 2017

Owner

Just noting that this Rotated60Flag does not need to exist in this file. Since it is only for readability, it can be an alternative getter/setter as bool Cell::rotated60().

@@ -106,10 +106,11 @@ static bool hasOpenGLEngine(const QPainter *painter)
type == QPaintEngine::OpenGL2);
}

CellRenderer::CellRenderer(QPainter *painter)
CellRenderer::CellRenderer(QPainter *painter, const bool isHexagonal)

This comment has been minimized.

Copy link
@bjorn

bjorn Feb 21, 2017

Owner

Please make this isHexagonal into an enum like CellType with default value OrthogonalCells and alternative value HexagonalCells.

rotation = (cell.flippedAntiDiagonally() ? 90 : 0);
}

if (rotation != 0) {

This comment has been minimized.

Copy link
@bjorn

bjorn Feb 21, 2017

Owner

Here, please keep the if (cell.flippedAntiDiagonally()) { check and simplify the hexagonal case first by having something like:

if (mCellType == HexagonalCells) {
    if (cell.rotated60())
        fragment.rotation += 60;
    if (cell.rotated120())
        fragment.rotation += 120;
} else if (cell.flippedAntiDiagonally()) {
    ...
}
@nicolaichuk

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2017

@bjorn:
Please review and merge to main branch.

@bjorn
Copy link
Owner

left a comment

See comments.

@@ -39,6 +39,9 @@ const int FlippedHorizontallyFlag = 0x80000000;
const int FlippedVerticallyFlag = 0x40000000;
const int FlippedAntiDiagonallyFlag = 0x20000000;

const int RotatedHexagonal60Flag = 0x20000000;

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 6, 2017

Owner

I had noted earlier that this variable didn't actually need to exist in this file, since it is just an alias for FlippedAntiDiagonallyFlag. As such it's also only causing duplicated processing below. Only RotatedHexagonal120Flag needs to be added here.

@@ -111,6 +125,9 @@ class Cell
bool _flippedHorizontally;
bool _flippedVertically;
bool _flippedAntiDiagonally;

bool _rotatedHexagonal60;

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 6, 2017

Owner

Since this is just an alias for _flippedAntiDiagonally, please don't add it as a member. Instead, you can use _flippedAntiDiagonally in rotatedHexagonal60 and setRotatedHexagonal60. It may be a little confusing, but on the other hand it will make it more clear that the same flag is used here.

* Rotate this tile layer by 90 degrees left or right. The tile positions
* are rotated within the layer, and the tiles themselves are rotated. The
* dimensions of the tile layer are swapped.
*/
void rotate(RotateDirection direction);

/**
* Hexagonal rotate this tile layer by 90 degrees left or right. The tile positions

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 6, 2017

Owner

Documentation should mention 60 degrees here.

layer->flip(direction);
} else {
layer->flipHexagonal(direction);
}

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 6, 2017

Owner

Coding style: please leave out the superfluous { and } for one-line bodies.

}
if (cell.rotatedHexagonal120()) {
fragment.rotation += 120;
}

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 6, 2017

Owner

Coding style: please leave out the superfluous { and } for one-line bodies.

HexagonalCells
};

explicit CellRenderer(QPainter *painter, const CellType cellType = OrthogonalCells);

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 6, 2017

Owner

Please leave out the const here, since that has no meaning for arguments passed by value.

@@ -232,7 +232,11 @@ TileStamp TileStamp::flipped(FlipDirection direction) const

for (const TileStampVariation &variation : flipped.variations()) {
TileLayer *layer = variation.tileLayer();
layer->flip(direction);
if (variation.map->orientation() != Map::Hexagonal) {

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 6, 2017

Owner

I'd prefer if == was used here, and the bodies swapped of course.

* Rotate this tile layer by 90 degrees left or right. The tile positions
* are rotated within the layer, and the tiles themselves are rotated. The
* dimensions of the tile layer are swapped.
*/
void rotate(RotateDirection direction);

/**
* Hexagonal rotate this tile layer by 90 degrees left or right. The tile positions
* 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 Mar 6, 2017

Owner

I just realized that swapping the dimensions of the layer really makes no sense at all, and indeed the rotation of multiple tiles by 60 degrees doesn't really do anything useful. Essentially, the layer is rotated by 90 degrees while the tile graphics are getting rotated by 60 degrees. Is this something you have considered, and do you see a good solution to this? Of course, it doesn't help that trying to paint with a bigger stamp on a hex map doesn't work properly half the time due to the staggering.

This comment has been minimized.

Copy link
@nicolaichuk

nicolaichuk Mar 6, 2017

Author Contributor

May be this documentation helps http://www.redblobgames.com/grids/hexagons/#rotation

A bird in the hand is worth two in the bush )

Rotate group tile is good feature, but now it rather rotate a single tile for purposes in my application.

This comment has been minimized.

Copy link
@bjorn

bjorn Mar 6, 2017

Owner

Ok, so we can finish the basic feature first for single-tiles, and I can keep an issue open for making the rotation of groups work properly. That documentation definitely helps.

@nicolaichuk

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2017

@bjorn:
Please review and merge to main branch.

@bjorn

This comment has been minimized.

Copy link
Owner

commented Mar 7, 2017

Merged as fae6ebd, thanks!

I've left out the Windows related changes to README.md. While I agree Qbs instructions should be added there, I didn't want to include them in the patch.

@bjorn bjorn closed this Mar 7, 2017

@nicolaichuk

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2017

@bjorn, thanks.

When will you plane create next release?

@bjorn

This comment has been minimized.

Copy link
Owner

commented Mar 7, 2017

@nicolaichuk That would be Tiled 1.0, which I hope will be out in April or May. In the meantime though, this feature will be in the development snapshots later today, which are also easy to install on any platform.

@nicolaichuk

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2017

@bjorn, Ok, thanks.

@bjorn

This comment has been minimized.

Copy link
Owner

commented Mar 7, 2017

@nicolaichuk If you happen to find any time to improve hex rotation of multiple tiles that would be appreciated. If not, it'll be on my list of things to do before the release.

@nicolaichuk

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2017

@bjorn, I will now switch to other tasks for my project.
If I have free time, I'll see how I can help.

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