Changes to Primitive Meshes Texel Size project setting do not apply until the scene is reloaded

Calinou commented 1 month ago

Tested versions

Reproducible in: 4.2.2.stable, 4.3.beta 92c8e87cd

System information

Godot v4.3.beta (92c8e87cd) - Fedora Linux 40 (KDE Plasma) - X11 - GLES3 (Compatibility) - NVIDIA GeForce RTX 4090 (nvidia; 555.58.02) - 13th Gen Intel(R) Core(TM) i9-13900K (32 Threads)

Issue description

Changes to lightmapping's Primitive Meshes Texel Size project setting do not apply until the scene is reloaded. This requires you to restart the editor or use Scene > Reload Saved Scene after saving the current scene to be able to use the updated texel size for LightmapGI baking.

We should listen to the project_settings changed signal somewhere and regenerate all PrimitiveMeshes when that particular project setting's value changes.

cc @BastiaanOlij

Steps to reproduce

Create a MeshInstance3D with a BoxMesh. Enable Add UV2 in the BoxMesh's properties.
Add a DirectionalLight3D and set its global illumination mode to Static (so that lightmap texel density is easier to notice).
Add a LightmapGI node and bake lightmaps.
Change Primitive Meshes > Texel Size in the Project Settings and bake lightmaps again.
Notice how the lightmap resolution did not change.
Use Scene > Reload Saved Scene. Notice how the lightmap layout breaks due to the new UV2 size (this is expected behavior). You can now bake lightmaps again and see lightmaps with updated texel density.

Minimal reproduction project (MRP)

N/A

clayjohn commented 1 month ago

I would just change it to a GLOBAL_DEF_RST property instead. The situation is a bit awkward since the texel size, once changed, will apply to all new or changed meshes.

Ultimately, since this really only impacts editor stuff, I think it is best to take the simplest solution so we don't have to add a bunch of complexity to the PrimitiveMeshes for a feature that won't even be useful at run time.

Calinou commented 1 month ago

I would just change it to a GLOBAL_DEF_RST property instead.

It doesn't actually require an editor restart, just a reload of the current scene. That's why I feel GLOBAL_DEF_RST() would be overkill here, as it would lead to users thinking they have to restart the editor to see changes (which is quite a bit slower than just reloading the scene).

There could be a dedicated hint for project settings that require a scene reload to apply, but that would also be a fair amount of changes to implement.

BastiaanOlij commented 1 month ago

Looks pretty straight forward, I think if we retrieve the texel size in the constructor on PrimitiveMesh and connect to the ProjectSettings.settings_changed signal, we can easily react to it:

So basically:

void PrimitiveMesh::_on_settings_changed() {
    float new_texel_size = GLOBAL_GET("rendering/lightmapping/primitive_meshes/texel_size");
    if (texel_size != new_texel_size) {
        texel_size = new_texel_size;
        _update_lightmap_size();
        request_update();
    }
}

I'll write up a PR later tonight.

godotengine / godot