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

[3.x] Add support for saving multiple Images in BakedLightmap #58102

Merged
merged 1 commit into from
Feb 15, 2022

Conversation

JFonS
Copy link
Contributor

@JFonS JFonS commented Feb 14, 2022

Instead of fitting all atlas slices into a single image, which meant there was a hard limit on the size, BakedLightmap will now save as many images as needed to fit all the slices generated by the lightmapper.

I only did some quick testing on a single scene, so it would be good to get this tested on more scenarios before merging.

The size limit is much less of an issue in master since the maximum size for images is much larger there. Still, once we see everything is working fine in 3.x, this can be forward-ported to master.

Closes #51741, implements godot-proposals#2147.

@Calinou
Copy link
Member

Calinou commented Feb 14, 2022

I tried this on Reflection, but the resulting bake looks broken:

2022-02-14_22 11 26

When atlas generation is disabled, it looks correct:

2022-02-14_22 46 28

Testing project: test_lightmap_large_3.x.zip
Baking lightmaps can take a while on this scene, so I recommend setting bake quality to Low, setting the number of bounces to 1 and disabling the denoiser.

@JFonS
Copy link
Contributor Author

JFonS commented Feb 15, 2022

I force-pushed a fix for @Calinou's issue, everything should be working now.

rects[i].was_packed = !(rects[i].x == STBRP__MAXVAL && rects[i].y == STBRP__MAXVAL);
// -- GODOT start --
// rects[i].was_packed = !(rects[i].x == STBRP__MAXVAL && rects[i].y == STBRP__MAXVAL);
rects[i].was_packed = !(rects[i].x == (int) STBRP__MAXVAL && rects[i].y == (int) STBRP__MAXVAL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you PR that fix upstream too?

@@ -36,6 +36,7 @@
#include "thirdparty/misc/clipper.hpp"
#include "thirdparty/misc/triangulator.h"
#define STB_RECT_PACK_IMPLEMENTATION
#define STBRP_LARGE_RECTS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth noting that stb_rect_pack 1.01 would make this the default behavior (we have 1.00): https://github.com/nothings/stb/blob/af1a5bc352164740c1cc1354942b1c6b72eacb8a/stb_rect_pack.h#L44

Instead of fitting all atlas slices into a single image, which meant there
was a hard limit on the size, BakedLightmap will now save as many images
as needed to fit all the slices generated by the lightmapper.
@JFonS
Copy link
Contributor Author

JFonS commented Feb 15, 2022

@akien-mga stb_rect_pack 1.01 doesn't have the warning issue because it gets rid of the dual modes.

Since updating to 1.01 fixes both changes I had made in this PR, I just removed them and we can do the upgrade in a separate PR.

@akien-mga akien-mga merged commit 9343c66 into godotengine:3.x Feb 15, 2022
@akien-mga
Copy link
Member

Thanks!

@azur-wolve
Copy link

azur-wolve commented Apr 4, 2022

Looks awesome!
There are any advantages a part from reducing memory usage? (if understood correctly)

@Calinou
Copy link
Member

Calinou commented Apr 4, 2022

Looks awesome! There are any advantages a part from reducing memory usage? (if understood correctly)

Supporting multiple atlases makes it possible to successfully bake lightmaps in large levels without exceeding the maximum texture size. Before multiple atlases were supported, you had to disable atlases to successfully bake lightmaps in a large scene. This was less efficient to render and resulted in a lot of files being created when baking lightmaps.

@gdlq
Copy link

gdlq commented May 30, 2022

Doesn't work in 3.5 RC2.
Same error as before Failed determining lightmap size. Maximum lightmap size too small?
I tried the test scene from https://github.com/godotengine/godot/files/8064047/test_lightmap_large_3.x.zip
Added 10 additional cubes from blender to the scene and changed import settings in the Lightmap Texel Size to 0.05, I should have several exr textures but I got error Maximum lightmap size too small, if I change Lightmap Texel Size to 1.0 then it bakes all in single exr texture.

@Calinou
Copy link
Member

Calinou commented May 30, 2022

@gdlq Please open a new issue with a minimal reproduction project attached.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants