BakedLightmap atlas size not respected

[grevius5]

Godot version

3.4.beta.3 mono

System information

Windows 10

Issue description

BakedLightmap node is not respecting the atlas size as you can see:

Also i remenber in the older version there was an error message that tells you that the size of the uv's were too big, in thet version i haven't see that error anymore.

Steps to reproduce

Bake a scene with 3d models imported with low lightmap texel size (for example 0.01) and light baking as "Gen Lightmaps". Then bake

Minimal reproduction project

No response

[Calinou]

The Atlas Max Size property likely defines the smaller dimension only. It could also mean that individual layers are being clamped in size, not all layers as a whole.

[JFonS]

Yes, Atlas Max Size refers to the individual layers, which are then stacked on top of each other to create a single image.

Right now there isn't any way to limit the total height of the atlas, so if you want to keep images below 2048 in both dimensions you will need to disable atlassing.

We could implement multi-image atlases, so instead of a single image for all layers, multiple images are used, but I don't really have the time right now. If anyone is interested, I'll be happy to give guidance on what needs to be changed.

[Calinou]

We could implement multi-image atlases, so instead of a single image for all layers, multiple images are used, but I don't really have the time right now. If anyone is interested, I'll be happy to give guidance on what needs to be changed.

For reference, support for multiple atlases in BakedLightmap is being tracked in godotengine/godot-proposals#2147.

[Koalamana9]

Yes, Atlas Max Size refers to the individual layers, which are then stacked on top of each other to create a single image.

Right now there isn't any way to limit the total height of the atlas, so if you want to keep images below 2048 in both dimensions you will need to disable atlassing.

We could implement multi-image atlases, so instead of a single image for all layers, multiple images are used, but I don't really have the time right now. If anyone is interested, I'll be happy to give guidance on what needs to be changed.

I don't understand why multi-image atlases is left behind in a backlog, this is so critically important for proper lightmapping setup with performance in mind.
Any relatively large interior static scene will greatly benefit from baked lightmaps that nicely packed in pow2 textures with fixed size, not in one huge atlas and not in thousands small textures, this is just ridiculous.
And no GIProbe is not an option for interior scenes with small details due to the poor quality, noticeable light leakage and unnecessary performance impact.

@JFonS What should be done to implement multi-image atlases? Instead of stacking on top of each other to create a single atlas we need to constrain each atlas to fixed width and height, and constantly check whether we fill one out completly? This just screams for an efficient UV packing algorithm...

[Calinou]

@KoxaKoxama JFonS is currently working on rendering features for the master branch, so he doesn't have time to dedicate to the CPU lightmapper.

As a workaround, you can increase the lightmap texel size on imported scenes to reduce the size of the generated textures. Lighting will probably still look good enough, especially if you only bake indirect lighting. Bicubic sampling (which is enabled by default on desktop) does a good job of hiding the aliasing that can be present on lightmaps. And if you do use fully baked lights, remember that sharper shadows are not always more realistic 🙂

[JFonS]

@KoxaKoxama There are many things that need our attention and we have to set priorities one way or another, we can't please everyone. Whether you think this is ridiculous or not doesn't really matter, so you can skip that part in future posts on Godot's issue tracker.

That said, I did look into this issue a while back, and I made some progress. The only thing preventing larger lightmaps is the fact that the Image resource has a maximum size.

As a quick fix I tried saving a TextureArray resource directly, but it appears that the saving code for this resource type was never implemented, only the loader was. Implementing this saver would take quite some time and testing, and the solution would not be ideal anyway, so I left it there.

There are two options moving forward:

• Implement the saver for TextureArray.
  This is my work-in-progress changes: diff.txt in case someone wants to continue working on it. But as I said, the solution is not ideal since images won't be compressed.

• Instead of saving a single image or a single texture array, add support for saving multiple images.
  Since the CPU lightmapper already generates a list of images, saving them separately should be easy, but this needs some changes in LightmapperCPU in order to keep track of which meshes ended un in which image. It's not a complicated process, but it takes some time and some careful testing to make sure everything keeps working as expected. I can provide more specific details if anyone is interested.

[Koalamana9]

@JFonS There is no need for an implementation of TextureArray saver.
Support for saving multiple images/atlases from lightmapper is exactly what I was wondering about, currently there is two extreme options how baked lightmaps are stored, it's either thousands of small textures or one unreasonably huge texture, both of which is harmful for performance, especially a very big non power of two atlas.

When texture atlas size is specified, lightmapper needs to keep texture size as pow2 and try to fill it with as much UV's as it can, and then create another texture and fill it with rest, repeat untill all mesh is done. A perfect middle ground.

This could be useful for GPU lightmapper in 4.0 as well, maybe this can uplift priorities a bit.

[JFonS]

As I mentioned, the TextrueArray approach was an attempt to get a quick fix. As I also mentioned, saving multiple images is possible, but requires a bit more work. After taking another look, I see the lightmapper already keeps track of the correspondence between meshes and images, so it's just a matter of correctly saving and loading the images. I will give it a go one of these days, when I have some spare time.

Note: the master branch has an image size limit of 2^24 (16.7M) instead of 2^14 (16.3k), so for 4.0 this is much less relevant.

[akien-mga]

Fixed by #58102.

[gdlq]

Fixed by #58102.

Not fixed at all. Same error as before Failed determining lightmap size. Maximum lightmap size too small?

[Calinou]

@gdlq See my comment in #58102: #58102 (comment)

Edit: Disregard the accidental reopen/close below.