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.

Sign up for GitHub

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

Jump to bottom

Higher quality bicubic lightmap sampling #16740

Merged
merged 12 commits into from
Jan 12, 2025
Merged

Higher quality bicubic lightmap sampling #16740

merged 12 commits into from
Jan 12, 2025

Conversation

JMS55
Copy link
Contributor

@JMS55 JMS55 commented Dec 10, 2024 •
edited
Loading

Objective

Solution

Testing

  • Did you test these changes? If so, how?
    • Ran on lightmapped example. Practically no difference in that scene.
  • Are there any parts that need more testing?
    • Lightmapping a better scene.

Changelog

  • Lightmaps now have a higher quality bicubic sampling method (off by default).

@JMS55 JMS55 added the A-Rendering Drawing game state to the screen label Dec 10, 2024
@JMS55 JMS55 requested a review from pcwalton December 10, 2024 05:48
@JMS55 JMS55 added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Dec 10, 2024
@DGriffin91
Copy link
Contributor

imo this should be optional (with at least linear being the other option). Try with a low res light map to see the effect.

pcwalton
pcwalton approved these changes Dec 10, 2024
View reviewed changes
Copy link
Contributor

@pcwalton pcwalton 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 modulo a little inaccuracy.

I could go either way on exposing an option for bilinear, and I won't block the PR on that. A quote from the Bakery author's blog post: https://ndotl.wordpress.com/2018/08/29/baking-artifact-free-lightmaps/

Bicubic interpolation. If you are not shipping on mobile, there are exactly 0 reasons to not use bicubic interpolation for lightmaps. Many UE3 games in the past did that, and it is a great trick. But some engines (Unity, I’m looking at you) still think they can get away with a single bilinear tap in 2018. Bicubic hides low resolution and makes jagged lines appear smooth. Sometimes I see people fixing jagged sharp shadows by super-sampling during bake, but it feels like a waste of lightmapping time to me.

See the post for a screenshot comparison.

The "if you are not shipping on mobile" comment is 6 years old, so it may well just be time to say bicubic everywhere.

crates/bevy_pbr/src/lightmap/lightmap.wgsl Outdated Show resolved Hide resolved
crates/bevy_pbr/src/lightmap/lightmap.wgsl Show resolved Hide resolved
crates/bevy_pbr/src/lightmap/lightmap.wgsl Outdated Show resolved Hide resolved
@DGriffin91
Copy link
Contributor

DGriffin91 commented Dec 11, 2024 •
edited
Loading

Based on these images from @pcwalton I don't think bicubic is correct: https://discord.com/channels/691052431525675048/743663924229963868/1316259592308527134

image

@JMS55
Copy link
Contributor Author

JMS55 commented Dec 14, 2024

Consensus is to make bicubic vs bilinear a toggle, and default to bilinear. Bicubic is oftentimes only a marginal quality improvement, can lead to light leaks, and is more expensive.

@JMS55
Copy link
Contributor Author

JMS55 commented Dec 15, 2024

I think prepass + deferred might be broken on main. I don't see any usage of lightmap_image in queue_prepass_material_meshes, unlike how it's used in queue_material_meshes.

@alice-i-cecile alice-i-cecile added the C-Feature A new feature, making something new possible label Dec 15, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 15, 2024
@alice-i-cecile alice-i-cecile removed this pull request from the merge queue due to a manual request Dec 15, 2024
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 15, 2024
@alice-i-cecile
Copy link
Member

@DGriffin91 @JMS55 what's your final opinion on the status of this? Did d224365 resolve the concerns?

@JMS55
Copy link
Contributor Author

JMS55 commented Dec 15, 2024 •
edited
Loading

It's fine now imo, but lightmaps being broken with deferred needs fixing first, so that I can setup the shaderdefs for bicubic sampling in the deferred gbuffer pass properly.

@alice-i-cecile alice-i-cecile changed the title Higher quality lightmap sampling Higher quality bicubic lightmap sampling Dec 15, 2024
@alice-i-cecile alice-i-cecile added S-Blocked This cannot move forward until something else changes and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Dec 15, 2024
@JMS55
Copy link
Contributor Author

JMS55 commented Dec 23, 2024

Blocked on #16836, and then once that's merged I need to update the deferred queuing code.

@JMS55 JMS55 marked this pull request as draft January 3, 2025 01:44
@JMS55 JMS55 added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Blocked This cannot move forward until something else changes labels Jan 11, 2025
@JMS55 JMS55 marked this pull request as ready for review January 11, 2025 19:28
@JMS55 JMS55 requested a review from pcwalton January 11, 2025 19:42
@pcwalton pcwalton mentioned this pull request Jan 12, 2025
Merged
pcwalton
pcwalton approved these changes Jan 12, 2025
View reviewed changes
Copy link
Contributor

@pcwalton pcwalton left a comment

Choose a reason for hiding this comment

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

Looks fine, tested and working. I submitted a very small PR to your repo: JMS55#29 The PR makes the feature easier to test by adding a --bicubic option to the lightmaps example.

crates/bevy_pbr/src/lightmap/lightmap.wgsl
let p1 = (vec2(iuv.x + h1x, iuv.y + h0y) - 0.5) * texel_size;
let p2 = (vec2(iuv.x + h0x, iuv.y + h1y) - 0.5) * texel_size;
let p3 = (vec2(iuv.x + h1x, iuv.y + h1y) - 0.5) * texel_size;
let color = g0(fuv.y) * (g0x * sample(p0, lightmap_slot) + g1x * sample(p1, lightmap_slot)) + g1(fuv.y) * (g0x * sample(p2, lightmap_slot) + g1x * sample(p3, lightmap_slot));
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure I already reviewed this, so I'm just assuming this is correct :)

crates/bevy_pbr/src/lightmap/mod.rs
///
/// Bicubic sampling is higher quality, but slower, and may lead to light leaks.
///
/// If true, the lightmap texture's sampler must be set to [`bevy_image::ImageSampler::linear`].
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to mention that the default is false.

@JMS55
Merge pull request #29 from pcwalton/JMS55-lightmap-sampling-cli-arg
d8f52df 
Add a `--bicubic` switch to the `lightmaps` example
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 12, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 12, 2025
Merged via the queue into bevyengine:main with commit bb0a82b Jan 12, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@pcwalton pcwalton pcwalton approved these changes

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Rendering
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use bicubic filtering in lightmaps
4 participants
@JMS55 @DGriffin91 @alice-i-cecile @pcwalton