Make TileMapLayers extend Node2D and work as children of TileMap

[groud]

The main goal of this PR is to start the work of moving TileMapLayers to individual nodes. For now, those are implemented as TileMap internal nodes, but I aim to move them as independent nodes in later PRs.

A few notes:

• the main goal of the PR was to remove some layer-specific properties that were redundant with the Node2D properties.
• the TileMapLayer node is still significantly dependent on the parent TileMap node:
  • The TileMap node is the one responsible to queue tilemap internal updates. This update queuing will need to be moved to the internals of the TileMapLayer
  • Some properties are defined at the TileMap level (collision_animatable, rendering_quandrant_size, collision_visibility_mode, etc...). The plan here is to "duplicate" those properties at the layer-level, then make the TileMap node update all its children layers at once.
• This should be 99% backward-compatible for now, though there might be some unexpected side-effects here and there as moving to a child node needed some tweaks here and there.
• For now, TileMapLayer properties/API is not exposed, it should be one of the last step. Basically, I plan a similar API to TileMap but without the need to specify a layer.

Once this is merged, I plan in the short term to remove the remaining dependencies from TileMapLayer to TileMap, then to split TileMap into two:

• TileMapGroup: a simpler node that will group layers together, allowing children TileMapLayers to share a TileSet, get highlighted/dimmed depending on the selected layer (and, maybe, get their position constrained). The API should be fairly simple.
• TileMap (extending TileMapGroup) : for backward-compatibility

Related to: godotengine/godot-proposals#7122

[KoBeWi]

Layer names need some default:

I wonder how the editing will look once TileMapLayers are nodes. I assume your plan is to allow the TileMap editor work per layer, so you can select node and edit it directly. But will the current workflow be preserved? Once the TileMapGroup is a thing, will I be able to select it and edit any layer without switching nodes?

Also name TileMapGroup sounds confusing, because it suggests grouping TileMaps, while it groups layers.

[groud]

Layer names need some default:

Ah yeah, I'll fix that.

I wonder how the editing will look once TileMapLayers are nodes. I assume your plan is to allow the TileMap editor work per layer, so you can select node and edit it directly.

Yes, that's the plan. Basically you would select a layer node in the scene tree instead of selecting the layer in the bottom dropdown.

But will the current workflow be preserved? Once the TileMapGroup is a thing, will I be able to select it and edit any layer without switching nodes?

Yes, for backward compatibility, but for the TileMap nodes. I might keep the TileMap editor as-is and provide a converter from a TileMap node to TileMapGroup + TileMapLayers. I thought about removing the TileMap editor, and only provide the converter, but I think forcing people to change all their API calls might be a bit too early right now.

Basically the plan is:

