Skip to content

Conversation

@BlueCube3310
Copy link
Contributor

@BlueCube3310 BlueCube3310 commented Dec 24, 2024

Depends on #100640
Inspired by https://media.contentapi.ea.com/content/dam/eacom/frostbite/files/gdc2018-precomputedgiobalilluminationinfrostbite.pdf

Changes the way directional lightmap textures are stored: Previously they were part of the HDR lightmap texture array in spite of only storing LDR data. Now, they are saved as a separate .png file and compressed to DXT1/ETC2 RGB on import.

This approach reduces the memory footprint by half (when compressed as DXT1/ETC2 RGB). I've tested this with several scenes (many with a solid albedo, to see how the quality will be affected) and the low-quality versions don't look much worse than high-quality (BC7/ASTC 4x4) ones (even with etcpak).

#108405 will allow scaling them independently from regular lightmaps, making it possible to bake lower-resolution directional maps and save even more video memory.

TODO:

@solitaryurt
Copy link

solitaryurt commented Jan 20, 2025

After baking the lightmaps with this branch, am I supposed to see a separate .png for the directional data? I am only seeing the .exr file right now.

@BlueCube3310
Copy link
Contributor Author

After baking the lightmaps with this branch, am I supposed to see a separate .png for the directional data? I am only seeing the .exr file right now.

Yes, the directional lightmap has a "_directional.png" suffix

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, it works as expected in all rendering methods (both baking and rendering). Visuals are identical to before, and code looks good to me.

Testing project: test_lightmap_directional_ldr.zip

Note that right now, you need to bake lightmaps again for them to render correctly. This shouldn't be required anymore once a conversion tool is implemented.

This provides a significant size reduction for lightmap data that is committed to version control:

  • Before: 93.4 MB EXR
  • After: 23.5 MB EXR + 16.6 MB PNG = 40.1 MB (2.33x smaller)

Note that like shadowmask images, we could benefit from saving to a lossless WebP instead of PNG to reduce file sizes of what you commit to version control: #101881
However, this may be challenging to do unless directional data is stored at a halved texel density, since it will frequently go over WebP's 16383×16383 limit. For example, in the MRP, the directional data texture is 8192×24576.

Bake times are very similar before and after this PR (note that bake times include the time taken to reimport textures with VRAM compression).

Also, this paves the way for making #50574 usable with directional lightmaps, since directional data is no longer part of the main lightmap image.

PS: With this PR, we now have two lightmap textures that can go unused depending on which properties you end up disabling. For example, if you bake with shadowmasks and directional enabled, then disable both and bake again, both the shadowmask and directional textures will go unused but remain within the project files. We should look into printing a warning after baking when unused textures have been detected based on naming patterns. This would complement the existing Project > Tools > Orphan Resource Explorer dialog.

@passivestar
Copy link
Contributor

looks like it doesn't compile on master anymore

servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp:3334:6: error: use of undeclared identifier 'u'
 3334 |                                         u.append_id(rd_texture);

@BlueCube3310
Copy link
Contributor Author

looks like it doesn't compile on master anymore

servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp:3334:6: error: use of undeclared identifier 'u'
 3334 |                                         u.append_id(rd_texture);

Rebased, it should now compile. If possible, the lightmapper will now try to save the textures as .webp

@passivestar
Copy link
Contributor

passivestar commented Jun 5, 2025

Tested,

Master: 24.69 MiB exr
PR: 10.98 MiB (7.08 MiB exr + 3.90 MiB webp)

Visually seems to be identical in my test. An option to bake directional data at lower resolution would be incredibly useful, though this PR already puts directional lightmaps in a shippable range

We should look into printing a warning after baking when unused textures have been detected based on naming patterns

Since the paths to the previous images are already stored in lightmap's LightmapGIData wouldn't it make sense to just delete those on rebake? Doesn't seem like checking for naming patterns will be necessary in this case

@Rudolph-B
Copy link
Contributor

While I was developing #107168 I tested on top of this PR quite extensively. Its visual fidelity always matched that of the hdr textures.

@BlueCube3310
Copy link
Contributor Author

Rebased on top of #107168, it's now ready for wider testing

directional

@michaelharmonart
Copy link

marked as draft since it's lacking conversion code for old lightmaps or simply because more testing is needed?

@BlueCube3310
Copy link
Contributor Author

This PR is now ready for initial feedback, though it's not fully complete as backwards compatibility needs to be added.

@BlueCube3310
Copy link
Contributor Author

I've added a way to upgrade older lightmaps - a new button will appear next to the "Bake" one for incompatible lightmaps, and pressing it will automatically convert the map to the new format

@michaelharmonart
Copy link

michaelharmonart commented Jun 17, 2025

Tested with some distant buildings from one of my XR projects, and seems there's an issue with the compatibility renderer currently:
Lightmap baked in 4.5 Dev5 (as viewed in Compatibility Renderer)

When opening in a build from this PR, the lightmap shows a warning asking to upgrade format, and when doing so the resulting scene looks like this (as viewed in Compatibility Renderer):

Switching to Forward+ it all looks good:

Size savings are quite good. With the auto conversion from the old format, in this case the lightmaps go from 12.0MB to 7.5MB. I can't discern a difference in quality

@BlueCube3310
Copy link
Contributor Author

BlueCube3310 commented Jun 17, 2025

I'm unable to reproduce the issue, on my end it converts fine.
Does reloading the editor resolve this?

@michaelharmonart
Copy link

michaelharmonart commented Jun 17, 2025

The conversion of the map is the same whether in compatibility or forward+ (works as expected), but rendering of the directional lightmaps is broken in compatibility. The weird blueish values only appear when using fog (depth fog) probably due to the negative values.

will try and make an MRP
edit: I'm currently unable to make an MRP as just adding a lightmap GI to a fresh 3d scene in a new godot project crashes the editor with my build from this PR.....

@jcostello
Copy link
Contributor

Works as expected in my Sponza test scene

@michaelharmonart
Copy link

set up the following MRP with a build from the master branch. When opened with a build from this PR, and after converting lightmaps, the rendering error I showed earlier is present in compatibility mode

System Info:
Godot v4.5.beta (f5fc2e0) - Manjaro Linux #1 SMP PREEMPT_DYNAMIC Wed, 11 Jun 2025 04:28:00 +0000 on Wayland - X11 display driver, Multi-window, 1 monitor - OpenGL 3 (Compatibility) - NVIDIA GeForce RTX 3060 (nvidia; 575.57.08) - AMD Ryzen 9 9900X 12-Core Processor (24 threads) - 30.44 GiB memory

MRP:
lightmaps.zip

@BlueCube3310
Copy link
Contributor Author

BlueCube3310 commented Jun 17, 2025

Does the issue go away when you add the directional light into the scene?
Toggling the sunlight on and off seems to fix it

Edit: This is fixed now

@michaelharmonart
Copy link

Tested with your updates and directional lightmaps are now rendered properly for compatibility in the test for me as well!

For some reason adding a lightmapGI node with this branch still crashes the editor for me though

@BlueCube3310
Copy link
Contributor Author

For some reason adding a lightmapGI node with this branch still crashes the editor for me though

The crashing should be fixed now

@BlueCube3310 BlueCube3310 force-pushed the dir-lightmap-ldr branch 2 times, most recently from 4ef4368 to 4988b9b Compare September 26, 2025 09:25
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.

8 participants