Fix tilemap layer copy/paste
Addresses copy/paste of tilemap layers from the timeline as well as copying a portion with the selection tool (and site.tilemapMode() == TilemapMode::Tiles). Fix #4872 in the immediate sense, but there's some loose ends that I think could be tackled in this patch too (see notes below). Making this PR a draft as I need feedback on some of these points.
Notes: When copying from the timeline, a reference to the source tileset could be used instead, but that causes two problems:
- Modifications to the destination could affect the source.
- Pasting from one document to another "works" with references so to speak, but segfaults abound.
The more I thought about it, the more involved the logic got, so a hard copy is always made for now. An option could be added to control this behavior, but we would have to consider these problems.
Then, for copying with the selection tool, some matching and remapping is necessary when the destination and source are different. The code for the operation is pretty simple, but it requires adding missing tiles to the destination through cmd::AddTile, so I moved it to PixelsMovement to make it all a single transaction/undo. There's a few things about this:
- I'm not a 100% sure if the way I did it is correct: I wanted to call the function I added (
PixelsMovement::remapTilesForPaste()) fromEditor::pasteImage()to make sure it's only ever used in that specific case, but to do so I had to make it public, which I feel is a bit confusing.- An alternative would be calling the function from the
PixelsMovementconstructor, that works too, it's just that it'd be nice to check that the operation is indeed a paste. I could do something likestrcmp(operationName, "Paste")here but that doesn't quite feel right either.
- An alternative would be calling the function from the
- The call to
PixelsMovement::remapTilesForPaste()should be skipped when source and destination layer are the same, but I can't perform the check as no reference to the source layer is stored in this situation, and comparing theTileset*s won't work either as the source tileset is itself a copy. - The implementation may actually need to be somewhere else to allow reuse for the non-interactive version, I'm just not sure where to put it. Maybe we can also reuse some other code from
PixelsMovementfor this too, like themerge_tilemaps()?
Lastly, there was no check for pasting between two tilemap layers with a different tile size, so for now I added a quick exception to catch that. Pasting in this case anyway doesn't seem to cause a crash (far as I tested), but it can give weird results when the destination layer has a smaller tile width or height.
I've been looking into this PR and found some confusing behaviors:
- Copying and pasting a selection of a tilemap into another tilemap layer (with the same tile size) won't do anything if pixel mode is selected. I believe it should paste the tilemap independently of which mode is selected.
- Duplicating a tilemap layer creates a copy of the tilemap with a reference to the same tileset as the source tilemap. But copying & pasting a tilemap creates a copy of the tilemap with its own tileset (which is a copy of source's tileset). Not sure if these two behaviors are intended, but it could be a bit confusing I guess. The nice side of this is that there are two ways to create a copy of a tilemap (with a tileset reference or with a tileset copy). So I'm not sure about how the app should behave here regarding tilesets...as is? always using references? always using hard copies? However, if we go for using references, it should be only for copying & pasting in the same doc, if the pasting takes place in a different doc, a hard copy of the tileset would be better for those cases IMO. This is related to
Notes: When copying from the timeline, a reference to the source tileset could be used instead, but that causes two problems:
- Modifications to the destination could affect the source.
- Pasting from one document to another "works" with references so to speak, but segfaults abound. The more I thought about it, the more involved the logic got, so a hard copy is always made for now. An option could be added to control this behavior, but we would have to consider these problems.
So, I think there is no doubt that pasting into different doc must do a hard copy of source's tileset.
-
Copy & pasting a selection from a tilemap into another tilemap doesn't behave the same as with regular layers for the following case: a. Copy a selection of tiles from tilemap "A". b. Paste the selection into tilemap "B". c. Click on some other layer in the timeline. d. You will see that the pasted tiles disappear. If you try the same with regular layers, you will see that in step "d" the pasted pixels don't disappear. I would expect the same behavior for the tilemaps case.
-
Copy & pasting a selection from a tilemap into another tilemap (empty and with bigger tile size) throws the exception you added, which is fine I think, but after closing the console window, the selection looks like this:
Then if try to move the selection, the following assert fails:
But if instead of moving the selection right away after closing the console window, I first select the source layer in the timeline and then try to move the selection, this other assert fails:
It would be nice to fix those issues as well.
Okay, this was just all about behavior, I will review the code now and leave comments on the review itself.