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

Implement bicubic sampling for lightmaps #89919

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BlueCube3310
Copy link
Contributor

@BlueCube3310 BlueCube3310 commented Mar 26, 2024

Supersedes #77912
Closes #49935

Adds support for bicubic sampling in LightmapGI, which results in much smoother-looking baked shadows:

Bilinear Bicubic
bilinear bicubic
bilinear bicubic

To do:

  • Add a project setting for using bicubic sampling,
  • Optimize for Mobile & Compatibility,
  • Don't break compatibility with older LightmapGIData files.

@BlueCube3310 BlueCube3310 requested review from a team as code owners March 26, 2024 19:38
@BlueCube3310 BlueCube3310 marked this pull request as draft March 26, 2024 19:46
@BlueCube3310 BlueCube3310 changed the title Implement support for bicubic lightmap sampling Implement bicubic sampling for lightmaps Mar 26, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Mar 27, 2024
@fire
Copy link
Member

fire commented Mar 27, 2024

We're unable to approve for merge if the github actions is failing.

https://docs.godotengine.org/en/stable/contributing/workflow/pr_workflow.html

@BlueCube3310 BlueCube3310 force-pushed the bicubic-lightmap branch 2 times, most recently from e533ddb to 9151cba Compare March 27, 2024 13:00
@jcostello
Copy link
Contributor

The bicubic sampling is adding artifacts around the edges of the boxes in the second example?

@BlueCube3310
Copy link
Contributor Author

The bicubic sampling is adding artifacts around the edges of the boxes in the second example?

Unfortunately yes, this sort of light bleeding happens with low-resolution lightmaps (3 x 256 x 256 for 38 meshes in this case).

@BlueCube3310 BlueCube3310 force-pushed the bicubic-lightmap branch 2 times, most recently from 59e4bb9 to 3f80ace Compare March 30, 2024 11:42
@BlueCube3310 BlueCube3310 marked this pull request as ready for review March 30, 2024 12:18
@BlueCube3310
Copy link
Contributor Author

The filtering can now be toggled from the ProjectSettings and the lightmaps are backwards-compatible with older ones, so upgrading should be seamless.

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 (rebased on top of master 29b3d9e), it works as expected in all rendering methods. Code looks good to me at a glance.

Toggling the project setting works without having to restart the project as well, which is nice for providing graphics options in projects. (This is an upgrade from 3.x where a restart was required when using GLES2, but not GLES3.)

Disabled Enabled
Disabled Enabled

PS: I noticed toggling bicubic filtering works on existing projects with lightmaps that were baked without this PR. They seem to look fine in this scenario, though I haven't looked extensively to see if they suffer more from lightmap bleeding when bicubic filtering is enabled compared to lightmaps baked with this PR.

Should we print a warning when bicubic filtering is enabled and a LightmapGIData lacks texture size information?

Also, note that this PR prevents using lightmaps baked after this PR is merged from working in older builds not including this PR. The scene will still load but lightmaps won't display until you bake lightmaps again. It's not a dealbreaker, just worth keeping in mind.

@akien-mga akien-mga requested a review from clayjohn April 4, 2024 11:45
@BlueCube3310
Copy link
Contributor Author

PS: I noticed toggling bicubic filtering works on existing projects with lightmaps that were baked without this PR. They seem to look fine in this scenario, though I haven't looked extensively to see if they suffer more from lightmap bleeding when bicubic filtering is enabled compared to lightmaps baked with this PR.

Should we print a warning when bicubic filtering is enabled and a LightmapGIData lacks texture size information?

If the texture size information isn't present in the file, it's taken from the lightmap textures instead.
The light bleeding is the same, regardless of which version of the file is used. The best way to mitigate it would probably be to increase the padding on the atlas textures in the lightmapper.

Co-authored-by: Calinou <hugo.locurcio@hugo.pro>
@@ -2474,6 +2474,7 @@ void RenderingServer::_bind_methods() {
ClassDB::bind_method(D_METHOD("light_directional_set_sky_mode", "light", "mode"), &RenderingServer::light_directional_set_sky_mode);

ClassDB::bind_method(D_METHOD("light_projectors_set_filter", "filter"), &RenderingServer::light_projectors_set_filter);
ClassDB::bind_method(D_METHOD("lightmaps_set_bicubic_filter", "filter"), &RenderingServer::lightmaps_set_bicubic_filter);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ClassDB::bind_method(D_METHOD("lightmaps_set_bicubic_filter", "filter"), &RenderingServer::lightmaps_set_bicubic_filter);
ClassDB::bind_method(D_METHOD("lightmaps_set_bicubic_filter", "enable"), &RenderingServer::lightmaps_set_bicubic_filter);

To match the docs. Might also be worth changing the C++ methods argument from p_use to p_enable for consistency, but it's not required.

<description>
Adds an object that is considered baked within this [LightmapGIData].
Adds an object that is considered baked within this [LightmapGIData]. [param texture_size] must match the size of the [i]entire[/i] lightmap texture, which is used for bicubic filtering when rendering the lightmap.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Adds an object that is considered baked within this [LightmapGIData]. [param texture_size] must match the size of the [i]entire[/i] lightmap texture, which is used for bicubic filtering when rendering the lightmap.
Adds an object that is considered baked within this [LightmapGIData]. [param texture_size] must match the size of the [i]entire[/i] lightmap texture which is used for bicubic filtering when rendering the lightmap.

No break here in English I'd say

@AThousandShips
Copy link
Member

You will also need to add compatibility binds

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.

Vulkan: LightmapGI bicubic sampling not implemented (unlike BakedLightmap in 3.x)
6 participants