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

Vulkan: LightmapGI with Environment Mode set to a value other than Disabled exhibits light leaking through one-sided surfaces #69126

Closed
Tracked by #56033
Calinou opened this issue Nov 24, 2022 · 17 comments · Fixed by #81872

Comments

@Calinou
Copy link
Member

Calinou commented Nov 24, 2022

Related to #63437 (but not the same, as that issue is about general light leaking).

Godot version

4.0.beta6

System information

Fedora 36, Vulkan Forward Plus, AMD Radeon RX 6900 XT

Issue description

LightmapGI with Environment Mode set to a value other than Disabled exhibits light leaking, especially through one-sided surfaces. This light leaking is caused by environment lighting, which spreads incorrectly through corners.

While this can be worked around to an extent by ensuring all surfaces are two-sided (manifold), one-sided surfaces are fairly common in level design for optimization reasons.

Other things I've tried to no avail:

  • Increasing/decreasing Bias on LightmapGI
  • Disabling Use Denoiser
  • Enabling Interior
  • Increasing bake quality to Ultra
Environment Mode: Disabled Environment Mode: Scene
2022-11-24_18 13 37 2022-11-24_17 58 22
2022-11-24_18 13 44 2022-11-24_17 58 41
2022-11-24_18 13 50 2022-11-24_17 58 57
2022-11-24_18 13 56 2022-11-24_18 06 15

(The little bit of light leaking that remains on the 3rd screenshot is from the shadow map, not the lightmap.)

Steps to reproduce

  • Import a 3D scene with its Light Baking property set to Static Lightmaps.
  • Add a DirectionalLight3D node and WorldEnvironment.
  • Add a LightmapGI node.
  • Bake lightmaps.

Minimal reproduction project

test_lightmapgi_leak.zip

@Calinou Calinou added this to the 4.0 milestone Nov 24, 2022
@Calinou Calinou moved this to To Assess in 4.x Priority Issues Nov 24, 2022
@Calinou Calinou changed the title Vulkan: LightmapGI with Environment Mode set to a value other than Disabled exhibits light leaking Vulkan: LightmapGI with Environment Mode set to a value other than Disabled exhibits light leaking through one-sided surfaces Nov 24, 2022
@clayjohn clayjohn modified the milestones: 4.0, 4.x Jan 12, 2023
@WickedInsignia
Copy link

This seems to be happening with direct light GI too. Here's a manifold structure with thick walls and environment set to "disabled" in the bake. The directional light is set to dynamic, so this is the GI leaking.
LightmapLightLeak_Godot4

LightmapLeaking.zip

@DarioSamo
Copy link
Contributor

DarioSamo commented Sep 18, 2023

My working theory is this is likely related to the origin of the ray not being computed from the right spot of the pixel. I'll research this.

@DarioSamo
Copy link
Contributor

DarioSamo commented Sep 18, 2023

My theory was right, this is a fairly simple fix.

A half pixel offset needs to be added to gl_Position in the rasterization step.

vec2 half_pixel_offset = vec2(-1.0f, -1.0f) / vec2(params.atlas_size.xy);

Before
image

After
image

After of the scene above.
image

There's some leaking on the last one but to be honest I don't trust the denoiser (OIDN) is doing a good job there, as it exhibits no leaking when the denoiser is off (which wasn't the case before). Testing with my other denoiser branch might be a good way to check that.

image

No denoiser and quality pushed to ultra.
image

I'll open a PR with this change.

DarioSamo added a commit to DarioSamo/godot that referenced this issue Sep 18, 2023
Add half-pixel offset to lightmapper to fix issues where the ray would be generated from the wrong spot corresponding to the pixel and causing light leaks. Fixes Issue godotengine#69126.
@akien-mga akien-mga modified the milestones: 4.x, 4.2 Sep 20, 2023
YuriSizov pushed a commit to YuriSizov/godot that referenced this issue Sep 21, 2023
Add half-pixel offset to lightmapper to fix issues where the ray would be generated from the wrong spot corresponding to the pixel and causing light leaks. Fixes Issue godotengine#69126.

(cherry picked from commit ddc3126)
@jeffhube
Copy link

I am seeing significantly more leaking with this change. Should I open a new issue? Example project is attached.

I have also confirmed this behavior when cherry-picking this change onto 4.2-dev5.

image
image

LightmapTest.zip

@Calinou
Copy link
Member Author

Calinou commented Sep 22, 2023

LightmapTest.zip

The scene in the MRP doesn't load. (Don't remove the .lmbake and .exr files without clearing the LightmapGIData from the LightmapGI node first.)

