-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
add tonemapping LUT bindings for sprite and mesh2d pipelines #13262
add tonemapping LUT bindings for sprite and mesh2d pipelines #13262
Conversation
c1e2911
to
5bcaaac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this bindings are defined in the separate file because of the bevyengine/naga_oil#90
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe link that issue in the file or where it is imported? I couldn't figure out from skimming the issue if you feel like a cleaner solution is possible later.
5bcaaac
to
42aac10
Compare
Rebased due to the conflcts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, just looks like half the diff is miscellaneous cleanup? Not a problem, just want to make sure I'm not missing something. Couple of unimportant style things. Otherwise lgtm
let view_layout = render_device.create_bind_group_layout( | ||
"mesh2d_view_layout", | ||
&BindGroupLayoutEntries::sequential( | ||
&BindGroupLayoutEntries::with_indices( |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I read in the Bevy Discord, the common consensus is that it's better to use explicit indices instead of sequential
let view_bind_group = render_device.create_bind_group( | ||
"mesh2d_view_bind_group", | ||
&mesh2d_pipeline.view_layout, | ||
&BindGroupEntries::with_indices(( |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@@ -528,13 +554,46 @@ pub fn queue_sprites( | |||
} | |||
|
|||
#[allow(clippy::too_many_arguments)] | |||
pub fn prepare_sprites( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This diff is pretty gnarly but it looks like it's just a (unrelated?) refactor of this system into two, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not unrelated. These systems are split into two because one of them does per-sprite setup, prepare_sprite_image_bind_groups
, and the other one does per-view setup, prepare_sprite_view_bind_groups
.
Before this PR, it was done in one system because earlier view bind groups were always the same, even for different views. Now, since different views (cameras) can have different tonemapping settings, we need to be able to provide different LUTs for different views.
It's pretty close to what is currently done for
@jnhyatt thank you for the review ❤️
tonemapping_shared.wgsl can not be imported unless |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, also tested tonemapping example on wasm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe link that issue in the file or where it is imported? I couldn't figure out from skimming the issue if you feel like a cleaner solution is possible later.
@arcashka once merge conflicts are resolve I'll merge this in :) |
42aac10
to
d5a0e45
Compare
Rebased and resolved merge conflicts :) |
Fixes #13118
If you use
Sprite
orMesh2d
and createCamera
withYou would get
Because of missing tonemapping LUT bindings
Solution
Add missing bindings for tonemapping LUT's to
SpritePipeline
&Mesh2dPipeline
Testing
I checked that
tonemapping
color_grading
sprite_animations
2d_shapes
meshlet
deferred_rendering
examples are still working
2d cases I checked with this code:
Changelog
Fix the bug which led to the crash when user uses any tonemapper without hdr for rendering sprites and 2d meshes.