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

Move TileMap layers to their own node #7122

Closed
groud opened this issue Jun 20, 2023 · 19 comments
Closed

Move TileMap layers to their own node #7122

groud opened this issue Jun 20, 2023 · 19 comments
Labels
Milestone

Comments

@groud
Copy link
Member

groud commented Jun 20, 2023

Describe the project you are working on

Godot

Describe the problem or limitation you are having in your project

As described in #6415, I mentioned that we may, at some point, move TileMap layers to their own nodes. I wanted to describe a bit more what I had in mind.

The current implementation is OK-ish, it works, but it has several issues:

  • I originally thought that layers would only be kept quite simple, but people keep asking about adding new feature to them (enabling, modulation, Y-sort, etc...). The layer list in the inspector is now quite big and is hard to work with.
  • The layer selector in the bottom toolbar isn't very optimal, it's weirdly placed.
  • It makes the API complex, as you always have to provide a layer index for all TileMap manipulation.
  • In general, it kind of goes against Godot's design

Describe the feature / enhancement and how it helps to overcome the problem or limitation

The idea would be to go back to one TileMap node = one layer. That would allow adding a lot more properties there and simplifying things. Selecting layers would be like selecting node and would simplify the API.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

To keep what the TileMap layers system helps with (highlighting the selected layer, keeping all layers aligned and allowing several layers to share the same TileSet) I would suggest to introduce a new TileMapGroup node.

TileMap layers as a node could use their own TileSet, but if they are child of a TileMapGroup node, a TileSet set on the TileMapGroup could be used instead. Also, the TileMapGroup node would be able to constraint its children transform, like Containers do for Control nodes. Finally, the TileMapGroup node should be able to keep the information regarding the layer highlighting.

This should allow keeping all features that are currently available.

As a side note, I think that keeping compatibility should be doable on the scene part. For the code, I don't think it will be possible easily.

If this enhancement will not be used often, can it be worked around with a few lines of script?

It's core

Is there a reason why this should be core and not an add-on in the asset library?

It's core

@groud groud added the topic:2d label Jun 20, 2023
@jordanlis
Copy link

Hello, seriously, this is one of the most interesting feature of this new tilemap/tilesets editor.

Totally disagree that the technical part should drive the final solution.

I think that this new tilemap system is good, but it has a lot of ux issues, and less features than the previous one.

We should really focus on improving the ux and adding what has been lost from the old tilemap editor rather than spending time undoing this amazing new layer system.

I decided to go on Godot 4 exclusively because it was so time consuming creating levels with the old tile maps, now I'm really disappointed to see that some people want to remove them.

Seriously, just look at all the other level editor : it's too much time consuming to have mutiple layers with tilemap, currently it's perfect to be able to keep everything as it is when painting tiles and just change the layer. In the old editor, it was so complex, I had to remember what type of tiles were on which tilemap, and handling separately each tileset was awefull.

I had to make sure that each tiles was on the right tilemap and sometimes, I was searching on which tilemap was a given tile so I had to look through all my layers to paint a given time.

All this is totally fine now with this new system. But the ux needs to be improved, not the technical part again they will also break the compatibility.

Just look at all the other tilemap / tileset editors that are working and try to copy what is working best.

You are on a good path, don't undo the improvements please because some people are asking it. We don't have the luxury to rethink again the whole system, because users were waiting for so long for this feature

@jordanlis
Copy link

And I must add a thing : sorry but currently, all users can continue to work with one tilemap per layer, so what's the bug deal here?

Definitely, why bother those that are satisfied with it?

@Zireael07
Copy link

@jordanlis The OP quite clearly explains why they want to change. The current inspector is big AND people keep asking for more features on layers. To be able to accomodate said new features, layers need to be made into a node.

@groud
Copy link
Member Author

groud commented Jun 27, 2023

We should really focus on improving the ux and adding what has been lost from the old tilemap editor rather than spending time undoing this amazing new layer system.

I think you misunderstood the proposal, I don't suggest to remove any feature to it. Everything should work more or less the same. The only difference would be that the different layers would be in different nodes instead of having a huge list in the inspector.

This whole proposal is basically about improving the UX, not remove any feature to it.

@jordanlis
Copy link

OK, so on my side I would like to be more precise on what's important for me :

  • layer selector : important to keep it, I agree that it should be moved on top of the window
  • tiles selecting : when switching from one layer to another, it's important that the same tileset is selected, and that changing tileset can be done separately than selecting a layer. For example when I switch from layer 1 to layer 2, I don't want my workspace to chance either the selected tileset or the tile selected. It means that a tileset can be defined inside multiples tilemap nodes as you described.

@groud
Copy link
Member Author

groud commented Jun 27, 2023

layer selector : important to keep it, I agree that it should be moved on top of the window

There would be no need to have a layer selector anymore. Selecting a layer would be like selecting a node.

