Vulkan: LightmapGI does not take MultiMeshes into account for baking

Calinou commented 2 years ago

Related to https://github.com/godotengine/godot/issues/56024 which is for VoxelGI.

Godot version

4.0.dev (1cbf3947d)

System information

Fedora 34, GeForce GTX 1080 (NVIDIA 470.74)

Issue description

Similar to https://github.com/godotengine/godot/issues/48909, but with the new lightmap GI implementation in master.

LightmapGI does not take MultiMeshes into account for baking. On the left, you can clearly notice the indirect lighting, but it's completely missing on the right:

Left: Individual MeshInstances, right: MultiMeshInstance:

2021-12-17_18 09 28

Steps to reproduce

Note: An imported 3D scene with UV2 generated must be used. You cannot use primitive meshes for testing this, as they do not contain a UV2 layer and it can't be generated using the MeshInstance tools. The minimal reproduction project includes a ready-to-use setup for testing this.

Import a 3D scene with Static Lightmaps bake mode. Click Advanced in the import dock, go to the Meshes tab and enable Save to File. Specify a path to a .tres file to save.
Add a MeshInstance3D node with the mesh resource loaded to act as a floor. Scale it so it can fit several objects on top. Set the global illumination mode to Baked in the inspector.
Add a MeshInstance3D node with the mesh resource loaded to act as a source mesh for the MultiMesh. Set the global illumination mode to Baked in the inspector.
Add a MultiMeshInstance3D node. Use the menu at the top of the 3D editor viewport to populate the MultiMeshInstance with BoxMeshes.
Add a LightmapGI, select it and click Bake at the top of the 3D editor viewport.

Minimal reproduction project

test_lightmapgi_multimesh.zip

Old MRPs

~~[test_lightmapgi_multimesh.zip](https://github.com/godotengine/godot/files/7736682/test_lightmapgi_multimesh.zip)~~ *With `.godot/` included to workaround importing issue:* [test_lightmapgi_multimesh_1.zip](https://github.com/godotengine/godot/files/7777212/test_lightmapgi_multimesh_1.zip)

Calinou commented 4 weeks ago

I can still reproduce this as of 4.3.beta2. I updated the MRP in OP.

clayjohn commented 4 weeks ago

Probably needs the same solution as implemented in https://github.com/godotengine/godot/pull/81616/

Calinou commented 3 weeks ago

Probably needs the same solution as implemented in #81616

I was looking around and noticed that LightmapGI relies on get_bake_meshes() and not get_meshes(). This is done to support GridMap, and I noticed MultiMeshInstance has no equivalent (it only has get_meshes()).

I've tried replacing https://github.com/godotengine/godot/blob/374807f427eec5ee7caebfc509a158fe715a6bfe/scene/3d/lightmap_gi.cpp#L345 with the following:

Array bmeshes;
MultiMeshInstance3D *multi_mesh = Object::cast_to<MultiMeshInstance3D>(p_at_node);
if (multi_mesh) {
    bmeshes = multi_mesh->get_meshes();
} else {
    bmeshes = p_at_node->call("get_bake_meshes");
}

However, this doesn't result in any visual difference after baking, even though no errors are printed.

Do we need to reimplement get_bake_meshes() for MultiMeshInstance3D?

cc @bitsawer

godotengine / godot