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 Mobile backend: Environment lighting and reflections are twice as dark as they should be #55878

Closed
Tracked by #55871
Calinou opened this issue Dec 12, 2021 · 5 comments · Fixed by #76692
Closed
Tracked by #55871

Comments

@Calinou
Copy link
Member

Calinou commented Dec 12, 2021

Godot version

4.0.dev (2a9dd65)

System information

Fedora 34, GeForce GTX 1080 (NVIDIA 470.74)

Issue description

When using the Vulkan Mobile backend, environment lighting and reflections appear twice as dark as they should be.

Vulkan Clustered

2021-12-12_22 04 28

Vulkan Mobile

2021-12-12_22 03 44

Vulkan Clustered with Ambient Light Sky Contribution set to 0.5

This makes environment lighting consistent with the mobile backend, but this doesn't adjust reflections. Since Sky Contribution can't be set above 1.0 and Energy does not affect sky contributed lighting, it's not possible to brighten ambient light on the Vulkan Mobile backend to bring it back to the level of Vulkan Clustered.

2021-12-12_22 04 37

Steps to reproduce

  • Add 3 MeshInstance3D nodes to the scene: 2 with the fallback material, and 1 with a fully metallic and non-rough material (to see environment reflections).
  • Add a DirectionalLight3D node and a WorldEnvironment node.
  • Switch to Vulkan Mobile backend in the project settings (search for back end).
    • Note: The MRP linked below already uses the Vulkan Mobile backend by default.
  • Notice how environment lighting is twice as dark (but not the DirectionalLight3D itself).
  • Switch back to Vulkan Clustered backend and restart the editor. Decrease Ambient Light > Sky Contribution in the WorldEnvironment to 0.5. Notice how lighting now looks very similar to the mobile backend's.

Minimal reproduction project

test_vulkan_clustered_vs_mobile_environment_lighting.zip

@Calinou
Copy link
Member Author

Calinou commented Apr 10, 2022

I've tried to fix this by using the luminance multiplier to multiply specular lighting. However, the appearance still isn't completely right:

diff --git a/l.diff b/l.diff
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/servers/rendering/renderer_rd/shaders/scene_forward_mobile.glsl b/servers/rendering/renderer_rd/shaders/scene_forward_mobile.glsl
index 6911cab27b..1ace099ae4 100644
--- a/servers/rendering/renderer_rd/shaders/scene_forward_mobile.glsl
+++ b/servers/rendering/renderer_rd/shaders/scene_forward_mobile.glsl
@@ -899,6 +899,7 @@ void main() {
 #endif //USE_RADIANCE_CUBEMAP_ARRAY
 		specular_light *= horizon * horizon;
 		specular_light *= scene_data.ambient_light_color_energy.a;
+		specular_light *= sc_luminance_multiplier;
 	}
 
 #if defined(CUSTOM_RADIANCE_USED)
@@ -919,6 +920,7 @@ void main() {
 #endif //USE_RADIANCE_CUBEMAP_ARRAY
 
 			ambient_light = mix(ambient_light, cubemap_ambient * scene_data.ambient_light_color_energy.a, scene_data.ambient_color_sky_mix);
+			//ambient_light *= sc_luminance_multiplier;
 		}
 	}
 #endif // !USE_LIGHTMAP

Doubling ambient lighting makes things too bright however. (sc_luminance_multiplier is 2.0.)

Default mobile backend

2022-04-11_00 23 28

Specular lighting doubled

2022-04-11_00 25 18

Ambient and specular lighting doubled

2022-04-11_00 22 56

@clayjohn
Copy link
Member

I can't reproduce this on Beta8 the lighting brightness doesn't change between the forward_plus and mobile backends. This must have been fixed sometime in the last year by another change

@Calinou
Copy link
Member Author

Calinou commented Jan 8, 2023

I can still reproduce this in 4.0.beta 8a98110 (Linux, AMD Radeon RX 6900 XT with Mesa RADV).

You can check this in the demo projects' 4.0-dev branch on Truck Town or Platformer 3D after switching them to Forward Mobile: https://github.com/godotengine/godot-demo-projects/tree/4.0-dev

(Platformer 3D uses reflection probes, but Truck Town doesn't.)

Platformer 3D

Forward Plus Forward Mobile
Forward Plus Forward Mobile

Truck Town

Forward Plus Forward Mobile
Forward Plus Forward Mobile

PS: --rendering-method mobile is valid syntax, but it appears to do nothing on both demos, even though --rendering-method gl_compatibility works. I had to change the backend through the project settings to take the Forward Mobile screenshot.

@Calinou Calinou reopened this Jan 19, 2023
@BastiaanOlij
Copy link
Contributor

Hmmm, I'm not certain, I know one issue we have on mobile is that we only have 10 bits of color precision, not 16 bits, as we need to use 32bit color buffers, 64bit color buffers kill performance on mobile GPUs, while 32bit buffers often have additional optimisations to minimize bandwidth transfering data between tiled memory and texture storage.

The A2R10B10R10_UNORM color format used is also a normalised format, so valued from 0.0 - 1.0, which we scale up to 0.0 - 2.0

It can simply be a bug in how radiance colors are mixed in, possibly we're forgetting the take the luminence multiplier into account something. As we divide all colors by 2, to get the 0.0 - 2.0 range when we're storing 0.0 - 1.0, we might be forgetting to multiply it in these conditions.

@clayjohn clayjohn modified the milestones: 4.0, 4.x Feb 23, 2023
@clayjohn
Copy link
Member

clayjohn commented May 3, 2023

Looks more like an issue with blurring the radiance map. We are losing energy with successive applications of the cubemap roughness shader. By the time we reach the last level the entire thing is almost black.

Mobile
Screenshot from 2023-05-02 23-43-23

Clustered
Screenshot from 2023-05-02 23-41-07

Okay, after some exploration, it is clear that the issue comes from the cubemap downsampling phase

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.

4 participants