Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lightmap baking crashes if the size required by the mesh is bigger than the size allowed by LightmapGI. #81453

Closed
Tracked by #56033
DarioSamo opened this issue Sep 8, 2023 · 2 comments · Fixed by #81543

Comments

@DarioSamo
Copy link
Contributor

DarioSamo commented Sep 8, 2023

Godot version

v4.2.dev.custom_build [8c1817f]

System information

Godot v4.2.dev (8c1817f) - Windows 10.0.22621 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3090 Ti (NVIDIA; 31.0.15.3699) - AMD Ryzen 9 7950X 16-Core Processor (32 Threads)

Issue description

NOTE: This is a separate issue from #54679, as this can be reproduced with much smaller values like 2K and 4K resolution lightmaps.

The issue originates from this particular error handling.

int max = nearest_power_of_2_templated(atlas_size.width);
max = MAX(max, nearest_power_of_2_templated(atlas_size.height));
if (max > p_max_texture_size) {
return BAKE_ERROR_LIGHTMAP_TOO_SMALL;
}

When this is true, the following code fails to catch the error correctly and will crash afterwards due to the data being invalid (Notice how none of the code references this particular error case).

godot/scene/3d/lightmap_gi.cpp

Lines 1083 to 1087 in 8c1817f

Lightmapper::BakeError bake_err = lightmapper->bake(Lightmapper::BakeQuality(bake_quality), use_denoiser, bounces, bias, max_texture_size, directional, Lightmapper::GenerateProbes(gen_probes), environment_image, environment_transform, _lightmap_bake_step_function, &bsud, exposure_normalization);
if (bake_err == Lightmapper::BAKE_ERROR_LIGHTMAP_CANT_PRE_BAKE_MESHES) {
return BAKE_ERROR_MESHES_INVALID;
}

It seems it was intended at some point to consider the error but it doesn't seem to account for it.

I've opened an issue to discuss what solution would be preferable:

  1. We handle the error correctly, propagate it as best as possible and indicate to the user the exact problem instead of crashing.

  2. We handle the error silently and clamp the light-map size to the max dimensions allowed. A console warning could be an interesting solution here. This would personally be my preferred behavior as I wouldn't want to have to fix the scaling in every mesh I've re-imported just to test a low resolution bake, which can be fairly common to iterate on lighting before doing the final bake.

Steps to reproduce

  1. Set LightmapGI's Max Texture Size to a small value (e.g. 2048).
  2. Attempt to bake the lightmap for a mesh with a big enough size and texel size so that a texture bigger than LightmapGI's Max Texture Size is required.

Minimal reproduction project

N/A

@Calinou
Copy link
Member

Calinou commented Sep 8, 2023

I think it makes the most sense to clamp the dimension automatically, with a note in the class reference (in GeometryInstance3D.lightmap_size_hin t and LightmapGI.max_texture_size).

A warning on bake may prove annoying if it occurs every time you bake lightmaps, and it's not a scenario one may encounter very often in the first place.

@DarioSamo
Copy link
Contributor Author

Turns out the second suggestion is not that simple.

The packing algorithm requires the size of the texture to be big enough to fit all the mesh instances inside of it. If the mesh instance requires a lightmap that is too big and the maximum texture size clamps it silently, it'll be stuck in an infinite loop attempting to place the lightmap rectangle into a small texture that it can't fit.

In these cases it'd be necessary to scale down the lightmap itself, but I get the feeling it won't be that simple either as other parts of the rendering pipeline rely on this texel size to be correct.

The most reasonable thing to do right now would be to handle the error so at least the application doesn't crash.

@akien-mga akien-mga added this to the 4.2 milestone Sep 11, 2023
YuriSizov pushed a commit to YuriSizov/godot that referenced this issue Sep 20, 2023
…small.

Add error handling for BAKE_ERROR_LIGHTMAP_TOO_SMALL, which was previously ignored. Fixes godotengine#81453.

(cherry picked from commit 7dfb854)
mandryskowski pushed a commit to mandryskowski/godot that referenced this issue Oct 11, 2023
…small.

Add error handling for BAKE_ERROR_LIGHTMAP_TOO_SMALL, which was previously ignored. Fixes godotengine#81453.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants