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

Multi-layer copy/paste not working when two layers have the same name #3094

Closed
sebaglen opened this issue Jul 2, 2021 · 14 comments
Closed
Assignees
Labels
bug Broken behavior.
Projects

Comments

@sebaglen
Copy link

sebaglen commented Jul 2, 2021

When layers have the same layer name when multi-layer copy/pasting, objects on top of eachother in the layers with the same layer names ignore one of those layers tile image.

Visual example below.

Bugged behaviour, some images are ignored
Layer names:
Interior 1
Interior 1
Interior 1
bug1

Expected behaviour, no images are ignored
Layer names:
Interior 1
Interior 2
Interior 3
bug2

@bjorn
Copy link
Member

bjorn commented Jul 2, 2021

Right, this is because the layer name is used to identify the target layer when pasting. I could try to handle this case better, but in the end it should probably just lead to a warning, since at the moment there is no other way that would work to uniquely identify a layer. Of course, layers do have unique IDs, but they can be different for each map, while copy/paste also works across different maps.

So some potential actions that could be taken to improve this:

  • Use the layer IDs:

    • When copying, make sure the layer IDs in the clipboard match the IDs of the source layers.
    • When pasting, and when the name doesn't unique identify the layer, check if there's a layer that matches with both its name and its ID. This would make copying work better in case of duplicate layer names within the same map.
  • Give a warning when pasting, and the layer name didn't uniquely identify the target layer, and the ID didn't help to resolve the ambiguity.

  • Big refactor: move to a model where the layer is defined elsewhere with a unique ID, such that this ID can always be used to paste into the right layer even when pasting across maps.

And of course, until it is improved, you'll just have to ensure each layer has a unique name.

@bjorn bjorn added the bug Broken behavior. label Jul 2, 2021
@eishiya
Copy link
Contributor

eishiya commented Jul 14, 2021

This issue's bitten me in the butt a lot, especially in automapping rules where it's common to have many input layers with identical names. I ended up writing a helper script that appends/removes the layer ID to/from the layer name to aid in copy+pasting across many layers.

Rather than layer IDs, I wonder if perhaps Tiled could try a "relative to layer" model. This would be more consistent with how copy+pasting works for single-layers. The bottom-most layer of the layers you have selected in the destination map gets the output from the bottom-most layer of the brush. If multiple layers are selected, then layers above that get matched to selected layers above. If only one layer is selected, then the layers immediately above it get matched. In either case, if there aren't enough layers for all the layers in the brush, new layers are created, using the names of the layers in the brush.

Name-based matching does have its uses though, so I'd still like to see that remain an option you can toggle in a menu somewhere - perhaps even allowing it to be forced for single-layer matches.

@Lobachevskiy
Copy link
Contributor

Agreed, I think relative position of the layers would match more use-cases for this function than name or ID based. Wanted to suggest this exact thing until I saw that eishiya has done it already.

@Macksaur
Copy link

Macksaur commented Dec 18, 2021

Heya this just bit me, it's really frustrating not being able to copy/paste within the same map when building 1) autotile rules or 2) combination set pieces to paste in other maps.

For 1): I often destroy work moving automapping rules around in the same map. It really shouldn't break if they're required to have the same name for input_layer.

  regions
  output_layer
  input_layer <- gets pasted twice
  input_layer <- becomes unset/empty on paste

For 2): This specifically breaks when I use lots of layers with the same name but organize them in a folder structure.

Layer1/
  objects
  walls
  ground
Layer2/
  objects
  walls
  ground

I can't ever quite paste things into the correct layer with this structure, which I'm reluctant to give up. I end up doing multi-layer pastes slower or by hand here.


What does the clipboard data look like? Can't Tiled be reasonable about it? It should include the full path to the layer in the "name" for fixing 2). And it should try each successive match for a layer name in 1) if it's already pasted to the previous input_layers within that operation. Whilst a warning would prevent me deleting data... please don't just emit a warning. 😂 I'd love to see the friction go away in this workflow.

@eishiya
Copy link
Contributor

eishiya commented Dec 18, 2021

There's currently no concept of a layer "path" in Tiled, and if there were, the path would be too changeable to be a useful way to refer to layers, since layers can be reordered, layers with the same name can exist within the same group, etc. Even if merging is made relative to the selected layers as I suggested, however, Automapping would still have this issue since Automapping has no layer position data, only a layer name. Schemes for having Automapping refer to layers within specific groups have been proposed though.

The copypaste data for tiles is a brush, which is just another map. It has the original layers all preserved (minus groups, I think), but it's the merge function in Tiled that's used to merge the brush and the destination map that uses name-based matching that messes up.

