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

Lightmap artifacts around geometry edges. #82526

Closed
atirut-w opened this issue Sep 29, 2023 · 10 comments · Fixed by #82533
Closed

Lightmap artifacts around geometry edges. #82526

atirut-w opened this issue Sep 29, 2023 · 10 comments · Fixed by #82533

Comments

@atirut-w
Copy link
Contributor

Godot version

v4.2.dev.custom_build [1989061]

System information

Godot v4.2.dev (1989061) - Windows 10.0.22621 - Vulkan (Forward+) - dedicated AMD Radeon RX 6700 XT (Advanced Micro Devices, Inc.; 27.20.21034.37) - AMD Ryzen 7 5700X 8-Core Processor (16 Threads)

Issue description

I've been poking at @DarioSamo's JNLM denoiser for the past few days and I've been noticing these strange artifacts around the edge of lightmaps not present pre-JNLM:

image
image

I also may have identified a fix: accounting for the half-pixel offset added in 2c125bf in the JNLM denoiser. I took a look at #81872 recently and tried reverting it on a guess, and the artifacts disappears completely (see below). Of course, it also unfixes the issue it solved, so reverting it isn't the right fix.

Lightmap with 2c125bf reverted, for comparison:
image
image

Steps to reproduce

I have been unable to accurately reproduce the problem, but I've identified a (freely available) model that can trigger the issue (MRP).

  1. Open MRP
  2. Bake lightmap
  3. Artifacts can be seen throughout the scene.

Minimal reproduction project

lightmap_seams.zip

@DarioSamo
Copy link
Contributor

I think I noticed this and have a couple of theories as to the probable causes, but I'm surprised the half pixel offset has such an effect. Let's experiment for a bit as this should be easily traceable.

@DarioSamo
Copy link
Contributor

I can confirm the seam appears and is gone without the half pixel offset. I guess we didn't notice it in isolation in the other branch because the half pixel offset wasn't merged at the time of testing.

I tested taking out both albedo and normals from the formula and it didn't affect it, so I'm not entirely sure why the artifact occurs yet. More than a seam as far as I can tell it looks more like the result not being denoised as strongly to me.

I'll investigate it further but I don't think this would warrant reverting the denoiser as it's likely fixable.

@atirut-w
Copy link
Contributor Author

reverting the denoiser

If push comes to shove, we can always drop an if into the half-pixel offset parts. Not that it's the best fix, though.

@DarioSamo
Copy link
Contributor

Seems like it has to do with the fact the noisy pixels don't have a valid normal or position whatsoever, so they don't get denoised. I'm confused as to how exactly the pixel is getting lighting in the first place under those conditions but that explains it.

@DarioSamo
Copy link
Contributor

DarioSamo commented Sep 29, 2023

It's not related to the denoiser. I turned it off and forced it to set all invalid pixels to this green color. I think the noise you're seeing is from previous bakes (will need to check if the texture is getting cleared as it should).

So if anything we should investigate the other reason behind this. I just think OIDN was obscuring it but the pixels are not valid at all. Seems like there's something we've yet to settle when it comes to the half pixel offset and the UV2 generation as evident by the primitive meshes issue.

image

@atirut-w
Copy link
Contributor Author

atirut-w commented Sep 29, 2023

I think the noise you're seeing is from previous bakes (will need to check if the texture is getting cleared as it should).

I tried nuking the light data (*.lmbake and *.exr) before baking. No joy.

@DarioSamo
Copy link
Contributor

DarioSamo commented Sep 29, 2023

I think the noise you're seeing is from previous bakes (will need to check if the texture is getting cleared as it should).

I tried nuking the light data (*.lmbake and *.exr) before baking. No joy.

I don't think that's quite it either then, but it's definitely grabbing some invalid data from somewhere.

I can at least confirm the denoiser is not at fault, so we can ignore that for now. It just automatically fixed it with OIDN because it happened to put invalid pixels next to it and denoise them, but they weren't valid pixels to begin with as they're not in the position and normal textures at all.

@DarioSamo
Copy link
Contributor

DarioSamo commented Sep 29, 2023

Okay I think I figured it out.

The invalid data came from the dilate step. I'm not sure how it landed there, but there's a dilate step before the denoising, which means it just expands a bunch of noise.

The dilate is required for the seams at the edges of the UV2. I just moved it afterwards and removed the dilate before the denoising.

Can you test the commit in this branch and see if it fixes the problem? It did at least for me.

https://github.com/DarioSamo/godot/tree/no-dilate-before-denoise

@atirut-w
Copy link
Contributor Author

It seem to have fixed the issue for me, too

@DarioSamo
Copy link
Contributor

Cool, the logic makes perfect sense to me, so I'll make it a PR then.

mandryskowski pushed a commit to mandryskowski/godot that referenced this issue Oct 11, 2023
Dilating noisy data caused issues for the denoiser. Fixes godotengine#82526.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants