LightmapGI causes crash when using --headless

[yahkr]

Tested versions

• Reproducible in 4.2.1.stable.mono
• Reproducible in 4.2.2.rc1.mono
• Reproducible in 4.3.dev4.mono

System information

Godot v4.2.1.stable.mono - Windows 10.0.22621 - Vulkan (Forward+)

Issue description

Attempting to run a basic project with a baked LightmapGI node causes this crash when using --headless.

Godot Engine v4.2.1.stable.mono.official.b09f793f5 - https://godotengine.org

Process finished with exit code -1,073,741,819.

Steps to reproduce

Create a scene with a LighmapGI node, bake the lightmap. Run the project with the --headless param.

Minimal reproduction project (MRP)

MRP.zip

[akien-mga]

I confirm the crash, tested on Linux. Stacktrace:

$ godot-git --headless
Godot Engine v4.3.dev.custom_build.f2045ba82 (2024-03-02 09:48:25 UTC) - https://godotengine.org

================================================================
handle_crash: Program crashed with signal 11
Engine version: Godot Engine v4.3.dev.custom_build (f2045ba822bff7d34964901393581a3117c394a9)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] /lib64/libc.so.6(+0x3e9a0) [0x7f82279ac9a0] (??:0)
[2] HashSet<RendererSceneCull::Instance*, HashMapHasherDefault, HashMapComparatorDefault<RendererSceneCull::Instance*> >::_insert(RendererSceneCull::Instance* const&) (/home/akien/Godot/godot/./core/templates/hash_set.h:170)
[3] HashSet<RendererSceneCull::Instance*, HashMapHasherDefault, HashMapComparatorDefault<RendererSceneCull::Instance*> >::insert(RendererSceneCull::Instance* const&) (/home/akien/Godot/godot/./core/templates/hash_set.h:408 (discriminator 1))
[4] RendererSceneCull::instance_geometry_set_lightmap(RID, RID, Rect2 const&, int) (/home/akien/Godot/godot/./servers/rendering/renderer_scene_cull.cpp:1475)
[5] RenderingServerDefault::instance_geometry_set_lightmap(RID, RID, Rect2 const&, int) (/home/akien/Godot/godot/./servers/rendering/rendering_server_default.h:833)
[6] LightmapGI::_assign_lightmaps() (/home/akien/Godot/godot/./scene/3d/lightmap_gi.cpp:1326 (discriminator 2))
[7] LightmapGI::_notification(int) (/home/akien/Godot/godot/./scene/3d/lightmap_gi.cpp:1313)
[8] LightmapGI::_notificationv(int, bool) (/home/akien/Godot/godot/./scene/3d/lightmap_gi.h:116 (discriminator 14))
[9] Object::notification(int, bool) (/home/akien/Godot/godot/./core/object/object.cpp:849)
[10] Node::_propagate_ready() (/home/akien/Godot/godot/./scene/main/node.cpp:258)
[11] Node::_propagate_ready() (/home/akien/Godot/godot/./scene/main/node.cpp:250 (discriminator 3))
[12] Node::_propagate_ready() (/home/akien/Godot/godot/./scene/main/node.cpp:250 (discriminator 3))
[13] Node::_set_tree(SceneTree*) (/home/akien/Godot/godot/./scene/main/node.cpp:3090)
[14] SceneTree::initialize() (/home/akien/Godot/godot/./scene/main/scene_tree.cpp:452)
[15] OS_LinuxBSD::run() (/home/akien/Godot/godot/platform/linuxbsd/os_linuxbsd.cpp:934)
[16] /home/akien/Godot/godot/bin/godot.linuxbsd.editor.dev.x86_64(main+0x15a) [0x2b15b10] (/home/akien/Godot/godot/platform/linuxbsd/godot_linuxbsd.cpp:88)
[17] /lib64/libc.so.6(+0x2814a) [0x7f822799614a] (??:0)
[18] /lib64/libc.so.6(__libc_start_main+0x8b) [0x7f822799620b] (??:0)
[19] /home/akien/Godot/godot/bin/godot.linuxbsd.editor.dev.x86_64(_start+0x25) [0x2b158f5] (??:?)
-- END OF BACKTRACE --
================================================================
Aborted (core dumped)

