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: Fix lightleaks caused by insufficient padding and add denoiser range property for LightmapGI #91601

Merged
merged 1 commit into from
May 15, 2024

Conversation

lander-vr
Copy link
Contributor

@lander-vr lander-vr commented May 5, 2024

Partially resolves- #82607, but I think there is a little bit more work to do besides this PR before we can close that issue.

This PR changes the hardcoded padding from 2 pixels, to a padding that adjust automatically alongside a newly exposed "denoiser range" property. It resolves light leakage issues that were caused by the denoiser, and gives users vastly improved control over the results of their lightmap bakes.

Improved seams between modular assets:

Master This PR
image firefox_DPqTccQJx8

The half_search_window (the range of pixels the denoiser samples from) was larger than the padding distance, causing it to sample pixels from irrelevant islands.
Leakage example with current padding (2px) and sufficient padding:
image

I exposed the half_search_window property to be adjustable by the user (as denoiser_range), because this gives a heap more control over the results without adding any cost.
gif

The padding now gets adjusted automatically based on which denoiser range is set: Increased range requires increased padding, a lower range allows for lower padding and more optimal lightmap usage.
Larger ranges will smooth the baked result more, so are more appropriate for highly noisy bakes, while lower ranges preserve more details.
I tried to avoid exposing this property, and have it be tied to denoiser strength, but found that different ratios between strenght and range work well for different noise and resolution combinations. It's very dependent on the situation, so losing that individual control meant I wasn't able to achieve clean results like is possible with it being separately exposed.

And finally a seamless bake comparison between master and PR:

Master:

  • Texel scale of 4x at 64x64 pixels per surface = result at 256x256
  • HALF_SEARCH_WINDOW = 10px (master)
  • Denoiser strength = 0.1
  • Render time = 00:05:06
    image

Custom:

  • Texel scale of 2x at 32x32 pixels per surface = result at 64x64
  • HALF_SEARCH_WINDOW = 3px
  • Denoiser strength = 0.025
  • Render time = 00:00:38
    image

Some more information is available here: #82607 (comment)

One issue still with this PR is that I'm unsure how to hide the property when "use denoiser" is disabled, so it follows the behavior of the denoiser strength property.

@lander-vr lander-vr requested review from a team as code owners May 5, 2024 19:49
@lander-vr lander-vr force-pushed the lightmap_seams_fix branch from 8908e1a to d214104 Compare May 5, 2024 20:40
@lawnjelly
Copy link
Member

This looks like a nice solution, I guess the only question is whether this is good for tying into a solution in the future in terms of backward compatibility.

For the original 2 pixels of padding, I'm not sure whether this is 2 each side (4 total) or 2 total:

  • It might need 4 total to prevent artifacts from texture compression which often uses 4x4 blocks
  • How well does it work with mipmaps? With each mipmap the distance sampled by filtering doubles (the same can occur with island padding).

Providing there are no problems with filtering / mipmaps / compression then determining by denoiser alone could be fine.

Alternative would be to denoise each block in the atlas separately (e.g. put the block into an image, add some border, dilate then denoise, then copy to the atlas).

@lander-vr
Copy link
Contributor Author

For the original 2 pixels of padding, I'm not sure whether this is 2 each side (4 total) or 2 total

I'm still not 100% positive on this, but I tried doing the padding based on denoiser_range/2, which re-introduced leaks, so I'm fairly certain it's 2 pixels total.

How well does it work with mipmaps? With each mipmap the distance sampled by filtering doubles (the same can occur with island padding).

I guess the only way to completely ensure no bleeding with mipmaps is to account for enough padding for the smallest mipmap, which would likely be a large waste of pixels and probably unnecessary. It may be best to settle for a middle ground. This article was something that was referenced on AAA projects in my experience, but it has a focus on regular assets so I'm not sure how relevant this would be for lightmapper mipmaps.

I guess this could be tied to an advanced setting exposed in the lightmappers project settings though? Giving the user opportunity to decide where they want to make the tradeoff between mipmap leakage and memory usage (i.e. amount of padding) makes sense to me since priorities regarding memory usage and quality are very project and user dependent.

dilate then denoise

This is something you probably want to avoid doing because it results in noise persisting a lot of times after running the denoiser. Dilating the noisy result sort of "adds weight" to noisy pixels at the edge of an island. #82533

@lawnjelly
Copy link
Member

lawnjelly commented May 6, 2024

This is something you probably want to avoid doing because it results in noise persisting a lot of times after running the denoiser. Dilating the noisy result sort of "adds weight" to noisy pixels at the edge of an island. #82533

A little off topic, but this may depend on the denoiser. If the area outside the islands is e.g. black, or white, then the denoiser can potentially introduce a color shift into the border pixels. But the trade off as you say is noise at the boundaries can become more prominent if you dilate first.

I dilate first currently in my lightmapper, but was planning to fade out denoised versions at borders, because denoising will probably introduce irregularities at seams, unless you do seam stitching after denoise.

(On a quick look, OIDN may be pretty good at not introducing color bleed from borders based on the images in #82533, whereas mine offers both OIDN and a simpler denoiser that is likely susceptible to color bleed 😁 so have to be a little careful of this problem.)

scene/3d/lightmap_gi.h Outdated Show resolved Hide resolved
@DarioSamo
Copy link
Contributor

Making this configurable sounds fine to me. I was a bit hesitant to expose it as they're not the friendliest parameters to play with, but as long as the defaults are maintained it should be alright.

One issue still with this PR is that I'm unsure how to hide the property when "use denoiser" is disabled, so it follows the behavior of the denoiser strength property.

Just take a look at how LightmapGI::_validate_property works and it should be pretty straightforward.

@lander-vr lander-vr force-pushed the lightmap_seams_fix branch from d214104 to 286a4fd Compare May 6, 2024 14:31
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 55b8724), it works as expected.

Testing project: test_lightmap_padding.zip

Before After (Denoiser Range = 2)
Screenshot_20240507_204621 Screenshot_20240507_204447

@Calinou
Copy link
Member

Calinou commented May 7, 2024

  • How well does it work with mipmaps? With each mipmap the distance sampled by filtering doubles (the same can occur with island padding).

Lightmaps are imported with mipmaps disabled, as their low texel density compared to world textures generally makes mipmaps unnecessary. This also reduces memory usage by 25%.

Most other engines tend not to use mipmaps for lightmaps as well, although they might provide an option for it (with a risk of possible lightmap bleeding as you point out).

@akien-mga akien-mga requested a review from DarioSamo May 10, 2024 07:54
Copy link
Contributor

@DarioSamo DarioSamo left a comment

Choose a reason for hiding this comment

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

LGTM, just need to address a few minor issues.

@lander-vr lander-vr force-pushed the lightmap_seams_fix branch from 286a4fd to e7bd1b0 Compare May 15, 2024 06:26
@lander-vr
Copy link
Contributor Author

Implemented the requested changes and rebased.

@akien-mga akien-mga requested a review from DarioSamo May 15, 2024 06:41
Copy link
Contributor

@DarioSamo DarioSamo left a comment

Choose a reason for hiding this comment

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

Changes look good!

@akien-mga akien-mga merged commit 693a13a into godotengine:master May 15, 2024
16 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.

6 participants