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

Add a supersampling option to LightmapGI #100765

Merged

Conversation

Geometror
Copy link
Member

@Geometror Geometror commented Dec 23, 2024

Alternative to #93445 incorporating some of the feedback
Co-authored by @Calinou

Supersampling enhances lightmap quality by reducing noise, light leaking and producing smoother shadows with better small-scale detail. However, it significantly increases bake times and memory usage during lightmap generation, so supersampling is disabled by default.

image

Key differences:

  • Renamed the functionality from downsampling to supersampling
  • Configuration based on two properties:
    • supersampling (bool) to enable the feature
    • supersampling_factor (float) to tweak the size at which the lightmap is rendered before downsampling
  • Use Lanczos downsampling (I couldn't notice oversharpening or other problematic artifacts in any of my test scenes)

For some reason the issue of the original PR with shadows not getting smoother when using supersampling is gone now:
(likely something got fixed since this one in rebased on current master)

Texel scale 2, supersampling off

image

Texel scale 2, supersampling on

image

Test with the Unreal Sun Temple scene, texel scale 1, supersampling on (with factor 4)

(Minor light leaks on the ceiling due to a very low texel density of that mesh.)
image

@SpockBauru
Copy link

Made a small MRP with a quick screenshot button on the LightmapGI node, so is easy to compare ^_^

supersamplint_test.zip

Tested the windows artifact. It does indeed improve quality proportionally to the supersampling factor, take a look on the shadow at the base of the tree:

Factor 1.0 Factor 2.0
supersampling_factor_1 0 supersampling_factor_2 0
Factor 3.0 Factor 4.0
supersampling_factor_3 0 supersampling_factor_4 0

There is a bug when using the LighmapGI "Directional" property with Supersampling above 1.0, it outputs a wall of text and corrupts the lightmap:

supersampling_factor_2 0_corrupted_directional

Also, when choosing a Supersampling Factor other than the default, the reset icon does not appear:
image

@Geometror Geometror force-pushed the lightmapgi-add-downsampling branch from 6fbb50c to 35db729 Compare December 23, 2024 19:38
@Geometror
Copy link
Member Author

Thanks a lot for testing!

@SpockBauru Hmm, I'm unable to reproduce this issue on my machine. Can you share the error output? On which OS are you? Could you provide a MRP?

The default property bug has been fixed.

@SpockBauru
Copy link

Sure, my system info:
Godot v4.4.dev (a7eef3b) - Windows 11 (build 22631) - Multi-window, 1 monitor - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3080 (NVIDIA; 32.0.15.6614) - 12th Gen Intel(R) Core(TM) i7-12700KF (20 threads).

The MRP was provided on my last comment.

Here is the error:

supersampling_error.mp4

@lander-vr
Copy link
Contributor

Haha, I was just about to open a PR for this as well! :)

Wondering how necessary the bool is, since it's effectively the same as having the supersampling factor set to 1.0, and there are no other settings tied or hidden by this bool. This ultimately just results in an extra inspector property listed when it is enabled. I personally also think it could make more sense to have this setting placed underneath texel-scale. (Though the general order of the lightmapgi properties don't make much sense as they are now anyways)

There is an issue with supersampling higher than 2x:
image
Dark seams appear on the borders of lightmap islands. This is because besides the padding, the dilation also needs to be adjusted based on the supersampling factor. Currently on master the dilation is hard-coded for a 2 pixel distance.

@Geometror Geometror force-pushed the lightmapgi-add-downsampling branch from 35db729 to b22e6fd Compare December 23, 2024 20:30
@@ -39,10 +39,10 @@
If [code]true[/code], bakes lightmaps to contain directional information as spherical harmonics. This results in more realistic lighting appearance, especially with normal mapped materials and for lights that have their direct light baked ([member Light3D.light_bake_mode] set to [constant Light3D.BAKE_STATIC] and with [member Light3D.editor_only] set to [code]false[/code]). The directional information is also used to provide rough reflections for static and dynamic objects. This has a small run-time performance cost as the shader has to perform more work to interpret the direction information from the lightmap. Directional lightmaps also take longer to bake and result in larger file sizes.
[b]Note:[/b] The property's name has no relationship with [DirectionalLight3D]. [member directional] works with all light types.
</member>
<member name="environment_custom_color" type="Color" setter="set_environment_custom_color" getter="get_environment_custom_color">
<member name="environment_custom_color" type="Color" setter="set_environment_custom_color" getter="get_environment_custom_color" default="Color(1, 1, 1, 1)">
Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like they are unrelated to this PR, but these got modified since I changed all property usages to PROPERTY_USAGE_NO_EDITOR in _validate_property since PROPERTY_USAGE_NONE causes them to have no default value.