tiles selecting : when switching from one layer to another, it's important that the same tileset is selected, and that changing tileset can be done separately than selecting a layer. For example when I switch from layer 1 to layer 2, I don't want my workspace to chance either the selected tileset or the tile selected. It means that a tileset can be defined inside multiples tilemap nodes as you described.

The idea would be that the TileMapGroup node could be assigned a TileSet resource. If you set a TileSet there and not on the TileMap node, then this TileSet will be shared by the different TileMaps in the group. So yeah, everything could be kept synchronized as they are now.

@jordanlis
Copy link

Yeah well I kinda disagree about the selector thing Vs selecting a node in the architecture, cause that's not the same ux maybe you should think about keeping a selector to change a node but anyway fine for me if the workspace can stay the same and that a tileset can be shared.

I have a lot of ux suggestions to improve this feature, I should spend a little of time to create a proposal.

Other important point is the change of color when you select a tile : it's important that when you select a layer, the tiles of the other layer are darker to distinguish on what layer you are painting. That's another great improvement of this new system that you should keep according to me.

@jordanlis
Copy link

FYI, I worked on some improvements proposal for the tilesets management here : #7177

Will see if I see any improvement for the tilemap in another proposal

@Proggle
Copy link

Proggle commented Sep 19, 2023

We should really focus on improving the ux and adding what has been lost from the old tilemap editor rather than spending time undoing this amazing new layer system.

I think you misunderstood the proposal, I don't suggest to remove any feature to it. Everything should work more or less the same. The only difference would be that the different layers would be in different nodes instead of having a huge list in the inspector.

This whole proposal is basically about improving the UX, not remove any feature to it.

So the main change would be where you select the layer when placing tiles?

image
Or am I misunderstanding the workflow change?

@groud
Copy link
Member Author

groud commented Sep 19, 2023

So the main change would be where you select the layer when placing tiles?

Yes. Instead of selecting in the bottom-right dropdown menu, you would select it in the scene tree.

@jordanlis
Copy link

jordanlis commented Sep 19, 2023

By the way, why not keeping the dropdown menu, but inside it we can put the layers in the node hierarchy that you want to create ?
Like a mix between the 2 solutions.

I think the main "issue" for me overall is that it's not easy to switch from one layer to another.
I would love to put the cursor over the dropdown menu, scroll with the mouse wheel and change in one finger move the layer. I don't know if you see what I mean ?

Or do we have a keyboard shortcut to select the node up or down ?

@groud
Copy link
Member Author

groud commented Sep 19, 2023

By the way, why not keeping the dropdown menu, but inside it we can put the layers in the node hierarchy that you want to create ? Like a mix between the 2 solutions.

I don't understand why would we do that tbh. It's super common in the Godot UX to change what you edit using the scene hierarchy.

I think the main "issue" for me overall is that it's not easy to switch from one layer to another. I would love to put the cursor over the dropdown menu, scroll with the mouse wheel and change in one finger move the layer. I don't know if you see what I mean ?

Ah yeah, but that's not implemented right now anyway. But tbh, I don't think clicking a node in the scene tree is very complex anyway.

Or do we have a keyboard shortcut to select the node up or down ?

I don't think we have it, but that's actually a very good idea. We could add that.

@jordanlis
Copy link

Well, maybe I should create a separate request for that then.

To answer your "questions", I have a lot of difficulties to navigate in the scene hierarchy, even thouht I have a big screen, because I have many nodes. Not so many, but enough to have difficulties to select the desired node when searching with the mouse cursor.

@Proggle
Copy link

Proggle commented Sep 19, 2023

Yeah it would be good if it were easier to swap between layers, maybe a hotkey+mousewheel or something to go up/down. When drawing tiles I have multi-tile objects with components on multiple layers, and being able to easily map the whole object is pretty important to quickly building them. (For instance, I have tree sprites and tree shadow sprites - I put the shadow at the same x/y coordinates as the tree, but on a lower layer)

My main concern with this change is that splitting layers into their own nodes is that it would move the z-axis further away from being treated like the x and y axes in terms of interactions, since the z-axis would no longer be contained in the same node.

For instance one very useful feature that's in other tile editors is the ability to select multiple z-levels of tiles and move/copy/paste them. On the small scale, that's things like copy and pasting a tree and shadow together. On the larger scale, that's things like moving large chunks of the map around in a single operation so you can adjust the overall flow of the level (instead of moving each layer separately and trying to make sure they stay aligned).

That sort of cross-z selection/movement isn't yet implemented in godot, and I feel like splitting layers into different nodes would make that sort of multi-z-level interaction less natural (despite its obvious utility).

Or, in terms of actually implemented features, my current project has the ability to chop down tree tiles - so not only do I need to replace the tree tile with a stump, I also need to look at layers below it to get rid of the shadow. I have that currently set up with a physics layer for choppable objects. When it hits a tree, the axe find's the tree's coordinates, changes the tile at those coordinates, then looks at the lower z-layers for a shadow and changes that one.

