Feature 2833 (rotated Wang tiles)

[cpetig]

Implementation of rotated Wang tiles. Introduces new icons for rotate-on-demand and rotate-for-variation. Implements saving and loading these properties to tile sets. Implements rotated virtual Wang tile generation on loading.
Closes #2833

[cpetig]

My current plan for supporting direct use after Wang tile edit would be to regenerate the virtual Wang tile entries whenever the tileset is saved to a file. @bjorn Would you require this before merging to master?

[bjorn]

@cpetig It should work without needing to save the tileset, but I have not yet gotten around to looking at your approach in detail and to consider alternatives. I'll look into this tomorrow.

[cpetig]

I know that derived (rotated) Wang variants are not recalculated whenever a Wang tile is edited. I thought that recalculating on save might be a reasonable compromise (only need to change a single function) instead of having to recalculate at every change. Currently the derived set is only calculated at Tileset load time (which will probably confuse users)

[bjorn]

I've replaced the generated WangTiles approach by iterating the variations on-demand and applying a small penalty to variations in order to keep the "rotate-for-variation" optional. It's slower (even twice as slow at the moment), but I still prefer it since it makes other things easier. Maybe we can still optimize the setting up of the variations a bit later.

There's a few things still to be done before this can be merged:

• When the properties are changed, this should be done through undo commands.
• I'd prefer the options on the WangSet rather than on the Tileset, but on Discord I've had feedback that this could be useful as a per-tile flag as well (whether it can be rotated / flipped).
• Some people will need to be able to toggle these options separately:
  • Can tile be flipped vertically?
  • Can tile be flipped horizontally?
  • Can tile be rotated?
• Since this feels rather WangSet-specific, maybe the above per-tile options could be stored on the WangTile class. The downside of storing it there is that it will need some new UI approach to editing those options.

I'll see if I can help with these things next week, but in the meantime any work or additional feedback is really appreciated!

[cpetig]

I still have trouble imagining a tile set where different rules apply to different Wang sets (i.e. I believe that if any image in a set supports vertical flipping (birds eye view typically) all images would allow this). And since the system prefers existing flipped/rotated-WangId tiles over flipping/rotating tiles I see no benefit of making the decision per Wang Set or even per tile.
On another note I assume that permission to flip both horizontally and vertically (180°) would imply permission for 90° rotation (anti-diagonal flipping), so I assume it wouldn't need a separate third property. Of course this only applies to quadratic tiles, as with hexagonal or rectangular tiles 90° rotation would be invalid.

[cpetig]

Eishiya convinced me that there is at least one user for per tile settings, see https://discordapp.com/channels/524610627545595904/524610627545595906/777201497770819635 . Also it seems that indeed 180° doesn't imply 90° is valid.
I took a quick look and will make these flags part of WangTile (+inherit) and WangSet (+randomize).

[cpetig]

@bjorn Is there a specific reason for defining trivial one statement get functions at the end of the header instead of embedding them inside the class?
Oh, I see it is done differently in WangTile and WangSet. So I assume there is no specific reason. 👿

[cpetig]

I try to figure out whether the rotation permission is a property of a WangTile or a Tile? I assume a WangTile corresponds to an entity which is edited in the Wang editor for one tile, so this would be the correct place to store the information. @bjorn Could you please confirm this understanding as correct?

[bjorn]

@bjorn Is there a specific reason for defining trivial one statement get functions at the end of the header instead of embedding them inside the class?
Oh, I see it is done differently in WangTile and WangSet. So I assume there is no specific reason.

It just means I'm not feeling overly attached to having them in one place or another, since both have their advantages. The reason to put them outside of the class is simply to keep the class definition more readable.

I try to figure out whether the rotation permission is a property of a WangTile or a Tile? I assume a WangTile corresponds to an entity which is edited in the Wang editor for one tile, so this would be the correct place to store the information. @bjorn Could you please confirm this understanding as correct?

Yes, though when we're doing this per-WangTile there would need to be some way in the tileset editor to set up which tiles can be flipped in which way. Not sure what's the best approach for now.

I left some inline comments, though I saw you just pushed another change, which is not taken into account yet.

[cpetig]

Dear @bjorn , I completed the code for per WangSet and per WangTile independent flip permissions. But to me it feels that (especially after implementing the undo/redo part per WangTile) overriding flip permissions for a WangTile should be a per Tile property.
Do you agree? I will likely need to recharge my motivation to rewrite half of this patch, though (motivation gathering will take some hours).

[bjorn]

@cpetig Well, the way it's implemented now it indeed makes no sense for this stuff to be stored in the WangTile instead of the Tile. But in any case, I think you've done a great job adjusting this patch to the recent discussions. Let's take a short break. I will have time to take a detailed look into the patch again on Tuesday.

[cpetig]

@bjorn I tested the per tileset settings and would say that this could be ready (if necessary) for the next release if you hide the per tile flip properties (as they have no implemention yet and would only confuse users). The document/model/changedata classes will probably need some cleanup (especially when it comes to per tile properties).

[bjorn]

@cpetig Thanks for your hard work on this change and for your persistence as we try different approaches! As said, it may take until Tuesday for me to be able to get back to this change, but it's great to work together on this and I'm sure we'll manage to get this merged soon now.

[bjorn]

The following remaining things still need to be addressed:

• Code related to overriding of these options at the tile level should be either fixed and be implemented similarly to how they are implemented at the tileset level, or they should be entirely removed for now. I don't have a strong opinion on which way to go.

• The disabled code in ChangeTileWangId::changesOnSetColorCount and ChangeTileWangId::changesOnRemoveColor needs to be re-enabled, similarly to how I just re-enabled the code in AdjustTileMetaData (565645a).

• The options need to also be stored by the JSON format.

• The file format documentation for JSON and TMX needs to be updated.

[cpetig]

FYI: I rebased the branch on the current master for a start.

[cpetig]

I implemented JSON im+export and documented the file format changes (although the meaning of the bitset in the JSON file is not documented. Also I turned off the rotation properties for tiles, so that the incomplete feature is not accessible by/visible for users. I will go to bed now.

PS: I get a warning that Q_FLAGS() is obselete and should be replaced by Q_FLAG() see https://doc.qt.io/qt-5/qobject-obsolete.html#Q_FLAGS

[bjorn]

Thanks @cpetig, I'll finish up the rest. I've left some inline comments but I will resolve those as well.

[bjorn]

Puh, I guess this change is almost ready for merging. I wonder if I'm doing something wrong that makes some changes take such a long time to finish, and to even finish them in a state that leaves more work to be done!

One remaining thing that's nagging me right now, is that the option of whether to allow rotation essentially only allows a single 90-degree clockwise rotation. You need to allow flipping to enable 180 and 270 degree rotations as well. I wanted this flag to say "rotation" rather than "diagonal flip" because I think it's more intuitive and more useful, but I guess if only rotation is selected, it should really also include 180 and 270 degrees. Just need to find a way to work this into recalculateCells without making too much of a mess...

I also wonder a bit what should happen when "Rotate" and "Flip Horizontal" is allowed, but not "Flip Vertical". It makes no sense because Rotate180 + FlipHorizontal = FlipVertical. :S

Any opinions welcome, @cpetig!

[bjorn]

@cpetig Thanks for a nice collaboration!

[cpetig]

@bjorn Same to you! Thank you for programming, maintaining and offering this nice tool.