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

Fix lightmapper seam blending #91985

Merged
merged 1 commit into from
May 15, 2024

Conversation

passivestar
Copy link
Contributor

Closes #91614

Before After
before after

@@ -428,6 +428,7 @@ void LightmapperRD::_create_acceleration_structures(RenderingDevice *rd, Size2i
SWAP(edge.a, edge.b);
SWAP(edge.na, edge.nb);
SWAP(uv2.a, uv2.b);
SWAP(uv2.indices.x, uv2.indices.y);
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Narrator: "It was."

Copy link
Member

Choose a reason for hiding this comment

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

Narrator: "And just like that, the issue was resolved. A single line of code, the smallest of changes, had made all the difference."

@BlueCube3310
Copy link
Contributor

Does this also fix #90929?

@passivestar
Copy link
Contributor Author

Does this also fix #90929?

This fixes a specific bug with vertex index mismatch that caused super sharp stripes when blending seams

I believe #91601 improved the situation with light leaks that are denoider-related

But there's still lots of room for improvement after our 2 fixes

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, this doesn't appear to do much against the seams in the Global Illumination demo project:

Before After (this PR)
Screenshot_20240515_185503 Screenshot_20240515_190512

Testing project: global_illumination.zip (bake the LightmapGIAll node)

@BlueCube3310
Copy link
Contributor

BlueCube3310 commented May 15, 2024

These ones seem to be caused by the lightmap UVs being too small and packed too tightly, making the texture bleed onto its neighbours.

Edit: The lightmap looks like this, so leaks are bound to occur:
lightmap

@passivestar
Copy link
Contributor Author

this doesn't appear to do much against the seams in the Global Illumination demo project

yeah because those aren't caused by wrong vertex indeces. There's more than one issue with lightmaps

If I had to guess those are most likely due to wrong UV offset calculation during seam blending

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 great for a Yolo merge before Beta.

For context, that code block swaps all the other properties, so the change makes intuitive sense and the results speak for themselves

@permelin
Copy link
Contributor

I was so happy that I found the bug, only to see now that you beat me to it. (Why doesn't Github notify me when there's a PR that would close an issue that I'm subscribed to?)

When the bug is fixed, the blending effect is so subtle that it's difficult to be sure whether it's working or just broken to the point where it does nothing. But there are actually some seams that look smoother with it.

The left is with seam blending disabled, the right is with the fix.

blendseams-before-after

While I was debugging, I noticed that the texels that are blended often don't perfectly line up with the actual seam. But that is a completely different issue.

@passivestar
Copy link
Contributor Author

@permelin I hope you'll beat me to fixing the bug that @Calinou pointed out because I'm not looking forward to fixing that 😂

@akien-mga akien-mga merged commit 42b60c1 into godotengine:master May 15, 2024
16 checks passed
@akien-mga
Copy link
Member

akien-mga commented May 15, 2024

Thanks! Great work tracking this down 🥇

@passivestar passivestar deleted the fix-lightmapper-seams branch May 15, 2024 20:49
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.

LightmapGI: Stripes on curved surfaces
8 participants