For now, you may find my Automap Helper script useful. Among other things, it adds an action to the Edit menu that adds UIDs to your selected layers' names so that they're all uniquely named, so all your input_layer can be distinguished when pasting multiple layers. There's a corresponding action to remove the UIDs that you can run when you're ready to run your automap rules. There is also a "Convert Brush to Layers" action that renames the layers in your brush to match the currently selected layers, allowing you to multi-layer paste from one set of layers to a different set of layers.
This won't help your Automapping rules target same-named layers, but it should at least help while editing your automap rules, or to make sure that manual changes to your map get the correct layer.

@bjorn bjorn self-assigned this Dec 20, 2021
bjorn added a commit that referenced this issue Dec 20, 2021
* Don't reuse target layers in MapDocument::paintTileLayers. Instead,
  prefer to pick the next layer with a matching name, creating a new one
  if there isn't one.

* Adjusted logic in StampBrush::drawPreviewLayer to not rely on searching
  by layer name at all, and to create multiple layers with the same name
  in the preview, when they existed in the stamp.

WIP because the logic should probably be improved further, by taking the
selected layers into account. AbstractTileTool::targetLayersForStamp still
needs to be adjusted as well.

Issue #3094
bjorn added a commit that referenced this issue Dec 21, 2021
When painting into multiple layers, and having the same number of tile
layers selected as on the current brush, the layers are now matched based
on their order instead of their name. This has the following advantages:

* It makes sure that capturing or copying part of a map to the same map
  will always end up in the expected location, regardless of the layer
  names.

* It makes it possible to move the content of multiple layers to another
  set of layers, in the common case where the order of these layers stays
  the same.

When the number of selected tile layers does not match up, the layers are
still matched by name. However, target layers are no longer reused
when the tile stamp contains multiple layers with the same name. In this
case, each consecutive layer with the same name is matched with the next
layer with that name, or one will be created.

The logic in StampBrush::drawPreviewLayer and
AbstractTileFillTool::fillWithStamp was adjusted to not rely on searching
by layer name at all, and to create multiple layers with the same name in
the preview, when they existed in the stamp.

Closes #3094
@bjorn
Copy link
Member

bjorn commented Dec 21, 2021

I think the behavior implemented in e221d6c could be quite acceptable. Please help testing it out based on the following builds: https://github.com/mapeditor/tiled/actions/runs/1607260849.

bjorn added a commit that referenced this issue Dec 21, 2021
When painting into multiple layers, and having the same number of tile
layers selected as on the current brush, the layers are now matched based
on their order instead of their name. This has the following advantages:

* It makes sure that capturing or copying part of a map to the same map
  will always end up in the expected location, regardless of the layer
  names.

* It makes it possible to move the content of multiple layers to another
  set of layers, in the common case where the order of these layers stays
  the same.

When the number of selected tile layers does not match up, the layers are
still matched by name. However, target layers are no longer reused
when the tile stamp contains multiple layers with the same name. In this
case, each consecutive layer with the same name is matched with the next
layer with that name, or one will be created.

The logic in StampBrush::drawPreviewLayer and
AbstractTileFillTool::fillWithStamp was adjusted to not rely on searching
by layer name at all, and to create multiple layers with the same name in
the preview, when they existed in the stamp.

Closes #3094
@bjorn
Copy link
Member

bjorn commented Dec 21, 2021

Whoops, all builds failed because my patch only compiles against Qt 6 at the moment. Will have to fix later...

@bjorn
Copy link
Member

bjorn commented Dec 21, 2021

Hopefully the build at https://github.com/mapeditor/tiled/actions/runs/1607828339 will work out better.

@Macksaur
Copy link

Awesome! It seems to work great when it behaves! I mainly tested it with automapping rules as a point of context.

Working:

  • moving around lots of automapping rules, with duplicate layer names, within the same file ✔
  • copying automapping rules to an empty file with the same layer structure ✔
  • copying automapping rules to an empty file with only missing duplicate layers ✔
  • copying multiple layers to a different file with the same named multiple layers ✔
  • copying multiple layers to a different file with the same named multiple layers in a different order ✔

