Editor crashes when deleting a TileSet Terrain

anotherwally commented 1 year ago

Godot version

v4.0.beta1.official [20d667284]

System information

Windows 11 Pro, AMD Ryzen 3 3200G with Radeon Vega Graphics, Vulkan Clustered

Issue description

Editor crashes when removing a used terrain from the set. It should remove the terrain only without closing editor.

Steps to reproduce

Open project
Open tiles.tscn (res://scenes/utils/tiles.tscn)
From the "Inspector", click "TileSet"
Select "Paint Properties" from the bottom panel
Select "Terrains" in the "Select a property editor" drop-down
Under "Painting", select the "Terrain Set 0" as the Terrain Set
Finally, in the "Inspector", delete the "Wall Border" terrain.

Minimal reproduction project

godot_4_test.zip

japp-ctapuk commented 1 year ago

I also got the same crash. And if you delete the entire TileSet and not the cells, then it's fine

mrcdk commented 1 year ago

I've got this crash backtrace while removing a terrain from a terrain set while painting them having the tileset editor painting mode opened. I built the engine from the same commit as 4.0-rc5

ERROR: Condition "plugins_list.has(p_plugin)" is true.
   at: add_plugin (editor/editor_node.cpp:8152)
ERROR: Condition "format != p_src->format" is true.
   at: blit_rect (core/io/image.cpp:2775)
ERROR: Condition "format != p_src->format" is true.
   at: blit_rect (core/io/image.cpp:2775)
ERROR: Condition "format != p_src->format" is true.
   at: blit_rect (core/io/image.cpp:2775)
ERROR: FATAL: Index p_index = 4 is out of bounds (count = 4).
   at: operator[] (./core/templates/local_vector.h:168)

================================================================
handle_crash: Program crashed with signal 4
Engine version: Godot Engine v4.0.rc.custom_build (3863199ab940272f6844ff30910ec7a520e47f41)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] /usr/lib/libc.so.6(+0x38f50) [0x7f0790a51f50] (??:0)
[2] /home/mrcdk/development/godot/godot/bin/godot.linuxbsd.editor.x86_64.llvm(+0x58ede2a) [0x557c9e77ce2a] (/home/mrcdk/development/godot/godot/scene/resources/tile_set.cpp:?)
[3] /home/mrcdk/development/godot/godot/bin/godot.linuxbsd.editor.x86_64.llvm(+0x49a8f04) [0x557c9d837f04] (/home/mrcdk/development/godot/godot/editor/plugins/tiles/tile_data_editors.cpp:1711)
[4] /home/mrcdk/development/godot/godot/bin/godot.linuxbsd.editor.x86_64.llvm(+0x49a9b9e) [0x557c9d838b9e] (/home/mrcdk/development/godot/godot/editor/plugins/tiles/tile_data_editors.cpp:?)
[5] /home/mrcdk/development/godot/godot/bin/godot.linuxbsd.editor.x86_64.llvm(+0x498da66) [0x557c9d81ca66] (/home/mrcdk/development/godot/godot/editor/plugins/tiles/tile_data_editors.cpp:60)
[6] /home/mrcdk/development/godot/godot/bin/godot.linuxbsd.editor.x86_64.llvm(+0x2876784) [0x557c9b705784] (/home/mrcdk/development/godot/godot/./core/variant/binder_common.h:?)
[7] /home/mrcdk/development/godot/godot/bin/godot.linuxbsd.editor.x86_64.llvm(+0x6d970ae) [0x557c9fc260ae] (/home/mrcdk/development/godot/godot/core/object/object.cpp:?)
[8] /home/mrcdk/development/godot/godot/bin/godot.linuxbsd.editor.x86_64.llvm(+0x69d6b15) [0x557c9f865b15] (/home/mrcdk/development/godot/godot/core/variant/callable.cpp:62)
[9] /home/mrcdk/development/godot/godot/bin/godot.linuxbsd.editor.x86_64.llvm(+0x6d8fbd5) [0x557c9fc1ebd5] (/home/mrcdk/development/godot/godot/core/object/message_queue.cpp:230)
[10] /home/mrcdk/development/godot/godot/bin/godot.linuxbsd.editor.x86_64.llvm(+0x6d8febc) [0x557c9fc1eebc] (/home/mrcdk/development/godot/godot/core/object/message_queue.cpp:?)
[11] /home/mrcdk/development/godot/godot/bin/godot.linuxbsd.editor.x86_64.llvm(+0x4b699a4) [0x557c9d9f89a4] (/home/mrcdk/development/godot/godot/scene/main/scene_tree.cpp:173)
[12] /home/mrcdk/development/godot/godot/bin/godot.linuxbsd.editor.x86_64.llvm(+0x2724784) [0x557c9b5b3784] (/home/mrcdk/development/godot/godot/main/main.cpp:3161)
[13] /home/mrcdk/development/godot/godot/bin/godot.linuxbsd.editor.x86_64.llvm(+0x26afb7d) [0x557c9b53eb7d] (/home/mrcdk/development/godot/godot/platform/linuxbsd/os_linuxbsd.cpp:880)
[14] /home/mrcdk/development/godot/godot/bin/godot.linuxbsd.editor.x86_64.llvm(+0x26a3e1c) [0x557c9b532e1c] (/home/mrcdk/development/godot/godot/platform/linuxbsd/godot_linuxbsd.cpp:75)
[15] /usr/lib/libc.so.6(+0x23790) [0x7f0790a3c790] (??:0)
[16] /usr/lib/libc.so.6(__libc_start_main+0x8a) [0x7f0790a3c84a] (??:0)
[17] /home/mrcdk/development/godot/godot/bin/godot.linuxbsd.editor.x86_64.llvm(+0x26a3c65) [0x557c9b532c65] (??:?)
-- END OF BACKTRACE --
================================================================

I'm not a 100% sure but I have the feeling that the crash comes from this function TileSet.generate_terrain_icons()

mrcdk commented 1 year ago

Okay, I boiled it down to this:

Say we start with 4 terrains in a terrain set and have setup our tiles using all the terrains in the set.

After deleting a terrain bit_counts gets initialized here with the number of terrains (3 terrains after deleting one) https://github.com/godotengine/godot/blob/3863199ab940272f6844ff30910ec7a520e47f41/scene/resources/tile_set.cpp#L1784-L1785

but here, when requesting the terrain from a tile_data, it may be possible for this value to be bigger than the size of bit_counts causing the crash https://github.com/godotengine/godot/blob/3863199ab940272f6844ff30910ec7a520e47f41/scene/resources/tile_set.cpp#L1789-L1791

I did change that code to:

if (tile_data->get_terrain() >= 0) {
    if (tile_data->get_terrain() >= (int)bit_counts.size()) {
        WARN_PRINT(vformat("Invalid tile data terrain: %d", tile_data->get_terrain()));
    } else {
        bit_counts[tile_data->get_terrain()] += 10;
    }
}

which solves the crash but I don't think this is the correct fix as it only warns. I think it'd be better to reset the tile_data.terrain to -1 when deleting a terrain from a terrain set and I'm not sure where that should happen.

Hopefully this helps to fix the issue.

godotengine / godot