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

LightmapGI: Clean up and improve lightmap atlas storage #98289

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

BlueCube3310
Copy link
Contributor

@BlueCube3310 BlueCube3310 commented Oct 18, 2024

Excerpt from #85653

Godot's lightmap storage has gone through several iterations, with the current one working like this:
Storage-wise, the lightmaps are an array of layered textures. This is due to how layered textures are imported into the engine (A single image is loaded, then split into several smaller ones based on the defined layer count. Since the size limit of an image is 16K, large lightmaps may exceed that limit and the image will fail to load. As a solution, the lightmap textures are split into several layered textures at the end of the baking process).
When loaded, the engine recombines them into a single layered texture, which is then used for rendering.

The older methods are still kept for compatibility, though the 'old-style' texture references are still saved, which can result in the same image being loaded multiple times.

Additionally, the lightmap exposed in the editor inspector is still the old one, which only allowed a single layered texture.

This PR cleans up some parts of this process, namely by renaming the internal class members to be less confusing, adding comments, as well as ensuring the 'old-style' lightmaps are only loaded, but not saved.
Additionally, a new function was added for the splitting process described above. This will make it easier to add features like shadowmasks or LDR-packed directional data in the future.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally on Truck Town (with lightmap UV2 enabled and LightmapGI Texel Scale set to 10), it works as expected.

@clayjohn
Copy link
Member

Should we merge this before #85653? For context, I'd really like to get #85653 merged in the 4.4 cycle

@BlueCube3310
Copy link
Contributor Author

Should we merge this before #85653? For context, I'd really like to get #85653 merged in the 4.4 cycle

I think so, this is a part of that PR which I've split to reduce its complexity.

@clayjohn clayjohn modified the milestones: 4.x, 4.4 Oct 18, 2024
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Overall things look great, I just have one question about the bindings stuff

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks fine to me!

@Repiteo Repiteo merged commit b748c91 into godotengine:master Oct 30, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Oct 30, 2024

Thanks!

@BlueCube3310 BlueCube3310 deleted the lightmap-clenaup branch November 9, 2024 11:34
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.

4 participants