[akien-mga]

I confirm the crash, tested on Linux (master, f2045ba822bff7d34964901393581a3117c394a9). Stacktrace:

$ godot-git --headless
Godot Engine v4.3.dev.custom_build.f2045ba82 (2024-03-02 09:48:25 UTC) - https://godotengine.org

================================================================
handle_crash: Program crashed with signal 11
Engine version: Godot Engine v4.3.dev.custom_build (f2045ba822bff7d34964901393581a3117c394a9)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] /lib64/libc.so.6(+0x3e9a0) [0x7f82279ac9a0] (??:0)
[2] HashSet<RendererSceneCull::Instance*, HashMapHasherDefault, HashMapComparatorDefault<RendererSceneCull::Instance*> >::_insert(RendererSceneCull::Instance* const&) (/home/akien/Godot/godot/./core/templates/hash_set.h:170)
[3] HashSet<RendererSceneCull::Instance*, HashMapHasherDefault, HashMapComparatorDefault<RendererSceneCull::Instance*> >::insert(RendererSceneCull::Instance* const&) (/home/akien/Godot/godot/./core/templates/hash_set.h:408 (discriminator 1))
[4] RendererSceneCull::instance_geometry_set_lightmap(RID, RID, Rect2 const&, int) (/home/akien/Godot/godot/./servers/rendering/renderer_scene_cull.cpp:1475)
[5] RenderingServerDefault::instance_geometry_set_lightmap(RID, RID, Rect2 const&, int) (/home/akien/Godot/godot/./servers/rendering/rendering_server_default.h:833)
[6] LightmapGI::_assign_lightmaps() (/home/akien/Godot/godot/./scene/3d/lightmap_gi.cpp:1326 (discriminator 2))
[7] LightmapGI::_notification(int) (/home/akien/Godot/godot/./scene/3d/lightmap_gi.cpp:1313)
[8] LightmapGI::_notificationv(int, bool) (/home/akien/Godot/godot/./scene/3d/lightmap_gi.h:116 (discriminator 14))
[9] Object::notification(int, bool) (/home/akien/Godot/godot/./core/object/object.cpp:849)
[10] Node::_propagate_ready() (/home/akien/Godot/godot/./scene/main/node.cpp:258)
[11] Node::_propagate_ready() (/home/akien/Godot/godot/./scene/main/node.cpp:250 (discriminator 3))
[12] Node::_propagate_ready() (/home/akien/Godot/godot/./scene/main/node.cpp:250 (discriminator 3))
[13] Node::_set_tree(SceneTree*) (/home/akien/Godot/godot/./scene/main/node.cpp:3090)
[14] SceneTree::initialize() (/home/akien/Godot/godot/./scene/main/scene_tree.cpp:452)
[15] OS_LinuxBSD::run() (/home/akien/Godot/godot/platform/linuxbsd/os_linuxbsd.cpp:934)
[16] /home/akien/Godot/godot/bin/godot.linuxbsd.editor.dev.x86_64(main+0x15a) [0x2b15b10] (/home/akien/Godot/godot/platform/linuxbsd/godot_linuxbsd.cpp:88)
[17] /lib64/libc.so.6(+0x2814a) [0x7f822799614a] (??:0)
[18] /lib64/libc.so.6(__libc_start_main+0x8b) [0x7f822799620b] (??:0)
[19] /home/akien/Godot/godot/bin/godot.linuxbsd.editor.dev.x86_64(_start+0x25) [0x2b158f5] (??:?)
-- END OF BACKTRACE --
================================================================
Aborted (core dumped)

Edit: Also reproducible today (April 5) with 7e4c150573d6af7072f2b55ae76dc7b723f21f66.

[akien-mga]

This works around the crash, but it's not a full fix of course.

