Support rotate hexagonal tiles 60 degrees #1447

nicolaichuk · 2017-02-16T15:17:16Z

http://discourse.mapeditor.org/t/rotate-hexagonal-tiles-60-degrees/1546/5

Please review and merge to main branch.

http://discourse.mapeditor.org/t/rotate-hexagonal-tiles-60-degrees/1546/5

bjorn

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!

bjorn · 2017-02-21T14:28:35Z

src/libtiled/tilelayer.cpp

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


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.

bjorn · 2017-02-21T14:49:43Z

src/libtiled/gidmapper.cpp

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


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.

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

bjorn · 2017-02-21T14:52:01Z

src/libtiled/maprenderer.cpp

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

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


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

bjorn · 2017-02-21T14:58:06Z

src/libtiled/maprenderer.cpp

+        rotation = (cell.flippedAntiDiagonally() ? 90 : 0);
+    }
+
+    if (rotation != 0) {


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 · 2017-03-04T00:52:22Z

@bjorn:
Please review and merge to main branch.

bjorn

See comments.

bjorn · 2017-03-06T10:50:01Z

src/libtiled/gidmapper.cpp

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

+const int RotatedHexagonal60Flag    = 0x20000000;


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.

bjorn · 2017-03-06T10:52:11Z

src/libtiled/tilelayer.h

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


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.

bjorn · 2017-03-06T10:52:47Z

src/libtiled/tilelayer.h

     * 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


Documentation should mention 60 degrees here.

bjorn · 2017-03-06T10:54:08Z

src/tiled/tilestamp.cpp

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


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

bjorn · 2017-03-06T10:54:26Z

src/libtiled/maprenderer.cpp

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


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

bjorn · 2017-03-06T10:56:38Z

src/libtiled/maprenderer.h

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


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

bjorn · 2017-03-06T10:58:29Z

src/tiled/tilestamp.cpp

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


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

bjorn · 2017-03-06T11:06:20Z

src/libtiled/tilelayer.h

     * 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.


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.

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.

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 · 2017-03-06T16:02:28Z

@bjorn:
Please review and merge to main branch.

See http://discourse.mapeditor.org/t/rotate-hexagonal-tiles-60-degrees/1546/5

bjorn · 2017-03-07T09:42:11Z

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.

nicolaichuk · 2017-03-07T11:21:35Z

@bjorn, thanks.

When will you plane create next release?

bjorn · 2017-03-07T13:36:07Z

@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 · 2017-03-07T13:41:18Z

@bjorn, Ok, thanks.

bjorn · 2017-03-07T13:46:15Z

@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 · 2017-03-07T13:51:08Z

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

support Rotate hexagonal tiles 60 degrees

1d181fd

http://discourse.mapeditor.org/t/rotate-hexagonal-tiles-60-degrees/1546/5

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

test_hexagonal_tile

8d1ebb5

nicolaichuk mentioned this pull request Feb 20, 2017

Support rotate hexagonal tiles 60 degrees #1450

Closed

bjorn self-assigned this Feb 21, 2017

bjorn requested changes Feb 21, 2017

View reviewed changes

nicolaichuk added 4 commits February 22, 2017 11:22

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

8d93b33

Merge remote-tracking branch 'remotes/bjorn/master'

c56a634

add describe build for windows in README.md

d981fd2

Apply code-review notes for PR mapeditor#1447

1de648f

change examples

9313dbc

bjorn requested changes Mar 6, 2017

View reviewed changes

nicolaichuk added 2 commits March 6, 2017 18:37

Merge remote-tracking branch 'remotes/bjorn/master'

f5af112

apply code review mapeditor#1447

663e1d5

bjorn pushed a commit that referenced this pull request Mar 7, 2017

Support 60 degree rotation for hexagonal maps (#1447)

fae6ebd

See http://discourse.mapeditor.org/t/rotate-hexagonal-tiles-60-degrees/1546/5

bjorn closed this Mar 7, 2017

bjorn mentioned this pull request Mar 7, 2017

Rotating of hexagonal tile stamps #1476

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support rotate hexagonal tiles 60 degrees #1447

Support rotate hexagonal tiles 60 degrees #1447

nicolaichuk commented Feb 16, 2017

bjorn left a comment

bjorn Feb 21, 2017

bjorn Feb 21, 2017 •

edited

Loading

bjorn Feb 21, 2017

bjorn Feb 21, 2017

bjorn Feb 21, 2017

nicolaichuk commented Mar 4, 2017

bjorn left a comment

bjorn Mar 6, 2017

bjorn Mar 6, 2017

bjorn Mar 6, 2017

bjorn Mar 6, 2017

bjorn Mar 6, 2017

bjorn Mar 6, 2017

bjorn Mar 6, 2017

bjorn Mar 6, 2017

nicolaichuk Mar 6, 2017

bjorn Mar 6, 2017

nicolaichuk commented Mar 6, 2017

bjorn commented Mar 7, 2017

nicolaichuk commented Mar 7, 2017 •

edited

Loading

bjorn commented Mar 7, 2017

nicolaichuk commented Mar 7, 2017

bjorn commented Mar 7, 2017

nicolaichuk commented Mar 7, 2017 •

edited

Loading

Support rotate hexagonal tiles 60 degrees #1447

Support rotate hexagonal tiles 60 degrees #1447

Conversation

nicolaichuk commented Feb 16, 2017

bjorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorn Feb 21, 2017 • edited Loading

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

nicolaichuk commented Mar 4, 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

nicolaichuk commented Mar 6, 2017

bjorn commented Mar 7, 2017

nicolaichuk commented Mar 7, 2017 • edited Loading

bjorn commented Mar 7, 2017

nicolaichuk commented Mar 7, 2017

bjorn commented Mar 7, 2017

nicolaichuk commented Mar 7, 2017 • edited Loading

bjorn Feb 21, 2017 •

edited

Loading

nicolaichuk commented Mar 7, 2017 •

edited

Loading

nicolaichuk commented Mar 7, 2017 •

edited

Loading