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

Scripting: Allow specifying layer types for which a Tool should be enabled #3248

Closed
eishiya opened this issue Jan 19, 2022 · 3 comments · Fixed by #3591
Closed

Scripting: Allow specifying layer types for which a Tool should be enabled #3248

eishiya opened this issue Jan 19, 2022 · 3 comments · Fixed by #3591
Assignees
Labels
feature It's a feature, not a bug.
Milestone

Comments

@eishiya
Copy link
Contributor

eishiya commented Jan 19, 2022

Scripted Tools can be set to enabled or disabled, but in most cases, this just needs to react to the currently selected layer(s) without much complex logic. We have Tool.updateEnabledState() in which to make these decisions, but it would be great to offload this work to Tiled and just specify what kinds of layers the tool should be enabled or disabled on. Tiled already handles similar logic with Tool.usesSelectedTiles, which prevents the tool from changing when the user picks a tile in a tileset.

In #3246, it was suggested to have boolean properies like allowForObjectLayers, allowForTileLayers. I think it might be better to specify a single property like activateOn: ObjectGroup | ImageLayer | GroupLayer, so that its absence can less ambiguously mean "active for all layers".

It would be cool to have an additional property that decides whether to activate the tool when any of the selected layers are valid vs requiring all selected layers to be valid. Or I guess there can be one default behaviour, and if someone wants it different, they can override it in their script using the usual (tedious) method.

If Tileset tools are ever added, this could be expanded with Tileset as an additional acceptable item.

@bjorn bjorn added the feature It's a feature, not a bug. label Jan 19, 2022
@bjorn bjorn added this to the Tiled 1.10 milestone Feb 15, 2023
@bjorn
Copy link
Member

bjorn commented Feb 28, 2023

It would be cool to have an additional property that decides whether to activate the tool when any of the selected layers are valid vs requiring all selected layers to be valid.

Hmm, for most existing tools, they actually base their enabled state only on the "current layer", not the "selected layers". I think it would make sense to do the same for this setting, or if we choose a different behavior for this setting, to do the same for the existing tools (but I'm currently not sure if that's beneficial or which problems that might introduce).

@bjorn bjorn self-assigned this Feb 28, 2023
@eishiya
Copy link
Contributor Author

eishiya commented Feb 28, 2023

I agree that it should be consistent for scripted and built-in tools. Currently, most of Tiled's tools only work on either one layer (often, but not always, the current layer) or on any relevant layer, so making them activate based on selected layers wouldn't make sense. I'd like to say I'd love to see some of those tools modified to make sense for working on multiple selected layers, but that just doesn't have very clear logic as to how it should work. For example, it'd be great to Select Same Tile from several layers, but which tile is chosen when you click?

So, in the interest of consistency, until/unless more of Tiled's built-in tools can benefit from being selected based on selected layers rather than current layer, I'm fine with it being based on the current layer.

That said, I think the behaviour of setting the current layer could be improved. It's currently very easy to end up in a situation where you have only Tile Layers selected but can only choose Object tools because the current layer is an Object Layer or vice versa, if you deselect layers by modifier+clicking on them. Whenever possible, I think the current layer should be one of the selected layers.

@bjorn
Copy link
Member

bjorn commented Feb 28, 2023

It's currently very easy to end up in a situation where you have only Tile Layers selected but can only choose Object tools because the current layer is an Object Layer or vice versa, if you deselect layers by modifier+clicking on them. Whenever possible, I think the current layer should be one of the selected layers.

Yeah, actually it tries to do that, so possibly the deselection case is a bit of a bug. The question is how to address it. I guess we could try switching the current layer to the next selected layer, when available.

Thanks for your feedback!

bjorn added a commit to bjorn/tiled-dev that referenced this issue Feb 28, 2023
This new property specifies the target layer type for which this tool
should be enabled, providing a convenient alternative to implementing
Tool.updateEnabledState.

Closes mapeditor#3248
bjorn added a commit to bjorn/tiled-dev that referenced this issue Feb 28, 2023
This new property specifies the target layer type for which this tool
should be enabled, providing a convenient alternative to implementing
Tool.updateEnabledState.

Closes mapeditor#3248
bjorn added a commit to bjorn/tiled-dev that referenced this issue Feb 28, 2023
This new property specifies the target layer type for which this tool
should be enabled, providing a convenient alternative to implementing
Tool.updateEnabledState.

Closes mapeditor#3248
bjorn added a commit that referenced this issue Feb 28, 2023
This new property specifies the target layer type for which this tool
should be enabled, providing a convenient alternative to implementing
Tool.updateEnabledState.

Closes #3248
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature It's a feature, not a bug.
Projects
Status: Done
Status: Todo
Development

Successfully merging a pull request may close this issue.

2 participants