Add ability to rearrange tiles in a tileset #2983
Conversation
There was a problem hiding this comment.
This works quite well already! I've left some inline comments on the implementation.
Apart from the comments, there was a little problem with loading old maps, which only include tile elements when the tile was customized in some way. Those tiles will now appear at the start. We need to make sure the order of the tiles is not affected in this case.
I think a possible solution to this would be, to remember the last already found tile index in Tileset::loadFromImage, and to insert tiles after this index in mSortedTileIds instead of appending them (also incrementing this insertion index for each newly created tile).
src/tiled/tilesetdocument.cpp
Outdated
| void TilesetDocument::relocateTile(const Tile *tile, int location) | ||
| { | ||
| mTileset->relocateTile(tile->id(), location); | ||
| /* TODO: is that the right signal? */ |
There was a problem hiding this comment.
I think this is currently the right signal, though it isn't optimal or entirely correct. Ideally we'd send a signal before and after the change, such that the model can call beginResetModel and endResetModel in the appropriate moments. At the same time I'm trying to move away from all the separate signals, instead sending an specialized ChangeEvent in a unified Document::changed signal. I can help with this part of the change.
There was a problem hiding this comment.
I'll leave it as-is. If that's the best signal for this scenario (given how signal handling is currently implemented) I think it's better to make it consistent with the rest of the code. Signal handling improvements can come later in a separate change.
Also addressed two clang warnings in the TilesetModel
* Don't block mouse press events on non-tiles, which makes for strange behavior where the view thinks you're dragging something you previously pressed. * Instead, return nullptr instead of an empty QMimeData instance, to avoid going into a drag operation when dragging from a place with no tile.
|
@jmi2k I've addressed a few of my own comments today and plan to do another review later this week and address the rest of my feedback, hoping to get this merged. I think it'd also make sense to look into supporting multi-selection, because moving a lot of tiles can get tedious. Please feel free to still help polish up this feature. I'll be off for the next 16 hours or so. :-) |
|
Thanks, I'm very sorry I couldn't put some time aside to push this forward. I dismissed multi-selection because it could behave in weird ways. Does it preserve the shape? what does it do when reaching a corner? How does it behave when replacing tiles? I was hoping to get a basic version merged before discussing future improvements |
Now the model needs a TilesetDocument, but that is not a problem because all tilesets have one (I think that didn't used to be the case when this model was written). Now I could also simplify some update connections by moving them to the model.
Otherwise the preservation of the custom tile ordering depends on there being some other reason to write out the tiles, for example the tileset being an image collection.
When tiles based on a tileset image are not re-ordered then only the ones with relevant meta-information are included in the file. Since these tiles are now added to the mTiles list earlier, they would appear at the top in the tileset view. This problem is fixed by inserting each tile at its expected location, instead of appending it.
Bug introduced in aac7b41.
This reverts part of aac7b41. In AdjustTileMetaData we always need to make sure to copy the meta-data either to larger indices or smaller indices, which is why we can't just iterate the list of tiles since it can have a custom order.
bjorn
changed the title
|
Thanks for your help with this change @jmi2k! No worries about not finishing it, I fully understand other things can come up, as also happens for me since I've let #1856 be unresolved for way too long as well. I've added support for multi-selection in a follow-up change (a794403). It does not preserve shapes, it just moves all selected tiles as a sequence to the target location, in what appears to be the order in which they were selected. Edit: though it does behave rather strangely if you drag tiles from both directions to the middle... Let's hope we're still on time to help @hepari with some tileset reorganization. The feature is now available in the artifacts resulting from the Build Packages workflow, and should soon (within a few weeks) be part of a Tiled 1.7 release. |
|
I'm excited to hear that it will be part of the next release. Thank you for
your hard work!
…On Thu, 20 May 2021 at 23:05, Thorbjørn Lindeijer ***@***.***> wrote:
Thanks for your help with this change @jmi2k <https://github.com/jmi2k>!
No worries about not finishing it, I fully understand other things can come
up, as also happens for me since I've let #1856
<#1856> be unresolved for way
too long as well.
I've added support for multi-selection in a follow-up change (a794403
<a794403>).
It does not preserve shapes, it just moves all selected tiles as a sequence
to the target location, in what appears to be the order in which they were
selected.
Let's hope we're still on time to help @hepari <https://github.com/hepari>
with some tileset reorganization. The feature is now available in the
artifacts resulting from the Build Packages
<https://github.com/mapeditor/tiled/actions/workflows/packages.yml>
workflow, and should soon (within a few weeks) be part of a Tiled 1.7
release.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2983 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACPHCGFSBZBEHCXZNBMBY23TOUJL5ANCNFSM4XHOAIIQ>
.
|
WIP: The patch works: I think I've fixed all the bugs I could have introduced and it's ready for code review. However, there are still some details I want to fix before merging. Should fix #1856, or at least the latest requests exposed in that issue.
This PR allows tiles in a tileset to be moved around at will. Previously, the tileset order was fixed and couldn't be changed. That was acceptable for prebuilt tilesets but horrendous for collections of images (after you've added some tile, it was appended to the list couldn't be placed elsewhere, which made working with them a nightmare).