@lander-vr
Copy link
Contributor

lander-vr commented Dec 23, 2024

I think it also make sense to have the supersample_factor affect denoiser_range, just by multiplying the denoiser range by a rounded supersample factor. This keeps the relative denoiser range between a supersampled and non-supersampled bake the same.
This should keep the end result of a supersampled denoised bake more consistent to a regular denoised bake (Besides of course the qualitative improvements from supersampling).

@SpockBauru
Copy link

Updated the MRP to be more readable and focus on the issue. Just bake the lightmaps and the bug may trigger: supersampling_mrp.zip

@Geometror Geometror force-pushed the lightmapgi-add-downsampling branch from b22e6fd to e974f0c Compare December 29, 2024 20:44
@Geometror
Copy link
Member Author

The bug with directional lightmaps should now be fixed.
However, after digging deeper into the inner workings of the lightmapper, I think there is a better approach to supersampling here, which is more sophisticated, but should be cleaner and more performant. (downsampling via GPU before dilation [and maybe even before denoising], so that it doesn't need to be adjusted)

@SpockBauru
Copy link

SpockBauru commented Dec 29, 2024

Tested the Windows artifact on Forward+, Mobile and Compatibility renderers. The directional lightmaps bug is fixed 😄

I didn't find any visual issues related to this PR. (There are some error messages on Compatibility, but they are also on the Master branch so is not related to this PR)

@lander-vr
Copy link
Contributor

However, after digging deeper into the inner workings of the lightmapper, I think there is a better approach to supersampling here, which is more sophisticated, but should be cleaner and more performant. (downsampling via GPU before dilation [and maybe even before denoising], so that it doesn't need to be adjusted)

I'm curious how beneficial this would be. The cost of dilation and denoising isn't very big in the context of a full bake, so the difference in the end with a more complex implementation wouldn't be very significant. Unless I'm mistaken, the time savings here I assume would be at most a matter of a few seconds on baketimes that are in the order of (many) minutes to hours, with a much more complex implementation.

I would suggest we go for a simple implementation now, still targeting 4.4, and to explore a more sophisticated approach later if it makes sense to do. On the user side nothing should change between the implementations, so there shouldn't be any compatibility issues with that afaik.

I messaged you on Rocketchat sharing the dilation code I wrote for my branch, which you're free to use however you'd like.

@Geometror Geometror force-pushed the lightmapgi-add-downsampling branch 3 times, most recently from a270b1f to c7707d7 Compare January 4, 2025 04:00
@Geometror
Copy link
Member Author

Alright, added the dilation algorithm @lander-vr sent to me (slightly modified). Thanks! Edges no longer have dark seams.
With that, I also increased the dilation radius (4 or 4 * supersampling factor) and made denoiser_range to be scaled by the supersampling factor.

In additon, the texel size used for anti-aliasing in the lightmapping compute shader is scaled by the supersampling factor as well - now there's only a slight increase in aliasing proportional to the supersampling factor (caused by the denoiser, but only noticeable at a very high supersampling factor). Ideally, downsampled should take place before denoising/dilating - that's one downside of this approach.

@SpockBauru
Copy link

SpockBauru commented Jan 4, 2025

Tested multiple times this one, but results are results: on the tree MRP the new build (c7707d7) is worse than the old version (e974f0c) on both image clarity and baking times.

Here is a comparison between both builds for supersampling factor 1.0 (disabled), 2.0 and 4.0. Everything else is at the default settings. The image is blurrier on all supersampling factors, even disabled:

build 1.0 (disabled) 2.0 4.0
e974f0c e974f0c-supersampling_factor_1 0 e974f0c-supersampling_factor_2 0 e974f0c-supersampling_factor_4 0
c7707d7 c7707d7-supersampling_factor_1 0 c7707d7-supersampling_factor_2 0 c7707d7-supersampling_factor_4 0

Also the shadows at the base of the hill seems to be misaligned from the curvature on the new build.

As expected, baking times increased, mostly because the denoiser:

build 1.0 (disabled) 2.0 4.0
e974f0c 3.0s 7.0s 30.5s
c7707d7 3.0s 8.1s 56.5s
increase 0.0% 15.7% 85.2%

Tested without the denoiser, seems like the image degradation is happening before this step:

build 1.0 (disabled) 2.0 4.0
e974f0c e974f0c-no_denoiser-supersampling_factor_1 0 e974f0c-no_denoiser-supersampling_factor_2 0 e974f0c-no_denoiser-supersampling_factor_4 0
c7707d7 c7707d7-no_denoiser-supersampling_factor_1 0 c7707d7-no_denoiser-supersampling_factor_2 0 c7707d7-no_denoiser-supersampling_factor_4 0

@Geometror
Copy link
Member Author

Geometror commented Jan 5, 2025

@SpockBauru Thanks for testing again!
I'm guessing a lot of this comes from #100929, since the new version includes it.
The blurrier image is expected since lightmap anti-aliasing got broken when the transparency support was added, #100929 reintroduced it. The JNLM denoiser does not particularly well with fine detail and I believe the default denoiser range is also too high (will open a PR about that soon).

To be honest, I don't see any misalignment; it looks correct to me.

The denoiser range is now also multiplied by the supersampling factor, as suggested by @lander-vr - so the denoiser might also produce a tiny bit blurrier results than before. However, I think this is correct; we should adjust the default base denoiser range to mitigate that.

The increase in baking times is strange. I will investigate.

Just that you know: I edited your comment without any changes so that I can view your images in their original size again. This is a known GH bug and they might break again in a few hours :(

@lander-vr
Copy link
Contributor

lander-vr commented Jan 5, 2025

Just did some tests as well:

Supersampling 6fbb50c c7707d7
- 00:02 00:02
2x 00:08 00:09
2x w/o denoise 00:08 00:08
4x 00:32 00:54
4x w/o denoise 00:30 00:31

This time increase from the denoiser is probably caused by the multiplied range. The denoiser ranges input is limited to 20 pixels, so maybe for this initial implementation it'd be best to either just ignore it and not do the multiplication, or clamp it to 20. I don't remember if there was a technical reason for that limit, it's been quite a while since I implemented that. The increased cost does make sense though and I should've predicted that, that's my bad!

Its visual impact would be pretty small, missing the adjusted denoiser range is definitely not a dealbreaker for an initial implementation of supersampling. So the most pragmatic way forwards right now would be to just not adjust it (or clamp it to 20 as I mentioned before), or resize before the lightmap before denoising, though I'm not sure how easy that'd be.

Dark edges along lightmap UV islands are fully resolved and qualitatively I'm getting great results in my tests, no increased aliasing or any of the likes. Non integer factors are a little bit less sharp, as expected, but not very drastically so that's great.

@Geometror Geometror force-pushed the lightmapgi-add-downsampling branch from c7707d7 to 57cd53c Compare January 6, 2025 19:00
@Geometror
Copy link
Member Author

Geometror commented Jan 6, 2025

After thinking about it again I came to the conclusion that the denoiser range shouldn't depend on the lightmap's resolution (since the noise pattern doesn't scale), so removed the adjustment completely.
I also removed the dilation params uniform buffer (because we don't need it currently).

@SpockBauru
Copy link

Made a quick test on the windows artifact of 57cd53c and the performance is back to normal \o/.
There are no visual differences from c7707d7.

This provides increased lightmap quality with less noise, smoother
shadows and better small-scale shadow detail. The downside is that
this significantly increases bake times and memory usage while baking
lightmaps, so this option is disabled by default.

Co-authored-by: Hugo Locurcio <[email protected]>
Co-authored-by: landervr <[email protected]>
@Geometror Geometror force-pushed the lightmapgi-add-downsampling branch from 57cd53c to 054340b Compare January 7, 2025 18:03
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 great to me! I haven't tested locally, but I trust the testing done by others in the comments

@akien-mga akien-mga merged commit 709f2e1 into godotengine:master Jan 7, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@Calinou
Copy link
Member

Calinou commented Jan 8, 2025

However, after digging deeper into the inner workings of the lightmapper, I think there is a better approach to supersampling here, which is more sophisticated, but should be cleaner and more performant. (downsampling via GPU before dilation [and maybe even before denoising], so that it doesn't need to be adjusted)

This shader-based approach would avoid the VRAM utilization and texture size limits that "naive" downsampling can encounter, so I think we should attempt it after 4.4.

A shader-based approach still allows for high-quality downsampling algorithms. Area downsampling is a popular technique for downsampling from arbitrary supersampling factors, as it has great quality.1

Footnotes

  1. We don't have area downsampling in Image for the CPU-based resize(), but it could technically be added.

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.

6 participants