• TileMapGroup + TileMapLayer = new workflow, editor on TIleMapLayer but use the parents TileMapGroup for storing some things.
• TileMap: deprecated. Maybe keeps the current editor (if it's not too much work). Provides a converter to the new workflow.

Also name TileMapGroup sounds confusing, because it suggests grouping TileMaps, while it groups layers.

I kind of agree. Ideally I would have preferred the TileMapLayer node to be a TileMap one (basically removing the possibility to have layers on the TileMap node), but that would have made the backward compatibility a lot more complicated. We could rename TileMapGroup to TileMapLayerGroup, but that felt a bit long to me.

[KoBeWi]

So this is a problem. The current workflow allows to quickly switch between layers with a shortcut and it's very efficient. This ability should be preserved with the new workflow. However, even if we are able to quickly switch between layers, it will involve inspector updates, which are often expensive. Hence I think the Group node should allow editing all layers at once, like currently. It doesn't need any duplicate code, the editor can handle 2 node types and the group would be an extended case.

[arkology]

I would have preferred the TileMapLayer node to be a TileMap one (basically removing the possibility to have layers on the TileMap node), but that would have made the backward compatibility a lot more complicated. We could rename TileMapGroup to TileMapLayerGroup, but that felt a bit long to me.

I almost sure this was already discussed, but is it possible (reasonable) to integrate new layer system into old one using concept "use new system if has TileMapLayer child node, use old system if there are no TileMapLayer children"? Of course this will be complicated in terms of code maintinability, but solves "easy to use" goal.

BTW please link proposal issue to first post.

[groud]

So this is a problem. The current workflow allows to quickly switch between layers with a shortcut and it's very efficient. This ability should be preserved with the new workflow

That's a good point, but I think it's probably easily doable.

However, even if we are able to quickly switch between layers, it will involve inspector updates, which are often expensive. Hence I think the Group node should allow editing all layers at once, like currently. It doesn't need any duplicate code, the editor can handle 2 node types and the group would be an extended case.

Hmm, that could maybe be doable but I am not sure. The main problem is that the TileMap node relies on the layers being strictly indexed, so right now, in this PR, there's even an array to list all children layers. So, like, there's kind of an incompatibility between the TileMap editor (with the dropdown) and the TileMapLayer workflow. That probably means that I would need a special editor for TileMapGroup, and would not be able to reuse parts of the TileMap editor. But maybe I can figure out a solution later on.

However, this kind of sounds like putting a band-aid on a wooden leg. If switching node is slow due to inspector updates, we should maybe improve the inspector for that (like, threading its update or something).

I almost sure this was already discussed, but is it possible (reasonable) to integrate new layer system into old one using concept "use new system if has TileMapLayer child node, use old system if there are no TileMapLayer children"? Of course this will be complicated in terms of code maintinability, but solves "easy to use" goal.

I'd rather avoid it TBH, this makes things more confusing than they need to be IMO.

[KoBeWi]

The TileMapLayer editor is the same as TileMap editor, but the dropdown is hidden and fixed to the currently selected layer. As for indexing, when selecting TileMap you can run through all children and assign layers based on the order they appear. They won't change while editing.

[KoBeWi]

I tested the changes and found some weird bug with collisions.
My game scenes each have TileMap. The scenes are changed when you go out of screen and the new scene has global position next to the previous one.
The bug is that when I change scenes, some TileMap layers will have colliders shifted. What appears in game is different from how it collides. It only applies to some layers and they are offseted by some amount (not sure how much, given their real collider is invisible).

Here I go from one scene to another, and the other scene has tiles with no collision and invisible tiles. The bricks are on a different layer (0) than the sand (2).

Output.mp4

What's worse, when I return to the first scene, my move_and_collide() will report a collision with collider being null, when I collide with the TileMap.

EDIT:
btw I made an icon for TileMapLayer :P

<svg height="16" viewBox="0 0 16 16" width="16" xmlns="http://www.w3.org/2000/svg"><path d="m1 7v2h2v-2zm3 0v2h2v-2zm3 0v2h2v-2zm3 0v2h2v-2zm3 0v2h2v-2z" fill="#8da5f3"/></svg>

[groud]

I tested the changes and found some weird bug with collisions.

Have you compared it with master ? The issue might be related to what #86841 solves, so I am not sure this PR causes it. Let me know.

[KoBeWi]

The problem does not exist on master.

[groud]

Alright, I've just updated the PR. The bugs seems fixed now.
This PR includes the changes in #86841, it is required for the bug reported by @KoBeWi to be fixed.

[KoBeWi]

The collision issue is fixed, but the null collider bug still exists (i.e. move_and_collide() returning a collision where get_collider() returns null).

[groud]

The collision issue is fixed, but the null collider bug still exists (i.e. move_and_collide() returning a collision where get_collider() returns null).

Hmm, that's quite weird. It seems attached to the tilemap node correctly: ps->body_attach_object_instance_id(body, tile_map_node->get_instance_id()); I'll investigate.

[KoBeWi]

It does not happen consistently. In my case it happens when going to another scene and back (refer to my comment/video above).
I could try making some minimal project.

[groud]

It does not happen consistently. In my case it happens when going to another scene and back (refer to my comment/video above).

I tried reproducing the bug but no luck. I tried something like that (trying to see if reloading would would have an impact):

func _input(event):
	if event is InputEventKey and event.is_pressed() and event.keycode == Key.KEY_R:
		get_tree().change_scene_to_file("res://level.tscn")

But it still works after a reload (the collider ID is different after pressing R, which proves the TileMap has been correctly replaced).

[KoBeWi]

In my case I don't truly change/reload scene, just swap the map. The player node exists between scenes. Seems like this is the required condition.

Here's some simple repro scene:
NoColliderBug.tscn.txt
Just press any key and you will start getting nulls. The problem does not occur in 4.3 dev2.

[groud]

The collision issue is fixed, but the null collider bug still exists (i.e. move_and_collide() returning a collision where get_collider() returns null).

The destructor for layers was simply not cleaning things up, including physics bodies. So you would end up with bodies attached to a freed node. I updated the PR and it should be fixed now. (It works in the MRP at least)

[groud]

I also added @KoBeWi 's icon. It's a bit weird, but that's a good placeholder for now:

[YuriSizov]

Thanks!

[groud]

Ah damn, the canvas_item.cpp changes was wrong, I was expecting the other PR to be merged before and to rebase that one.

Sorry, I should have said it. I'll open a PR to fix that tomorrow.

[YuriSizov]

No worries, we're not going into stable yet 🙃

Edit: If by the other you mean #86841, I assume all we need to do is to remove the line about the transform from this changeset?

[naturally-intelligent]

Will this change make an idea like selecting tiles in multiple layers, as described in this proposal (godotengine/godot-proposals#9296) harder or easier?

[KoBeWi]

Selecting multiple layers will be easier, but it doesn't affect editing them. The editor can still only handle a single layer.