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

Anamorphic Bloom #17096

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Anamorphic Bloom #17096

wants to merge 7 commits into from

Conversation

aevyrie
Copy link
Member

@aevyrie aevyrie commented Jan 2, 2025

Screen.Recording.2025-01-02.at.1.22.57.AM.mp4

The JJ Abrahams

Screen.Recording.2025-01-02.at.9.56.21.AM.mp4

The Cyberfunk 2025

image

Objective

  • Add the ability to scale bloom for artistic control, and to mimic anamorphic blurs.

Solution

  • Add a scale factor in bloom settings, and plumb this to the shader.

Testing

  • Added runtime-tweak-able setting to the bloom_3d/bloom_2d example

Showcase

image

  • Added scale parameter to Bloom to improve artistic control and enable anamorphic bloom.

@aevyrie aevyrie force-pushed the anamorphic-bloom branch 4 times, most recently from 7b020db to 0b518bb Compare January 2, 2025 10:10
@IQuick143 IQuick143 added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 2, 2025
@BenjaminBrienen BenjaminBrienen added the D-Shaders This code uses GPU shader languages label Jan 2, 2025
@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Jan 2, 2025
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Jan 2, 2025
examples/3d/bloom_3d.rs Outdated Show resolved Hide resolved
Copy link

@mate-h mate-h 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, just style changes

crates/bevy_core_pipeline/src/bloom/bloom.wgsl Outdated Show resolved Hide resolved
Copy link
Contributor

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

Approved pending SME feedback / benchmarking.

crates/bevy_core_pipeline/src/bloom/bloom.wgsl Outdated Show resolved Hide resolved
@tychedelia tychedelia added the S-Needs-SME Decision or review from an SME is required label Jan 3, 2025
@IceSentry
Copy link
Contributor

I don't have time to benchmark right now, but if someone benchmarks this and it shows no perf impact then the code LGTM. I'd just like to see some numbers first. My guess is that if there's an impact it will be pretty small, but would still be nice to know.

@aevyrie
Copy link
Member Author

aevyrie commented Jan 4, 2025

I added a shader def, so the fast path should still exist, and the new code is only compiled in when using nonuniform bloom scale.

@tychedelia tychedelia added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Needs-SME Decision or review from an SME is required S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Jan 4, 2025
@tychedelia tychedelia requested a review from mate-h January 4, 2025 03:04
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

With the ifdef I'm happy to merge it without benchmarking, but it would be nice to have numbers and yeet the ifdef if it's not necessary.

@IceSentry IceSentry 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 4, 2025
@aevyrie
Copy link
Member Author

aevyrie commented Jan 4, 2025

I'd like to try messing with the weights instead. After fixing the upscaling issue, non uniform blur looks worse because it can't rely on incorrect upsamples misaligned with the frags.

@aevyrie
Copy link
Member Author

aevyrie commented Jan 4, 2025

I couldn't find a decent path with weights without making more significant changes. I think this is good as-is now. I've tested with the car scene again with the latest settings, and I'm happy with how it looks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes D-Shaders This code uses GPU shader languages M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

7 participants