Problems:

  • copying automapping rules to an entirely empty file ❌ (it doesn't paste anything if there are any initial layers missing)
  • copying automapping rules to an empty file with missing duplicate layers creates the duplicate layers at the top before all other layers instead of after their same named layer ❌
  • copying multiple layers to a different file with the same named multiple layers (and they are selected in the target) ❌
  • copying multiple layers to a different file with none of the same named layers existing (and the desired target layers are selected) ❌ I can't get this to paste anything...
  • when I open a map the stamp tool does absolutely nothing and is broken until I rectangle tool + copy/paste something, then the stamp tool functions ❌(this happens with a single tile layer or a more complex group structure)
  • "repairing" the stamp tool by copy/pasting and then switching map breaks it in the original map you left ❌
  • the stamp and rectangle tool when making a multilayer copy/paste can sometimes get stuck drawing red instead of blue ❌ (sorry I don't know what led to this)

Hopefully that all makes sense. Thanks.

@bjorn
Copy link
Member

bjorn commented Dec 22, 2021

@Macksaur Thank you so much for the thorough testing! It's great that so many scenarios now worked fine, but it seems with my focus on the multi-layer painting and due to some last-minute changes, a few bugs sneaked in.

  • copying automapping rules to an entirely empty file x (it doesn't paste anything if there are any initial layers missing)

This is currently expected, because the Stamp Brush is only enabled when you have a tile layer selected. The "Paste in Place" (Ctrl+Shift+V) action still works in this case, though.

  • copying automapping rules to an empty file with missing duplicate layers creates the duplicate layers at the top before all other layers instead of after their same named layer

I've pushed a change that improves this behavior. It now tries to insert new layers right after the previous one, or right before the next one, using the same parent layer. When there is no previous or next layer, it still ends up at the top. Of course, this will not always result in the right group, but right now I don't have a good feeling about introducing groups in the tile stamp.

I think all the remaining issues should now be fixed (855fc23). Please try this again with the latest version: https://github.com/mapeditor/tiled/actions/runs/1611162885

@Macksaur
Copy link

Working:

  • copying automapping rules to an entirely empty file ✔ (ok, requires ctrl+shift+v)
  • copying automapping rules to an empty file with missing duplicate layers creates the duplicate layers in the correct order ✔
  • copying multiple layers to a different file with the same named multiple layers (and they are selected in the target) ✔
  • copying multiple layers to a different file with none of the same named layers existing (and the desired target layers are selected) ✔
  • stamp tool works on opening arbitrary maps ✔
  • stamp tool works when switching maps ✔
  • stamp/rectangle tool draw expected colours during the aforementioned tests ✔

Wow, this is great! 😅 I am happy with these changes, they account for my workflows and are very smooth. Please, if anyone else can check these changes that would be great. They may suit me, but do they suit others' workflows?


I have one remaining quibble:

Paste in place creates the required layers (that's fine) but also performs the paste in the x/y from where it was copied in the source map (which may or may not, probably not, make any sense in the target map). Naturally this isn't what I desire when I'm pasting set-pieces. So...

  1. If I first copy multiple source layers from a map and change to a new map
  2. Ctrl+V does not work ❌
  3. Ctrl+Shift+V does work ✔, but pastes immediately (and in the wrong place)
  4. If I click on a layer (any layer) immediately after paste-in-place I get a brush that allows me to paste/draw on the same named layers as the source map. (this is desirable behaviour to obtain said brush)
  5. If then, at this point, I Ctrl+Z: the original paste-in-place disappears and I can then use the stamp as-is (as if I used Ctrl+V originally).

It works, but it's a lot of steps to undo the wrong paste and obtain the brush that would allow the paste to go through.

This is currently expected, because the Stamp Brush is only enabled when you have a tile layer selected.

The stamp brush doesn't allow the multilayer paste in a map with a single tile layer with the wrong name ❌. I have to jump through the hoops above. Which I guess, I can manage. This patch gives so much quality of life. But is there any reason to forbid that paste and require the hoops?

Thank you!

@bjorn
Copy link
Member

bjorn commented Dec 22, 2021

The stamp brush doesn't allow the multilayer paste in a map with a single tile layer with the wrong name.

Fixed in 78ab365 (https://github.com/mapeditor/tiled/actions/runs/1611560623). Thanks again for the thorough testing!

@bjorn bjorn closed this as completed in d175392 Dec 22, 2021
Roadmap automation moved this from Up Next to Completed Dec 22, 2021
@Macksaur
Copy link

Macksaur commented Dec 22, 2021

  • stamp brush allows the multilayer paste in a map with a single tile layer with the wrong name ✔
  • stamp brush creates missing layers in the correct place (above or below) target layers when only one layer name matches and there are existing layers ✔

I know you said re: pasting

This is currently expected, because the Stamp Brush is only enabled when you have a tile layer selected.

Buuut...

  • multilayer paste requires a tile layer to be selected (any tile layer, it doesn't matter) even if it doesn't paste anything there ❌

Given that it's really quite hard to unselect all of the layers (deleting the currently selected layer works or selecting a group layer works) and given all of this fantastic new logic, should paste require a layer to be selected when it can now figure it out?

I am 50/50 about that being an actual problem that "needs fixing" because it may encroach on the normal use case of single-layer-pasting into another single-layer and confuse people. Maybe single-layer-pasting into a group or an object layer doesn't do anything? but multilayer paste does?... Since it can figure out layer creation? I don't know... Iffy.

Fantastic patch! Thanks!

@bjorn
Copy link
Member

bjorn commented Dec 22, 2021

Given that it's really quite hard to unselect all of the layers (deleting the currently selected layer works or selecting a group layer works) and given all of this fantastic new logic, should paste require a layer to be selected when it can now figure it out?

It's a good question, but at the moment I don't have the answer either. It's a topic we should return to though, also in the context of #2807, which is also due to tools getting disabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Broken behavior.
Projects
Archived in project
Roadmap
  
Completed
Status: Done
Development

No branches or pull requests

5 participants