Cannot open file 'res://main.lmbake'.
  Failed loading resource: res://main.lmbake. Make sure resources have been imported by opening the project in the editor at least once.
  ./scene/resources/resource_format_text.cpp:284 - res://main.tscn:57 - Parse Error: 
  Failed loading resource: res://main.tscn. Make sure resources have been imported by opening the project in the editor at least once.
  ./editor/editor_data.cpp:626 - Index p_idx = 1 is out of bounds (edited_scene.size() = 1).

I've been able to open main.tscn by removing all references to LightmapGIData. Fixed MRP with lightmaps baked using 4.2.dev 7831eed: LightmapTest_1.zip

I can confirm the issue reported, but I wonder how much it can be seen in real world scenarios. A inside-out cube may be a worst-case scenario for this.

Bake quality or bounce count doesn't make a difference on the light leaking (unless bounce count is 0).

4.1.1 4.2.dev 7831eed
image image

I suggest opening a new issue to track this.

@DarioSamo
Copy link
Contributor

DarioSamo commented Sep 23, 2023

You sure it's not just because this is built out of several planes with slight protrusions towards the corners that can get light? Transforming these planes around aren't exactly guaranteed to end in the same spot vs a mesh, and ray bias is a factor as well that can cause rays to be shot outside.

image

The texel resolution is also so low to the point it makes this even worse. If you increase the lightmap resolution you can see the issue pretty clearly fix itself.

image

We should test with a proper inside out cube IMO.

That said the half-pixel offset should be pushing this towards inside the mesh and never outside of it. I guess it's a reason to suspect that even at low resolution it should've still traced from the middle of the pixel... Probably worth keeping in mind but it seems like a bit of a corner case.

@WickedInsignia
Copy link

You'll want to be careful that you have enough texel density in these tests that the leak isn't occurring due to overlap in the lightmap image itself. That won't happen with a huge amount of padding between UV islands and a high texel density.
Intersecting planes pose an issue because if the edge of the planes are catching any light from outside, the low resolution of the lightmap will cause the light to bleed to the inside a lot more than it actually should physically.

This test would be better done with a cube with inverted normals, or a room with one wall removed and some thickness on the remaining walls.

@DarioSamo
Copy link
Contributor

DarioSamo commented Sep 23, 2023

I'm definitely a bit puzzled as to how it's even generating positions at the very edge of the planes at least at such low resolution, the half pixel offset is exactly meant to prevent that (and in fact it does on one side of the plane). I get the feeling there's some other mechanism at play on this scene.

At least all of these planes are just using a pretty plain independent UV2. I'll take a look at how the position texture is being generated next week to understand better why the position coordinate is ending outside of where it should be.

@jeffhube
Copy link

Here is a version with an inverted cube. LightmapTestCube.zip I'm getting pretty similar results.

4.2-dev5 / before change / 0.2 texel size 4.1.2-rc1 / after change / 0.2 texel size
image image
4.2-dev5 / before change / 0.05 texel size 4.1.2-rc1 / after change / 0.05 texel size
image image

Increasing the texel density does help, though even with an increased texel density there is less leaking before this change.

I thought it was interesting how when you look at the lightmap textures for 4.2-dev5 (included in each screenshot above), it is only the top left edges that leak. The same is true for the plane version as well, where every face is on its own slice. On 4.1.2-rc1 with this change, there is uneven leaking on all 4 edges. Each face of the cube (or each plane) has two adjacent edges that leak worse than the other two.

@DarioSamo
Copy link
Contributor

Thanks for testing with the cube, that makes it easier to rule out the other possibilities.

I thought it was interesting how when you look at the lightmap textures for 4.2-dev5 (included in each screenshot above), it is only the top left edges that leak. The same is true for the plane version as well, where every face is on its own slice. On 4.1.2-rc1 with this change, there is uneven leaking on all 4 edges. Each face of the cube (or each plane) has two adjacent edges that leak worse than the other two.