Currently, specifying the layer adds a little complexity to referencing the locations, but mostly because it's being treated differently in functions like get_cell_atlas_coords ( int layer, Vector2i coords). If we want to alter the current system to make it less complex, if anything we should make the coordinates into 3space with layer as the z and have get_cell_atlas_coords ( Vector3i coords) - that would make it just as easy to grab the layer above or below a given tile as it is to grab the layer to the left or the right. Basically, my intuitive model for dealing with tiles is to treat them as a 3d block with x,y,z coordinates, and that matches it well (and the current setup of being an unusual coordinate isn't too bad either)

If I understand your mention of changing the API to not include layer references, would that mean I would need to search the node tree to find layers, then run a get_cell_atlas_coords ( Vector2i coords) on each layer node individually?

@groud
Copy link
Member Author

groud commented Sep 20, 2023

That sort of cross-z selection/movement isn't yet implemented in godot, and I feel like splitting layers into different nodes would make that sort of multi-z-level interaction less natural (despite its obvious utility).

Editing multiple layers at once should not be a lot more difficult to implement with different nodes than with with a single one (it is still pretty difficult to do anyway, and would require a significant amount of work).

Basically, the TileMapGroup node node should be the easy reference for all multi-layer operations. And thus make things adapt according to

If I understand your mention of changing the API to not include layer references, would that mean I would need to search the node tree to find layers, then run a get_cell_atlas_coords ( Vector2i coords) on each layer node individually?

I mean, if you organize your layers as direct children of the TileMapGroup node (which I guess should be the most common use case) you could just do $TileMapGroup.get_child(tree_layer + 1).get_cell_atlas_coords(...) to access the next layer. But well, you could also do something like this too:

$TreeTileMapLayer.remove_cell(coords) # chop the tree
$ShadowTileMapLayer.remove_cell(coords) # remove the shadow

I don't think accessing layers by node path is more complex than using an index.

@Proggle
Copy link

Proggle commented Sep 21, 2023

That code snippit seems to add the requirement that all the child nodes of every tilemap will have be identical in terms of layer arrangements and name. At that point, if all your nodes are required to be laid out in an identical way, there doesn't seem to be much advantage in having an alterable node structure.

But yeah I think "$TileMapGroup.get_child(tree_layer + 1).get_cell_atlas_coords(...)" is noticablely more complex to deal with.

Let's say we start with our axe. It hits the 'cuttable' physics layer, and has the 'treeCutter' boolean set to true. So first, we detect it by getting the body_rid when we collide with it, and from there back out and extract our layer and tile

func _on_body_shape_entered(body_rid, body, _body_shape_index, _local_shape_index):
	#get the coordinates of the tile we collided with
	if body.has_method("get_coords_for_body_rid"):
		var collidedTile=body.get_coords_for_body_rid(body_rid)
		var collidedLayer=body.get_layer_for_body_rid(body_rid)

At this point, if we want to erase what's below the tree, currently we just do

body.erase_cell(collidedLayer-1, collidedTile)

I understand your proposal properly we would have a 'layerNode' instead of 'collidedLayer', and the new way to do that would be something like:

layerNode.get_parent().get_child( layerNode.get_index() +1).erase_cell(collidedTile)

Which I think is definitely more complex and a lot easier to screw up when coding (and in fact I'm not 100% sure I didn't mess that up).

@groud
Copy link
Member Author

groud commented Sep 21, 2023

That code snippit seems to add the requirement that all the child nodes of every tilemap will have be identical in terms of layer arrangements and name. At that point, if all your nodes are required to be laid out in an identical way, there doesn't seem to be much advantage in having an alterable node structure.

Not at all. I suggested one solution, but depending on you use case it might or might not make sense to lay them that way. I just illustrated how your problem could be solved with the proposed system.

But yeah I think "$TileMapGroup.get_child(tree_layer + 1).get_cell_atlas_coords(...)" is noticablely more complex to deal with.

I don't think it is. Nodes are simple to work with and users are quite used to them.

I understand your proposal properly we would have a 'layerNode' instead of 'collidedLayer', and the new way to do that would be something like:

I mean, we could make it a signal instead of a virtual function. That's likely the way to got if we want to group the logic in the same node.

@KoBeWi
Copy link
Member

KoBeWi commented Apr 7, 2024

Implemented by godotengine/godot#89179

@KoBeWi KoBeWi closed this as completed Apr 7, 2024
@KoBeWi KoBeWi added this to the 4.3 milestone Apr 7, 2024
@jonSP12
Copy link

jonSP12 commented Aug 23, 2024

Now the only thing missing is to add the option to add more layers on the ( TileMap layers Node )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants