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

OpenGL: Implement rendering of lightmaps #85120

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Nov 20, 2023

This PR aims to add rendering of lightmaps (via LightmapGI) when using the GL Compatibility renderer.

Note: This doesn't add lightmap baking when using the GL Compatibility renderer. Lightmaps still need to be baked using Vulkan.

This marked as a draft because while it's "working", the result looks worse than I'd expect. I want to see if I can get to the bottom of that first.

UPDATE: Eh, it looks good enough to get some review :-)

UPDATE 2: In a comment below, @Calinou (who knows rendering much better than me) says it looks as good as he'd expect given the way the Compatibility renderer works, so I guess this just needs code review now!

@dsnopek dsnopek added this to the 4.x milestone Nov 20, 2023
@dsnopek dsnopek requested a review from clayjohn November 20, 2023 03:44
@dsnopek dsnopek requested review from a team as code owners November 20, 2023 03:44
@dsnopek dsnopek marked this pull request as draft November 20, 2023 03:44
@dsnopek dsnopek marked this pull request as ready for review November 20, 2023 04:14
@dsnopek
Copy link
Contributor Author

dsnopek commented Nov 20, 2023

FYI, here's some images of the visual differences...

Compatibility:
Selection_093

Mobile:
Selection_094

In both, you can see some "lines" by the edges of the triangles, but it's much more noticeable on the Compatibility renderer, and the light seems dimmer overall.

I'm surprised that the difference is so big (at least relatively speaking) given that it's the exact same lightmap (it was baked with the editor using the Mobile renderer) and the exact same shader math (I copy-pasted it from the Mobile renderer).

Any advice would be appreciated!

@Calinou
Copy link
Member

Calinou commented Nov 20, 2023

In both, you can see some "lines" by the edges of the triangles, but it's much more noticeable on the Compatibility renderer, and the light seems dimmer overall.

This is due to sRGB vs linear rendering. The mobile renderer has higher precision available (RGB10A2 with 2.0 maximum exposure). Compatibility is RGBA8, which is similar to Godot 3.x GLES2.

This result looks 100% expected to me – as good as it can be when using RGBA8 sRGB rendering, that is. To get a better result, you can play with the Tonemap properties in Environment depending on the rendering method, as visual differences in tonemapping between rendering methods are expected (though Forward+ and Mobile are usually close to each other).

In both, you can see some "lines" by the edges of the triangles, but it's much more noticeable on the Compatibility renderer, and the light seems dimmer overall.

The lines are due to bilinear interpolation coupled with low lightmap texel density, which makes it obvious. Godot 4.x doesn't support bicubic sampling in lightmaps yet, unlike Godot 3.x where it's present and enabled by default.

@dsnopek
Copy link
Contributor Author

dsnopek commented Nov 20, 2023

@Calinou Ah, thanks!

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, it mostly works as expected.

Testing project: test_lightmapgi_opengl.zip

One issue is that light sampling for all light types isn't skipped on meshes that use the Static global illumination mode and have had lightmaps baked on them. This is done by design when using Forward+ and Mobile to avoid double lighting and improve performance.

Edit: I noticed Mobile has broken behavior too. --rendering-method mobile doesn't actually work, even though the command line output says it's using Vulkan Mobile… it keeps using Forward+ if the project is configured to do so.

Forward+ Mobile Compatibility
Screenshot_20231120_175447 Screenshot_20231120_175831 Screenshot_20231120_175456

The torus closest to the camera uses the Disabled GI mode, while the torus on the back uses the Dynamic GI mode. The rendering issue on Mobile is tracked in #84846.

Dynamic object lighting also appears to not influence the object enough compared to other rendering methods.

@dsnopek
Copy link
Contributor Author

dsnopek commented Nov 20, 2023

@Calinou Thanks for the feedback!

One issue is that light sampling for all light types isn't skipped on meshes that use the Static global illumination mode and have had lightmaps baked on them.

I think this is fixed in my latest push.

It got kind of messy, because there's just so many ways a light can be rendered, so there needed to be checks in a whole bunch of places.

Also, I had to add 16 bytes to the LightData struct , because there wasn't any spare padding to use. This isn't ideal, but if we were going to add anything else to that struct in the future, then we would have hit this eventually, and now there's a whole bunch of padding for those future things to use. :-)

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, it works as expected. Light shadow sampling for static lights is now skipped on lightmapped surfaces. This brings performance up from 1600 FPS (0.62 mspf) to 2050 FPS (0.48 mspf) in 4K on a RTX 4090 in the testing project linked above.

It also works in the web export (FPS is V-Synced to 120 Hz):

image

Also working on a Samsung Galaxy Z Fold4 running Android 13:

Screenshot_20231121_130023_Test LightmapGI OpenGL

drivers/gles3/shaders/scene.glsl Outdated Show resolved Hide resolved
drivers/gles3/shaders/scene.glsl Outdated Show resolved Hide resolved
drivers/gles3/storage/light_storage.cpp Outdated Show resolved Hide resolved
drivers/gles3/rasterizer_scene_gles3.cpp Outdated Show resolved Hide resolved
drivers/gles3/rasterizer_scene_gles3.cpp Outdated Show resolved Hide resolved
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Just some little shader nitpicks.

When running the scene from Calinou, I am getting a totally black mesh. Looking into it with a debugger now

edit: there is nothing obviously wrong. But the shader has disabled all the lights for some reason despite lights touching the mesh.

This is what I am seeing
image

drivers/gles3/shaders/scene.glsl Outdated Show resolved Hide resolved
drivers/gles3/shaders/scene.glsl Outdated Show resolved Hide resolved
@dsnopek
Copy link
Contributor Author

dsnopek commented Dec 5, 2023

When running the scene from Calinou, I am getting a totally black mesh. Looking into it with a debugger now

edit: there is nothing obviously wrong. But the shader has disabled all the lights for some reason despite lights touching the mesh.

That really weird :-/

I just recompiled (after making the suggested lowp changes) and re-tested with Calinou's scene, and it's still working for me.

I'm not sure what could be the difference. I'm on Linux with an nvidia card. What's your environment like?

@clayjohn
Copy link
Member

clayjohn commented Dec 5, 2023

I'm not sure what could be the difference. I'm on Linux with an nvidia card. What's your environment like?

On linux with an intel integrated GPU

@clayjohn
Copy link
Member

clayjohn commented Dec 5, 2023

Okay, some progress. It looks like UV2s are disabled for some reason
image

I've been rebasing on master, so maybe something upstream unintentionally broke this (perhaps #85092)

_edit: reverting #85092 seems to do the trick _

@dsnopek
Copy link
Contributor Author

dsnopek commented Dec 5, 2023

I've been rebasing on master, so maybe something upstream unintentionally broke this

Ah, I just tried rebasing on master, and now I'm seeing what you're seeing too.

@clayjohn
Copy link
Member

clayjohn commented Dec 5, 2023

Diff with the fix:

diff --git a/drivers/gles3/rasterizer_scene_gles3.cpp b/drivers/gles3/rasterizer_scene_gles3.cpp
index 90440d6b73..18d4ae3562 100644
--- a/drivers/gles3/rasterizer_scene_gles3.cpp
+++ b/drivers/gles3/rasterizer_scene_gles3.cpp
@@ -2773,12 +2773,16 @@ void RasterizerSceneGLES3::_render_list_template(RenderListParameters *p_params,
 
                        GLuint vertex_array_gl = 0;
                        GLuint index_array_gl = 0;
+                       uint64_t vertex_input_mask = shader->vertex_input_mask;
+                       if (inst->lightmap_instance.is_valid()) {
+                               vertex_input_mask |= 1 << RS::ARRAY_TEX_UV2;
+                       }
 
-                       //skeleton and blend shape
+                       // Skeleton and blend shapes.
                        if (surf->owner->mesh_instance.is_valid()) {
-                               mesh_storage->mesh_instance_surface_get_vertex_arrays_and_format(surf->owner->mesh_instance, surf->surface_index, shader->vertex_input_mask, vertex_array_gl);
+                               mesh_storage->mesh_instance_surface_get_vertex_arrays_and_format(surf->owner->mesh_instance, surf->surface_index, vertex_input_mask, vertex_array_gl);
                        } else {
-                               mesh_storage->mesh_surface_get_vertex_arrays_and_format(mesh_surface, shader->vertex_input_mask, vertex_array_gl);
+                               mesh_storage->mesh_surface_get_vertex_arrays_and_format(mesh_surface, vertex_input_mask, vertex_array_gl);
                        }
 
                        index_array_gl = mesh_storage->mesh_surface_get_index_buffer(mesh_surface, surf->lod_index);

In the end this wasn't really a regression. The case of UV2s just needed special handling as they can be accessed either by the shader, or inside the shader with a particular specialization constant. So we need to ask for UV2s whenever we enable the spec constant that needs them.

Prior to #85092 we just used all vertex arrays all the time which was bad for performance

@dsnopek
Copy link
Contributor Author

dsnopek commented Dec 5, 2023

Thanks so much! Your patch fixes it in my testing too, so I've rebased, added your patch and pushed.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks good to me! Tested locally with Calinou's test project and I can confirm it now works.

@YuriSizov YuriSizov merged commit 44d544f into godotengine:master Dec 8, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

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.

5 participants