Yes, this is why I'm inclined to believe this is not exactly a regression but uncovering some other bug where the other edge can somehow retrieve the position of the edge of the polygon, especially because one of the original test scenarios was very similar to this. In theory with the half-pixel offset, the rasterizer should never be writing coordinates that perfectly align like that when it's used... but something seems to be failing here.

I'll take a look at it on Monday by debugging the output position texture in the rasterization step.

@DarioSamo
Copy link
Contributor

DarioSamo commented Sep 25, 2023

I've been giving this a look and I'm unable to determine so far what's the reason behind the error. The half pixel offset is clearly doing the behavior I want it to do in proper meshes, but this mesh in particular seems to have something in its generation of the UV2 that makes the rasterization never generate the coordinate from the center as it should. Whenever I look at the position texture that it outputs, the values are pretty much at the very edge of the cube.

The size in the atlas itself is very weird and uneven. I noticed this was generated via ArrayMesh. Can we reproduce this behavior with something like a regular box instead (say from an obj import)? I'm just not sure where the difference lies yet that causes this behavior compared to regular scenarios.

@jeffhube
Copy link

Ah, it seems that an obj import works fine. LightmapTestObj.zip

4.2-dev5 / before change / 0.2 texel size 4.1.2-rc1 / after change / 0.2 texel size
image image

Before I was using PlaneMesh and BoxMesh, so maybe there is something funny going on there with the uv2 generation.

@Calinou
Copy link
Member Author

Calinou commented Sep 25, 2023

cc @BastiaanOlij, as he implemented UV2 generation in primitive meshes.

The default UV2 padding in those is set to 2 pixels, but maybe it's too high/low? You can configure it in the PrimitiveMesh resource inspector.

@DarioSamo
Copy link
Contributor

Ah, it seems that an obj import works fine. LightmapTestObj.zip
4.2-dev5 / before change / 0.2 texel size 4.1.2-rc1 / after change / 0.2 texel size
image image

Before I was using PlaneMesh and BoxMesh, so maybe there is something funny going on there with the uv2 generation.

Very interesting indeed. Sounds like this might warrant a close inspection of what UV2 values the primitive mesh ended up with.

@jeffhube
Copy link

I took a quick look at what is being passed to LightmapperRD::add_mesh() when lightmapping an obj plane and when lightmapping a PlaneMesh. The lightmapper generated size 16x16 slices for each plane. In summary, there's a half pixel offset difference between the two (not caused by this change).

For an obj plane, the image size was 14x14 and the uvs go from [0.035714, 0.035714] to 0.964286, 0.964286.
image

For a PlaneMesh, the image size was 12x12 and the uvs go from [0, 0] to [0.833333, 0.833333]. The UV2 Padding was 2 so that explains the unused pixels on the bottom and right.
image

@DarioSamo
Copy link
Contributor

@jeffhube Thanks for the investigation.

I think what you've shown so far might explain why I've seen primitive meshes sometimes linearly interpolate the light-map on one edge of a shape and wrap around to the other side when rendering.

We might need more eyes from this on from more experienced parties as now I'm not entirely sure if it's the model importer the one that is wrong or the primitive generation depending on what else the engine expects. However, I'm inclined to say the OBJ is correct, as it matches the behavior of what I've seen happen in other engines and the half pixel offset is required when dealing with linear interpolation.

@BastiaanOlij
Copy link
Contributor

It's been a long time since I last looked into this but the primitive meshes take a fairly straight forward approach. It calculates the size of the image required based on the dimensions of the primitive shape, taking the texelsize into account. Then adds the required margins to this.

The margins are indeed added just to the right and bottom edge, so that bottom image for the plane is indeed correct.

For a box it will lay out a grid of 2 x 3 faces adding the margins again at the right and bottom edges to space them out.

You can see the UV unwrap with the mesh instance selected, pressing the Mesh button and then selecting View UV2:

image

We really should add a grid view to this :)

If we want a border on the left and top, we could change the logic fairly easily by just adding a single texel to all UV coords.

mandryskowski pushed a commit to mandryskowski/godot that referenced this issue Oct 11, 2023
Add half-pixel offset to lightmapper to fix issues where the ray would be generated from the wrong spot corresponding to the pixel and causing light leaks. Fixes Issue godotengine#69126.
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.

7 participants