TileSet/TileMap rework

[groud]

This is a first step in the TileSet/TileMap rework. It includes:

• A rework of the TileMap and TileSet nodes. Most properties have been moved to the TileSet resource, they better handle shapes (true support for hexagonal and isometric tiles),
• The TileSet editor supports only atlases for now (support for scenes will be added later on),
• For each tile in an atlas, you can create "alternative tiles", they will share the same part of the texture but you may assign them different properties (not yet implemented).

Disclaimer: the API is not ready yet and a lot of feature are missing.

Some screenshots for pleasure:

Download the builds (produced by CI):

Linux with mono: https://github.com/godotengine/godot/suites/1851943757/artifacts/35926599
Windows: https://github.com/godotengine/godot/suites/1851943754/artifacts/35925761
MacOs: https://github.com/godotengine/godot/suites/1851943758/artifacts/35926056

WARNING: opening an existing project with this build will break your TileMaps/TileSets resources for any other version of Godot. Make a backup of your project if you wish to try it on existing projects.

This is the place to provide feedback on this build. Please report here any crash/errors.
I don't want to give any documentation for now on how to use the editors yet, to see if it is intuitive enough. ;)

[qarmin]

I have crash when trying to open empty project with Address Sanitizer(this errors probably means that uninitialised variable was used)

scene/2d/tile_map.cpp:75:9: runtime error: member access within misaligned address 0xbebebebebebebebe for type 'const struct TileMap', which requires 8 byte alignment
0xbebebebebebebebe: note: pointer points here
<memory cannot be printed>
scene/2d/tile_map.cpp:75:9: runtime error: reference binding to misaligned address 0xbebebebebebec1de for type 'const struct Ref', which requires 8 byte alignment
0xbebebebebebec1de: note: pointer points here
<memory cannot be printed>
scene/2d/tile_map.cpp:75:9: runtime error: member access within misaligned address 0xbebebebebebebebe for type 'const struct TileMap', which requires 8 byte alignment
0xbebebebebebebebe: note: pointer points here
<memory cannot be printed>
core/object/reference.h:61:14: runtime error: member access within misaligned address 0xbebebebebebec1de for type 'const struct Ref', which requires 8 byte alignment
0xbebebebebebec1de: note: pointer points here
<memory cannot be printed>
handle_crash: Program crashed with signal 11
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[1] bin/godot.linuxbsd.tools.64s() [0x1e180fa] (/mnt/Miecz/mojgodot/platform/linuxbsd/crash_handler_linuxbsd.cpp:54)
[2] /lib/x86_64-linux-gnu/libc.so.6(+0x46210) [0x7f87592a6210] (??:0)
[3] Ref<TileSet>::ref(Ref<TileSet> const&) (/mnt/Miecz/mojgodot/./core/object/reference.h:61)
[4] Ref<TileSet>::Ref(Ref<TileSet> const&) (/mnt/Miecz/mojgodot/./core/object/reference.h:178)
[5] TileMap::get_tileset() const (/mnt/Miecz/mojgodot/scene/2d/tile_map.cpp:76)
[6] TileMapEditor::_update_bottom_panel() (/mnt/Miecz/mojgodot/editor/plugins/tiles/tile_map_editor.cpp:81)
[7] TileMapEditor::TileMapEditor() (/mnt/Miecz/mojgodot/editor/plugins/tiles/tile_map_editor.cpp:1012)
[8] TilesEditor::TilesEditor(EditorNode*) (/mnt/Miecz/mojgodot/editor/plugins/tiles/tiles_editor_plugin.cpp:201 (discriminator 2))
[9] TilesEditorPlugin::TilesEditorPlugin(EditorNode*) (/mnt/Miecz/mojgodot/editor/plugins/tiles/tiles_editor_plugin.cpp:253 (discriminator 2))
[10] EditorNode::EditorNode() (/mnt/Miecz/mojgodot/editor/editor_node.cpp:6607 (discriminator 1))
[11] Main::start() (/mnt/Miecz/mojgodot/main/main.cpp:2139 (discriminator 1))
[12] bin/godot.linuxbsd.tools.64s(main+0x32f) [0x1e15df5] (/mnt/Miecz/mojgodot/platform/linuxbsd/godot_linuxbsd.cpp:57)
[13] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3) [0x7f87592870b3] (??:0)
[14] bin/godot.linuxbsd.tools.64s(_start+0x2e) [0x1e15a0e] (??:?)
-- END OF BACKTRACE --
Przerwane (zrzut pamięci)

[Xrayez]

Should #43916 be closed or merged then?

[groud]

I have crash when trying to open empty project with Address Sanitizer(this errors probably means that uninitialised variable was used)

I cannot reproduce. Could you give me the exact scons args you use and the steps to reproduce the bug?
I am not sure which variable would be uninitialized. (tile_map is likely initialized automatically to nullptr, and the Ref is correctly checked with is_valid())

[qarmin]

This contains leak, address and undefined sanitizers:

 scons p=linuxbsd -j7 use_asan=yes use_ubsan=yes;

[qarmin]

I found that this is caused by non initializing tilemap - this patch fixes issue

diff --git a/editor/plugins/tiles/tile_map_editor.h b/editor/plugins/tiles/tile_map_editor.h
index 3c8af2a19e..b9466d7103 100644
--- a/editor/plugins/tiles/tile_map_editor.h
+++ b/editor/plugins/tiles/tile_map_editor.h
@@ -84,7 +84,7 @@ private:
        void _tile_alternatives_control_gui_input(const Ref<InputEvent> &p_event);
 
        // --- TileMap ---
-       TileMap *tile_map;
+       TileMap *tile_map = nullptr;
 
        HBoxContainer *tilemap_toolbar;
 
diff --git a/editor/plugins/tiles/tiles_editor_plugin.h b/editor/plugins/tiles/tiles_editor_plugin.h
index 4e5e59bddb..35ce608b6e 100644
--- a/editor/plugins/tiles/tiles_editor_plugin.h
+++ b/editor/plugins/tiles/tiles_editor_plugin.h
@@ -44,7 +44,7 @@ class TilesEditor : public VBoxContainer {
        static TilesEditor *singleton;
 
 private:
-       TileMap *tile_map;
+       TileMap *tile_map = nullptr;
        Ref<TileSet> tile_set;
 
        Button *tileset_tilemap_switch_button;

[groud]

I found that this is caused by non initializing tilemap - this patch fixes issue

Ah ok. I though pointers were supposed to be initialized automatically to nullptr. That's weird.
Ok I'll patch this then, thanks!

[qarmin]

When opening and closing editor with this project - Temp Projekty.zip I have this read after free:
(tileset_tab_sources is already deleted, but tileset wants to use it again in destructor)

==244823==ERROR: AddressSanitizer: heap-use-after-free on address 0x61b000172990 at pc 0x000002a124e3 bp 0x7ffc892fd530 sp 0x7ffc892fd520
READ of size 8 at 0x61b000172990 thread T0
    #0 0x2a124e2 in Object::_change_notify(char const*) core/object/object.h:562
    #1 0xfcee69a in TileSet::remove_atlas_source(int) scene/resources/tile_set/tile_set.cpp:1097
    #2 0xfd07aad in TileSet::~TileSet() scene/resources/tile_set/tile_set.cpp:1400
    #3 0xb1d9642 in void memdelete<TileSet>(TileSet*) core/os/memory.h:115
    #4 0xb1d3584 in Ref<TileSet>::unref() core/object/reference.h:221
    #5 0xb1d1cfc in Ref<TileSet>::~Ref() core/object/reference.h:233
    #6 0xb1c836c in TilesEditor::~TilesEditor() editor/plugins/tiles/tiles_editor_plugin.cpp:222
    #7 0x2c007d9 in void memdelete<Node>(Node*) core/os/memory.h:115
    ...
    #125 0x1360312a in predelete_handler(Object*) core/object/object.cpp:1824
    #126 0xc7642fd in void memdelete<Window>(Window*) core/os/memory.h:111
    #127 0xc72316b in SceneTree::finalize() scene/main/scene_tree.cpp:527
    #128 0x1e23390 in OS_LinuxBSD::run() platform/linuxbsd/os_linuxbsd.cpp:266
    #129 0x1e15ec1 in main platform/linuxbsd/godot_linuxbsd.cpp:58
    #130 0x7f112c03e0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
    #131 0x1e15a0d in _start (/mnt/Miecz/mojgodot/bin/godot.linuxbsd.tools.64s+0x1e15a0d)

0x61b000172990 is located 16 bytes inside of 1584-byte region [0x61b000172980,0x61b000172fb0)
freed by thread T0 here:
    #0 0x7f112d2711b7 in __interceptor_free (/lib/x86_64-linux-gnu/libasan.so.6+0xb01b7)
    #1 0x12506880 in Memory::free_static(void*, bool) core/os/memory.cpp:169
    #2 0x2c007ea in void memdelete<Node>(Node*) core/os/memory.h:118
    #3 0xc62d644 in Node::_notification(int) scene/main/node.cpp:169
    #4 0x2bbd20f in Node::_notificationv(int, bool) scene/main/node.h:45
    #5 0x2bbff7a in CanvasItem::_notificationv(int, bool) scene/main/canvas_item.h:164
    #6 0x2bc2664 in Control::_notificationv(int, bool) scene/gui/control.h:47
    #7 0x2bc6fe2 in Container::_notificationv(int, bool) scene/gui/container.h:37
    #8 0xd3fd094 in TabContainer::_notificationv(int, bool) scene/gui/tab_container.h:39
    #9 0x135dad24 in Object::notification(int, bool) core/object/object.cpp:793
    #10 0x135cd75d in Object::_predelete() core/object/object.cpp:354
    #11 0x1360312a in predelete_handler(Object*) core/object/object.cpp:1824
    #12 0x2c0061b in void memdelete<Node>(Node*) core/os/memory.h:111
    #13 0xc62d644 in Node::_notification(int) scene/main/node.cpp:169
    #14 0x2bbd20f in Node::_notificationv(int, bool) scene/main/node.h:45
    #15 0x2bbff7a in CanvasItem::_notificationv(int, bool) scene/main/canvas_item.h:164
    #16 0x2bc2664 in Control::_notificationv(int, bool) scene/gui/control.h:47
    #17 0x2bc6fe2 in Container::_notificationv(int, bool) scene/gui/container.h:37
    #18 0x2bc97d0 in BoxContainer::_notificationv(int, bool) scene/gui/box_container.h:37
    #19 0x2bcf974 in VBoxContainer::_notificationv(int, bool) scene/gui/box_container.h:78
    #20 0xc1fbb4e in TileSetEditor::_notificationv(int, bool) editor/plugins/tiles/tile_set_editor.h:40
    #21 0x135dad24 in Object::notification(int, bool) core/object/object.cpp:793
    #22 0x135cd75d in Object::_predelete() core/object/object.cpp:354
    #23 0x1360312a in predelete_handler(Object*) core/object/object.cpp:1824
    #24 0x2c0061b in void memdelete<Node>(Node*) core/os/memory.h:111
    #25 0xc62d644 in Node::_notification(int) scene/main/node.cpp:169
    #26 0x2bbd20f in Node::_notificationv(int, bool) scene/main/node.h:45
    #27 0x2bbff7a in CanvasItem::_notificationv(int, bool) scene/main/canvas_item.h:164
    #28 0x2bc2664 in Control::_notificationv(int, bool) scene/gui/control.h:47
    #29 0x2bc6fe2 in Container::_notificationv(int, bool) scene/gui/container.h:37

previously allocated by thread T0 here:
    #0 0x7f112d271517 in malloc (/lib/x86_64-linux-gnu/libasan.so.6+0xb0517)
    #1 0x12505973 in Memory::alloc_static(unsigned long, bool) core/os/memory.cpp:76
    #2 0x12505872 in operator new(unsigned long, char const*) core/os/memory.cpp:41
    #3 0xc1f7335 in TileSetEditor::TileSetEditor() editor/plugins/tiles/tile_set_editor.cpp:52
    #4 0xb1c6e61 in TilesEditor::TilesEditor(EditorNode*) editor/plugins/tiles/tiles_editor_plugin.cpp:211
    #5 0xb1c9d24 in TilesEditorPlugin::TilesEditorPlugin(EditorNode*) editor/plugins/tiles/tiles_editor_plugin.cpp:253
    #6 0x87d9d11 in EditorNode::EditorNode() editor/editor_node.cpp:6607
    #7 0x1f61f41 in Main::start() main/main.cpp:2139
    #8 0x1e15df4 in main platform/linuxbsd/godot_linuxbsd.cpp:57
    #9 0x7f112c03e0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)

SUMMARY: AddressSanitizer: heap-use-after-free core/object/object.h:562 in Object::_change_notify(char const*)
 value after free

[qarmin]

This patch fixes invalid read(I don't know if it is proper, but works)

diff --git a/editor/plugins/tiles/tile_set_editor_sources_tab.cpp b/editor/plugins/tiles/tile_set_editor_sources_tab.cpp
index bdb0ac5ac5..858b7b7b3a 100644
--- a/editor/plugins/tiles/tile_set_editor_sources_tab.cpp
+++ b/editor/plugins/tiles/tile_set_editor_sources_tab.cpp
@@ -1814,6 +1814,7 @@ TileSetEditorSourcesTab::TileSetEditorSourcesTab() {
 }
 
 TileSetEditorSourcesTab::~TileSetEditorSourcesTab() {
+       edit(nullptr);
        memdelete(tile_proxy_object);
        memdelete(alternative_tile_proxy_object);
        memdelete(atlas_source_proxy_object);

[qarmin]

Also drag_type variable isn't initialized in constructor TileMapEditor::TileMapEditor() but it is used.

[groud]

Thanks again! I'll apply those patch.

[groud]

@qarmin I updated the PR, could you try again and see if most of your problems are solved?

[KoBeWi]

I gave the new editor a brief try and it's already much better than the old one. Fixed palette and multi-tile drawing is a life-changer for me.
My biggest gripe is that RMB doesn't erase tiles. I hope it's already planned, because switching to Eraser to remove something is a chore.

From other stuff (some of which is probably known/planned):

• TileMap doesn't seem to use tile size. It always draws with 16 pixels. Creating atlas with different size works fine.
• When you click TileMap with TileSet for the first time, it will show the atlas contents, but you can't select it, because atlas is not selected
  
• It's not really apparent that you can zoom the tileset view. I was going to report that zoom would be useful, but then tried Ctrl + Wheel and got surprised
• Holding Shift should switch to line tool (like in old TileMap)
• It would be useful if the faded grid was drawn over bigger area. E.g. if you want to draw a pitfall, the grid won't reach the other side, so it's difficult to judge distance
  
  maybe it should just extend to the preview?

Great work so far.

[groud]

My biggest gripe is that RMB doesn't erase tiles. I hope it's already planned, because switching to Eraser to remove something is a chore.

I am not against it, the main problem for me right now is that only right click works for me to move the view (I don't know why but middle mouse button to pan is not working well on my side).

When you click TileMap with TileSet for the first time, it will show the atlas contents, but you can't select it, because atlas is not selected.

Yeah, this is a bug I have to fix. If nothing is selected on the left the atlas should not be displayed. I'll fix this.

It's not really apparent that you can zoom the tileset view. I was going to report that zoom would be useful, but then tried Ctrl + Wheel and got surprised

Ah yeah, I should add the zoom buttons. :)

Holding Shift should switch to line tool

Make sense, though I might deal with that kind of things later on. There might be a lot of different features people might like with modifier keys (maybe the rect tool is used more often than the line tool for example). I think it's better to wait for now to see what might be the best solution.

It would be useful if the faded grid was drawn over bigger area. E.g. if you want to draw a pitfall, the grid won't reach the other side, so it's difficult to judge distance

Hmm... Maybe this should be made configurable, as this might depend on the game you are working on. But that's a good point. We might draw the grid around the preview, but it cannot be extended to there. There is a limit on how many lines we can draw.

[KoBeWi]

We might draw the grid around the preview, but it cannot be extended to there. There is a limit on how many lines we can draw.

But doesn't the grid extend anyways when you click and draw the tile at that position? My biggest worry would be that rapid mouse movements would cause lots of redrawing, but almost the same happens when you draw lots of tiles.

[groud]

But doesn't the grid extend anyways when you click and draw the tile at that position

No, there a limit on how many tiles shapes are drawn anyway, otherwise there would be significant slowdowns. The solution tries to always draw as much tiles as possible in the screen, and prioritizes the tiles in the center.

Btw I forgot this question:

TileMap doesn't seem to use tile size. It always draws with 16 pixels. Creating atlas with different size works fine.

That's weird. The base tile size is to be modified in the inspector though (when editing the TileSet resource), did you modify this value? Because the tile size value in the atlas is only for rendering purposes (size of the texture region), this is needed for isometric tiles for examples, where usually tiles in the texture will have a different size from the one in the grid.

[KoBeWi]

No, there a limit on how many tiles shapes are drawn anyway, otherwise there would be significant slowdowns. The solution tries to always draw as much tiles as possible in the screen, and prioritizes the tiles in the center.

But I mean this:

What if grid extended to cursor before clicking? In the end the effect is the same (or isn't?).

The base tile size is to be modified in the inspector though (when editing the TileSet resource), did you modify this value?

I only see quadrant size and changing it doesn't have any visible effect.

EDIT:

I am not against it, the main problem for me right now is that only right click works for me to move the view (I don't know why but middle mouse button to pan is not working well on my side).

I just checked and Space + LMB works fine for panning too.

[groud]

What if grid extended to cursor before clicking? In the end the effect is the same (or isn't?)

If you unzoom a lot this is what happen, not to draw too many lines:

It keeps the lines drawn in the middle of the viewport. I think drawing around the previews is a better idea. It can be drawn into a dedicated canvas item to avoid redrawing the whole grid too.

I only see quadrant size and changing it doesn't have any visible effect.

You have to edit the TileSet resource by clicking on it:

I just checked and Space + LMB works fine for panning too.

Ah yes. I'm ok to allow using for right click to erase but I think we should then remove right-click to pane (and eventually make the middle mouse button work again). Because otherwise people might not like not to be able to pan with right click anymore when a TileMap is selected.

[KoBeWi]

You have to edit the TileSet resource by clicking on it:

Ah right, I didn't expect this. But maybe I just had wrong assumptions about the new system 🙃

This led me to discover another bug: when you change tile size in the inspector, the currently drawn tiles aren't updated until you reload the scene.

I think we should then remove right-click to pane (and eventually make the middle mouse button work again)

But middle mouse works for me, so it's either system-specific bug or just something on your side.

IMO it would be fine if tilemap editor prevented right-click panning. (although removing it entirely isn't a bad idea, because right now we have 3 ways of panning)

[groud]

This led me to discover another bug: when you change tile size in the inspector, the currently drawn tiles aren't updated until you reload the scene.

Ah. I fixed a similar bug long ago. It seems it appeared again. I'll fix it.

But middle mouse works for me, so it's either system-specific bug or just something on your side.

Ah I had never checked in other programs, but it seems that I have bad behaviors in Firefox too. I'll investigate that on my side then.

IMO it would be fine if tilemap editor prevented right-click panning. (although removing it entirely isn't a bad idea, because right now we have 3 ways of panning)

Yeah, I think that for consistency it would be better. An it would allow use to use right click for more context-aware things I guess. The current situation is kind of wasting a button.

[groud]

Alright, I pushed some bugfixes and it now draws the grid around the preview.

[YuriSizov]

Regarding that last visual bug with content drawn outside of the scroll container: it's likely this issue #36164.

[groud]

Closing as superseded by #48535