search

godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
88.68k stars 20.11k forks source link

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

Closed DarioSamo closed 12 months ago

DarioSamo commented 12 months ago

Godot version

v4.2.dev.custom_build [8c1817f75]

System information

Godot v4.2.dev (8c1817f75) - 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 https://github.com/godotengine/godot/issues/54679, as this can be reproduced with much smaller values like 2K and 4K resolution lightmaps.

The issue originates from this particular error handling.

https://github.com/godotengine/godot/blob/8c1817f755b63a69378774d8d0f74499f663afe2/modules/lightmapper_rd/lightmapper_rd.cpp#L174-L179

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).

https://github.com/godotengine/godot/blob/8c1817f755b63a69378774d8d0f74499f663afe2/scene/3d/lightmap_gi.cpp#L1083-L1087

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 commented 12 months ago

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 commented 12 months ago

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.