Improve TileMap performances by using quadrants only for rendering

[groud]

A bit like #74603, but adds a ton of additional optimization and cleanup.

All TileMap update are now pushed at the end of the frame, though you may trigger this update earlier on by calling force_update(). Also, I added a notify_runtime_tile_data_update so that you can tell the TileMap that some runtime data was updated, without triggering a direct update.

The TileMap now uses a dirty structure (with flags and a list of modified cells) to know what was modified since the last update. Each TileMap's subsystem (rendering, physics, scenes...) then uses those flags and list of dirty cells to smartly update only what is needed. Unlike before, cells are only grouped in quadrants where needed. That means that updating a single cell does not update hundreds of others (at least on the physics matter) for nothing.

This improvement, added to a reduction of the number of allocations, lead to a x2.8 performance improvement on TileMap updates. Using this small benchmark script, I measured FPSs:

extends Node2D

func _process(delta):
	random_tile_map_update()
	
func random_tile_map_update():
	for x in range(100):
		for y in range(100):
			if $TileMap.get_cell_source_id(0, Vector2i(x,y)) == -1:
				$TileMap.set_cell(0, Vector2i(x,y), 0, Vector2i(randi_range(0, 2), randi_range(0, 2)), 0)
			elif randi_range(0, 100) == 0:
				$TileMap.set_cell(0, Vector2i(x,y), 0, Vector2i(randi_range(0, 2), randi_range(0, 2)), 0)

Right now this is a draft as I need to fix CI, add compatibility methods for set_quadrant_size and document a few things. But it's ready for testing otherwise. :)

Probably fixes: #79658

[KoBeWi]

What happened to the layer argument in force_update()? It doesn't exist in notify_runtime_tile_data_update().

EDIT:
I get this crash when changing a scene that contains TileMap:

[0] Object::notification (C:\godot_source\core\object\object.cpp:798)
[1] Object::_predelete (C:\godot_source\core\object\object.cpp:198)
[2] predelete_handler (C:\godot_source\core\object\object.cpp:1893)
[3] memdelete<TileData> (C:\godot_source\core\os\memory.h:105)
[4] TileMapLayer::_clear_runtime_update_tile_data (C:\godot_source\scene\2d\tile_map.cpp:1296)
[5] TileMapLayer::internal_update (C:\godot_source\scene\2d\tile_map.cpp:1942)
[6] TileMapLayer::~TileMapLayer (C:\godot_source\scene\2d\tile_map.cpp:2417)
[7] TileMapLayer::`scalar deleting destructor'
[8] memdelete<TileMapLayer> (C:\godot_source\core\os\memory.h:112)
[9] Ref<TileMapLayer>::unref (C:\godot_source\core\object\ref_counted.h:212)
[10] Ref<TileMapLayer>::~Ref<TileMapLayer> (C:\godot_source\core\object\ref_counted.h:223)
[11] Ref<TileMapLayer>::`scalar deleting destructor'
[12] LocalVector<Ref<TileMapLayer>,unsigned int,0,0>::resize (C:\godot_source\core\templates\local_vector.h:139)
[13] LocalVector<Ref<TileMapLayer>,unsigned int,0,0>::clear (C:\godot_source\core\templates\local_vector.h:113)
[14] LocalVector<Ref<TileMapLayer>,unsigned int,0,0>::reset (C:\godot_source\core\templates\local_vector.h:116)
[15] LocalVector<Ref<TileMapLayer>,unsigned int,0,0>::~LocalVector<Ref<TileMapLayer>,unsigned int,0,0> (C:\godot_source\core\templates\local_vector.h:322)
[16] TileMap::~TileMap (C:\godot_source\scene\2d\tile_map.cpp:4520)
[17] TileMap::`scalar deleting destructor'
[18] memdelete<Node> (C:\godot_source\core\os\memory.h:112)
[19] Node::_notification (C:\godot_source\scene\main\node.cpp:212)
[20] Node::_notificationv (C:\godot_source\scene\main\node.h:49)
[21] CanvasItem::_notificationv (C:\godot_source\scene\main\canvas_item.h:45)
[22] Node2D::_notificationv (C:\godot_source\scene\2d\node_2d.h:37)
[23] Object::notification (C:\godot_source\core\object\object.cpp:800)
[24] Object::_predelete (C:\godot_source\core\object\object.cpp:198)
[25] predelete_handler (C:\godot_source\core\object\object.cpp:1893)
[26] memdelete<Node> (C:\godot_source\core\os\memory.h:105)
[27] Node::_notification (C:\godot_source\scene\main\node.cpp:212)
[28] Node::_notificationv (C:\godot_source\scene\main\node.h:49)
[29] CanvasItem::_notificationv (C:\godot_source\scene\main\canvas_item.h:45)
[30] Node2D::_notificationv (C:\godot_source\scene\2d\node_2d.h:37)
[31] Object::notification (C:\godot_source\core\object\object.cpp:800)
[32] Object::_predelete (C:\godot_source\core\object\object.cpp:198)
[33] predelete_handler (C:\godot_source\core\object\object.cpp:1893)
[34] memdelete<Object> (C:\godot_source\core\os\memory.h:105)
[35] SceneTree::_flush_delete_queue (C:\godot_source\scene\main\scene_tree.cpp:1344)
[36] SceneTree::physics_process (C:\godot_source\scene\main\scene_tree.cpp:481)
[37] Main::iteration (C:\godot_source\main\main.cpp:3463)
[38] OS_Windows::run (C:\godot_source\platform\windows\os_windows.cpp:1474)
[39] widechar_main (C:\godot_source\platform\windows\godot_windows.cpp:182)
[40] _main (C:\godot_source\platform\windows\godot_windows.cpp:204)
[41] main (C:\godot_source\platform\windows\godot_windows.cpp:218)
[42] WinMain (C:\godot_source\platform\windows\godot_windows.cpp:232)
[43] __scrt_common_main_seh (D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[44] <couldn't map PC to fn name>

Not sure what causes it though as it doesn't happen with every scene.

EDIT2:
I also got this error, not sure if related, but also happened when changing scenes (this time in editor):

C:\godot_source\modules/navigation/godot_navigation_server.cpp:1029 - Attempted to free a NavigationServer RID that did not exist (or was already freed).

EDIT3:
The navigation error seems to be spammed when I animate layer's modulate. I don't use navigation at all.

[groud]

What happened to the layer argument in force_update()? It doesn't exist in notify_runtime_tile_data_update().

Ah good point. I removed it as update are ran once per frame by the TileMap node itself, and not by each layer individually. But it likely makes sense to add it to notify_runtime_tile_data_update. Will do that.

I'll have a look to the bugs.

[groud]

Done. I fixed the bugs and added an optional argument to notify_runtime_tile_data_update, let me know if you still spot any bug.

Edit: I "fixed" CI by adding some lines to the .expected file. However, this error is still logged:

ERROR: Validate extension JSON: Missing field in current API 'classes/TileMap/methods/force_update': arguments. This is a bug.
   at: compare_dict_array (core/extension/extension_api_dump.cpp:1205)

Not sure what we want to do about it.

[KoBeWi]

The previous problems are fixed, but I'm getting this error:

ERROR: No tiles for the given body RID 74178380178984.
   at: (C:\godot_source\scene/2d/tile_map.cpp:3392)

Might be caused by me deleting all collision polygons on a layer. Not sure if intended.

[groud]

The previous problems are fixed, but I'm getting this error:

This is the message given by get_coords_for_body_rid when it cannot find the provided RID. I think this message could maybe be removed, as you might call it with an RID you are not sure is part of a TileMap.

I am not sure what you mean by "deleting all collision polygons on a layer", I guess by using a runtime update ? If that's the case, it should not prevent get_coords_for_body_rid to work correctly (in case you are colliding with a TileMap), so there might be a bug.

[dsnopek]

As discussed on RocketChat, the compatibility methods added here will provide compatibility for GDExtension, however, renaming these methods will still affect GDScript and C# compatibility. I don't how popular/obscure these methods are, so maybe it's OK if GDScript/C# code calling the old names breaks, but that's a discussion that should be had!

If we want to preserve GDScript/C# compatibility, the old methods could be marked as deprecated and simply call the new methods. (And, if we went that way, the compatibility methods for GDExtension could be removed.)

[KoBeWi]

I guess by using a runtime update ?

Yes, I use it do disable collisions on a layer.

I don't how popular/obscure these methods are

In my project I only had 3 uses of force_update(). In my case I only needed it when changing something that doesn't cause TileMap's auto-update.

[groud]

Put that PR to draft for now, collision_animatable isn't working anymore and I need to fix it :)

[groud]

Put that PR to draft for now, collision_animatable isn't working anymore and I need to fix it :)

This is now fixed.

Edit: I think the bug was probably here on 4.1 already. This PR likely fix it (see #79658)

[YuriSizov]

Regarding force_update/update_now. Suppose the deprecation of the old method is reasonable (as the old name suggests it will force an update, and the new behavior won't do that, it acts more like flushing a queue). Can we maybe pick a different name for update_now to make it less colliding with the old force_update?

@akien-mga suggested apply_queued_update (or just apply_update), which sounds fine to me. Maybe some variation of that.

[groud]

@akien-mga suggested apply_queued_update (or just apply_update), which sounds fine to me. Maybe some variation of that.

Yeah, that could work fine too. I thought about preemptive_update maybe, or early_update, as it makes it clear that the TileMap would update anyway. apply_queued_update would work, though maybe a bit confusing as users do not manually "queue an update" themselves.

I don't know, I can't decide ahah.

[YuriSizov]

They kind of do with the notify method, no? But in that case, just apply_update should be a good option? As long as the docs make it clear that updates are "applied" automatically and this method is does not need to be called normally.

[groud]

They kind of do with the notify method, no?

If they use the runtime TileData update (which I believe not a lot of users need) they might.

But the current update_now function is a bit unrelated. Basically all TileMap update (like, physics, rendering, navigation, scenes, etc...) are pushed to the end of the frame. This makes it so all TileMap internals (bodies, canvas items, child scenes, etc...) are not accessible right after a property change on the TileMap.

For example, if you paint a scene tile, the actual scene will be added only at the end of the frame. That's where you might want to call update_now right away, so that you could access some internal data earlier on:

$TileMap.set_cell(0, position, 0, Vector2i(0,0)) # Set a scene tile.
$TileMap.get_child_count() # returns 0

$TileMap.update_now()
$TileMap.get_child_count() # returns 1

Basically, that means that the main use case for update_now does not need any call the other notification function.

[YuriSizov]

Basically, that means that the main use case for update_now does not need any call the other notification function.

Okay, I'm a bit confused about how old code maps to the new one. Can't it be just update then? What now brings to the table? It seems to just force an update.

[groud]

Basically, that means that the main use case for update_now does not need any call the other notification function.

Okay, I'm a bit confused about how old code maps to the new one. Can't it be just update then? What now brings to the table? It seems to just force an update.

Well, yeah. That would work perfectly, lol.

I think I did not consider the idea because it was not possible before due to CanvasItem.update(), but since it was renamed in #64377, it should be perfectly doable now. A simple update() would work.

Edit: done, renamed to update()

[akien-mga]

Basically, that means that the main use case for update_now does not need any call the other notification function.

Okay, I'm a bit confused about how old code maps to the new one. Can't it be just update then? What now brings to the table? It seems to just force an update.

Well, yeah. That would work perfectly, lol.

I think I did not consider the idea because it was not possible before due to CanvasItem.update(), but since it was renamed in #64377, it should be perfectly doable now. A simple update() would work.

Edit: done, renamed to update()

While I welcome the consensus, I'm a bit wary of re-adding methods with a name that's quite generic / can clash with user defined methods. Since it's on TileMap, which is a node users are likely to assign a script too, it's not unreasonable to expect some would have a method named update() doing some stuff. A slightly more specific name would help prevent clashes.

[groud]

Renamed to update_internals, as discussed on the maintainers' chat.

[YuriSizov]

I'll just re-approve to confirm this consensus :)

[akien-mga]

Thanks!