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: Pack L1 SH coefficients for directional lightmaps #96114

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

BlueCube3310
Copy link
Contributor

@BlueCube3310 BlueCube3310 commented Aug 26, 2024

Depends on #96113

Packs the L1 coefficients of SH lightmaps into a 0.0-1.0 range, which will allow compressing directional lightmaps as BC6U. This is an approach based on https://media.contentapi.ea.com/content/dam/eacom/frostbite/files/gdc2018-precomputedgiobalilluminationinfrostbite.pdf.

The packing is currently performed as an extra step after the image has already been denoised as a way to avoid precision errors.

TODO:

  • Add a testing project,
  • Compare the visual results between unpacked and packed,
  • Add a versioning system to LightmapGIData so as to not load old lightmaps and ask the user to rebake.

Future work:

Comparison:

bake mode 4.4-dev1 PR
Static 44dev-static pr-static
Dynamic 44dev-dynamic pr-dynamic

Testing project:
sh-lightmaps.zip

@BlueCube3310 BlueCube3310 requested a review from a team as a code owner August 26, 2024 12:12
@AThousandShips AThousandShips added this to the 4.x milestone Aug 26, 2024
@BlueCube3310 BlueCube3310 force-pushed the sh-lightmap-packing branch 2 times, most recently from f056a09 to 7fc357d Compare August 30, 2024 20:15
@BlueCube3310 BlueCube3310 requested a review from a team as a code owner August 30, 2024 20:15
@BlueCube3310
Copy link
Contributor Author

I've added a basic versioning system with the use of a new internal property whose purpose is to notify the LightmapGI node if the directional lightmaps are packed or not. I'm not sure if this is a good solution though.

@@ -1352,6 +1365,7 @@ void LightmapGI::_notification(int p_what) {
switch (p_what) {
case NOTIFICATION_POST_ENTER_TREE: {
if (light_data.is_valid()) {
ERR_FAIL_COND_MSG(light_data->is_using_spherical_harmonics() && !light_data->is_using_packed_directional(), "The directional lightmap textures are stored in a format that isn't supported anymore. Please rebake to fix the issue.");
Copy link
Member

Choose a reason for hiding this comment

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

Since the packing is implemented as a separate pass, could we not just run that pass when we encounter an old lightmap and upgrade it automatically for the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about it, but older lightmaps wouldn't work the same way since they use different coefficients

Copy link
Member

@clayjohn clayjohn Sep 1, 2024

Choose a reason for hiding this comment

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

Oh right, I guess #95888 technically breaks compatibility already for existing lightmaps so this PR fixes the compatibility breakage from that.

@clayjohn
Copy link
Member

Here is an MIT licenced implementation of the Geomerics technique. Its a but more expensive to unpack, but it looks really good.

https://github.com/kayru/Probulator/blob/4a97a2b021eb2ca7ef696f4ddf36ba9a9432cbb6/Source/Probulator/SphericalHarmonics.h#L136-L151

  • Consider saving L1 as LDR images. This will increase their precision, but the size will remain the same as they will require BC7 compression due to etcpak's BC1 being too low-quality,

We can probably switch to BC1 once we merge #95915

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 (rebased on top of master b6223c0), it works as expected in all rendering methods. This allows using VRAM compressed directional lightmaps with any rendering method, whereas you'd get error spam previously (and a black screen).

Testing project: test_lightmap_directional_compress.zip

Preview

VRAM Uncompressed

VRAM Uncompressed

VRAM Compressed (this PR)

VRAM Compressed

PS: I noticed directional lightmaps generally represent light direction in a more faint manner compared to real-time lights, even when there is only a single clear light direction at a given texel, no indirect light involved and when lightmap bounces are set to 0. Is there a way we could get the directional lightmapping appearance to make normal mapped surfaces closer to how they'd look like with real-time lighting? This isn't an issue specific to this PR, but I thought it'd be worth pointing out.

@clayjohn
Copy link
Member

clayjohn commented Sep 5, 2024

PS: I noticed directional lightmaps generally represent light direction in a more faint manner compared to real-time lights, even when there is only a single clear light direction at a given texel, no indirect light involved and when lightmap bounces are set to 0. Is there a way we could get the directional lightmapping appearance to make normal mapped surfaces closer to how they'd look like with real-time lighting? This isn't an issue specific to this PR, but I thought it'd be worth pointing out.

I have been thinking about this a lot. I think it has to do with our discussion above about the coefficients. Right now I think something is off with the spherical harmonic baking process that results in the average light being roughly correct, but the directional light being too weak.

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Sep 6, 2024
@akien-mga akien-mga merged commit aa07333 into godotengine:master Sep 6, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

7 participants