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

Reverse Z depth broke LightmapGI #90627

Closed
permelin opened this issue Apr 13, 2024 · 4 comments · Fixed by #90828
Closed

Reverse Z depth broke LightmapGI #90627

permelin opened this issue Apr 13, 2024 · 4 comments · Fixed by #90828

Comments

@permelin
Copy link
Contributor

Tested versions

Bisect shows that 69a4ff8 is the culprit

System information

Godot v4.3.dev (2886511) - Debian GNU/Linux trixie/sid trixie - X11 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 2080 (nvidia) - Intel(R) Core(TM) i7-9700K CPU @ 3.60GHz (8 Threads)

Issue description

After #88328 was merged, baking with LightmapGI outputs a tiny and completely black EXR which obviously doesn't provide any GI at all.

At first glance, changing RD::COMPARE_OP_LESS to RD::COMPARE_OP_GREATER in two places in modules/lightmapper_rd/lightmapper_rd.cpp does seem to fix it, but there is more than that going on with depth in that file and I'm not at all familiar with the code so I'll leave a PR to someone else.

Steps to reproduce

  1. Bake anything with LightmapGI.
  2. Notice the absolute lack of indirect lighting.

Minimal reproduction project (MRP)

lightmapgi-reverse-z-mrp.zip

@akien-mga
Copy link
Member

CC @godotengine/rendering

@akien-mga akien-mga added this to the 4.3 milestone Apr 13, 2024
@clayjohn
Copy link
Member

This bug is really surprising, looking through the lightmapper_rd code, it doesn't seem to interact with anything that would have been changed by the reverse z PR.

Your confirmed changes though sound exactly like the lightmapper is using reverse z somehow though, so there must be something I am missing

@JFonS
Copy link
Contributor

JFonS commented Apr 13, 2024

Had a quick look. The lightmapper uses rasterization to plot the base maps and the depth is hard-coded here (and similarly in lm_blendseams.glsl):

gl_Position = vec4((uv_interp + params.uv_offset) * 2.0 - 1.0, 0.0001, 1.0);

I guess the default clear value for depth has been flipped, so 0.0001 is discarded unless we flip the depth operator. We could also explicitly set the clear depth to 1.0 and call it a day ;)

@clayjohn
Copy link
Member

That looks like the issue, the default clear value flipped in draw_list_begin():

DrawListID draw_list_begin(RID p_framebuffer, InitialAction p_initial_color_action, FinalAction p_final_color_action, InitialAction p_initial_depth_action, FinalAction p_final_depth_action, const Vector<Color> &p_clear_color_values = Vector<Color>(), float p_clear_depth = 0.0, uint32_t p_clear_stencil = 0, const Rect2 &p_region = Rect2());

We should reverse that and then make sure the proper clear depth is used where it needs to be, or else we are breaking compat for a lot of RD consumers

@Calinou Calinou changed the title Reverse z depth broke LightmapGI Reverse Z depth broke LightmapGI Apr 18, 2024
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.

6 participants