Change TileMapEditor to TileMapLayerEditor

[groud]

This is the continuation of my work on moving TileMap layers to individual nodes.

This PR does several things:

• Implement TileMapGroup class, that holds the TileSet and a few editor things for TileMapLayers
• Replace the TileMapEditor by a TileMapLayerEditor, basically changing the logic to act at the layer-level instead of the TileMap one.

Things should work more or less the same for now. The only difference should be in the display of the grid, as it will consider only the currently selected layer instead of the whole TileMap.

[AThousandShips]

Are we okay with major compatibility breakage for tilemap at this point? I'd say changing the inheritance even by inserting a different class between a class and it's parent is breaking compatibility

[groud]

Are we okay with major compatibility breakage for tilemap at this point? I'd say changing the inheritance even by inserting a different class between a class and it's parent is breaking compatibility

I don't know if it has any implication, but I guess it's pretty limited? At least loading a project works it seems.
I am not sure it is breaking anything.

I am not sure where we are in the dev cycles anyway, so I guess it's up to the prod team to decide.

[AThousandShips]

I'll tag it as such for now, as per the general concept of it, and we'll see if it's desirable, unsure what areas it might affect elsewhere

Might be implications for extensions when moving properties from one class to another

[KoBeWi]

Unlike the previous PR, this one introduces an unimplemented functionality in the form of TileMapGroup. You can create it, but it's completely useless and has no documentation. It's not really a problem at this stage, but it puts more pressure to have the new workflow finished in time.

I wonder if there is a way to do what you are planning without adding a new node. From what I understand, the only difference between TileMap and TileMapGroup is that the former doesn't expose its layer nodes, so you have to edit them in the inspector. But since it will create the layer internally anyway, it should work fine if it happens when the user opens an old scene for the first time, no? It would require a bit of compatibility code, but the TileMap can be converted automatically to TileMapGroup, thus the new node is not really needed.

[groud]

Unlike the previous PR, this one introduces an unimplemented functionality in the form of TileMapGroup. You can create it, but it's completely useless and has no documentation. It's not really a problem at this stage, but it puts more pressure to have the new workflow finished in time.

I could make it an abstract class I guess, that would work.

I wonder if there is a way to do what you are planning without adding a new node. From what I understand, the only difference between TileMap and TileMapGroup is that the former doesn't expose its layer nodes, so you have to edit them in the inspector. But since it will create the layer internally anyway, it should work fine if it happens when the user opens an old scene for the first time, no? It would require a bit of compatibility code, but the TileMap can be converted automatically to TileMapGroup, thus the new node is not really needed.

Hmm. That is good question. There are several problem to solve, but I am not 100% sure of what is doable:

• if we keep only one node, that means we will have to keep the TileMap API as is, but deprecate a lot of its methods. I guess that's acceptable, but it might be a bit more confusing than deprecating the node.
• Some parts of the to-be-deprecated API rely on the child nodes having values controlled by the parent TileMap. For example, the collision_animatable property requires the children TileMapLayer nodes to be set as use_kinematic_body. Also, I moved some settings to individual layers, like the rendering_quadrant_size. This means that some of those will have no effect anymore on the children nodes (or it could have, but it might be weird in the editor that a parent node modifies its children properties)
• The deprecated API will be slowed down as every call will have to iterate over children nodes to find the layer corresponding to a given index (in case you have nodes as child which are not TileMapLayers). That might be acceptable, idk.
• In general, that would keep a lot of bloat in the TileMap API, so not sure what is worth. This might be solvable by keeping the TileMapGroup we have now, but not expose it to users though. That would simplify the work needed the day be remove the deprecated part basically.

[KoBeWi]

idk then. The day when we remove deprecated stuff is very far away, so when redesigning the API we need to keep in mind that this will stay for a long while. Also another concern is what we want to be the default name of the node. I think TileMap is more intuitive/familiar than TileMapGroup and making the latter default would be a bit odd. It would be best if we could rename the old node to e.g. TileMapOld, but it's not possible unfortunately :/

If we go the route of adding a new class (which can be abstract for now), I think at least it should have a better name.
LayeredTileMap? TileManager? TileLayerArray? TileLayerMap? No idea tbh.

[groud]

Alright. According to @dsnopek, adding the detected API changes in the .expected file should work.
I also renamed TileMapGroup to TileMapLayerGroup and made it abstract.

[AThousandShips]

You seem to have generated your documentation on an out of date branch and deleted some content

[groud]

You seem to have generated your documentation on an out of date branch and deleted some content

It should be fixed now, the PR should be ready.

[YuriSizov]

We should merge it soon (but probably not for dev3, which we plan to have tomorrow). But before we do, I wanted someone to take a look at the changes so we have two approvals. It can be me or Remi, but volunteers are also welcome!

[AThousandShips]

The code looks good, did some basic testing but haven't done extensive use of it yet, but the changes to the map itself seems reasonable, and most of the editor side is renaming

Edit: this might worsen threading issues due to making the queuing unique to each layer, but the threading should be simple to fix, see:

• Prevent threading problems in TileMap #87478

[kleonc]

Skimmed though the code, mostly looks fine to me. 👍

[AThousandShips]

Unsure if this is exactly what's intended here but applied this to fix the compilation issues on my end:

diff --git a/scene/2d/tile_map_layer.cpp b/scene/2d/tile_map_layer.cpp
index 5e351293b3c42..e866d82c03564 100644
--- a/scene/2d/tile_map_layer.cpp
+++ b/scene/2d/tile_map_layer.cpp
@@ -198,6 +198,7 @@ void TileMapLayer::_rendering_update() {
 
 		// Modulate the layer.
 		Color layer_modulate = get_modulate();
+#ifdef TOOLS_ENABLED
 		const Vector<StringName> selected_layers = tile_map_node->get_selected_layers();
 		if (tile_map_node->is_highlighting_selected_layer() && selected_layers.size() == 1 && get_name() != selected_layers[0]) {
 			TileMapLayer *selected_layer = Object::cast_to<TileMapLayer>(tile_map_node->get_node_or_null(String(selected_layers[0])));
@@ -212,6 +213,7 @@ void TileMapLayer::_rendering_update() {
 				}
 			}
 		}
+#endif // TOOLS_ENABLED
 		rs->canvas_item_set_modulate(get_canvas_item(), layer_modulate);
 	}
 
@@ -2965,4 +2967,4 @@ TerrainConstraint::TerrainConstraint(Ref<TileSet> p_tile_set, const Vector2i &p_
 		}
 	}
 	terrain = p_terrain;
-}
\ No newline at end of file
+}
diff --git a/scene/2d/tile_map_layer_group.cpp b/scene/2d/tile_map_layer_group.cpp
index be522e2aa9c0d..7ddafc12104ca 100644
--- a/scene/2d/tile_map_layer_group.cpp
+++ b/scene/2d/tile_map_layer_group.cpp
@@ -101,7 +101,9 @@ bool TileMapLayerGroup::is_highlighting_selected_layer() const {
 #endif // TOOLS_ENABLED
 
 void TileMapLayerGroup::remove_child_notify(Node *p_child) {
+#ifdef TOOLS_ENABLED
 	_cleanup_selected_layers();
+#endif
 }
 
 void TileMapLayerGroup::_bind_methods() {

Would be good to get that out of the way and this merged

[akien-mga]

Thanks!