diff --git a/servers/rendering/renderer_scene_cull.cpp b/servers/rendering/renderer_scene_cull.cpp
index 59d70958f1..0b7e5c2dbe 100644
--- a/servers/rendering/renderer_scene_cull.cpp
+++ b/servers/rendering/renderer_scene_cull.cpp
@@ -1471,6 +1471,7 @@ void RendererSceneCull::instance_geometry_set_lightmap(RID p_instance, RID p_lig

    if (lightmap_instance) {
        InstanceLightmapData *lightmap_data = static_cast<InstanceLightmapData *>(lightmap_instance->base_data);
+       ERR_FAIL_NULL(lightmap_data);
        lightmap_data->users.insert(instance);
        lightmap_instance_rid = lightmap_data->instance;
    }

[jamie-pate]

When calling LightmapGI::set_light_data() -> set_base() the rid is 0 when headless..

I think it'd be safe to just ignore lightmaps in headless mode as if there was no lightmap_instance?

if (lightmap_instance) {
  InstanceLightmapData *lightmap_data = static_cast<InstanceLightmapData *>(lightmap_instance->base_data);

  if (lightmap_data) {
      lightmap_data->users.insert(instance);
      lightmap_instance_rid = lightmap_data->instance;
  }
}

Edit: discussion on the PR requests an approach that stubs the lightmap_instance->base_data in the dummy/headless renderer instead of hacking in null checks for every type of data that may be missing there.

[jamie-pate]

similar: #92412

[clayjohn]

Just a note for interested contributors. The way to fix this will be to store a dummy RID for lightmap instances in the DummyRenderer

[jamie-pate]

Still mystified by the RID shadow resource system. Is there a walkthrough on how it works and why it exists? :D

That or examples of dummy RID for other resource types.

[clayjohn]

@jamie-pate We don't have a walkthrough or anything. Basically, the Dummy Server is an implementation of the RenderingServer that doesn't draw anything. It is used for --headless mode because --headless mode never needs to draw anything.

All the functions that RenderingServer exposes are also in the DummyServer, but in most cases they are empty, so they are just no-ops. However, in some cases, they need to return a value, so they return some default value. In some other cases, a real value is needed so the DummyServer needs to save a real value and then return it.

This is the case for anything with an RID. The DummyServer should be generating an RID, holding on to it, and then returning it when requested.

Particularly, the problem is here: https://github.com/godotengine/godot/blob/4e5ed0bbfb56f0a71eb61c868f965476652c23df/servers/rendering/dummy/storage/light_storage.h#L170

All lightmap instances get a default RID set to RID(), so when we later try to read back data we get null references.

We need to implement lightmap_instance_create() and lightmap_instance_free(). They can just be copied from the regular renderer here:

https://github.com/godotengine/godot/blob/4e5ed0bbfb56f0a71eb61c868f965476652c23df/servers/rendering/renderer_rd/storage_rd/light_storage.cpp#L1968-L1976

You also may need to do it for a few other functions that return RIDs. I'm not sure exactly which ones are necessary.

Here is an example of how the above looks in a PR https://github.com/godotengine/godot/commit/ed2b3d358d3883b98f741cbfc1ca3d7aa4fcbb7b

[jamie-pate]

I get that the RID system is an abstraction layer, but it kind of seems like throwing around a bunch of (void*) pointers everywhere from the outside.

having to implement stuff like this feel bad :D

    virtual RS::InstanceType get_base_type(RID p_rid) const override {
        if (RendererDummy::MeshStorage::get_singleton()->owns_mesh(p_rid)) {
            return RS::INSTANCE_MESH;
        } else if (RendererDummy::MeshStorage::get_singleton()->owns_multimesh(p_rid)) {
            return RS::INSTANCE_MULTIMESH;
        } else if (RendererDummy::LightStorage::get_singleton()->owns_lightmap(p_rid)) {
            return RS::INSTANCE_LIGHTMAP;
        }
        return RS::INSTANCE_NONE;
    }

Feels like abstract classes or some other kind of typed opaque data structure would be nicer than completely opaque RID handles (effectively a (void *) within special memory blocks that identify the pointer type.. i guess.)

(I think i'm progressing towards to a solution at least)