-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[Merged by Bors] - adjust cluster index for viewport origin #5947
Conversation
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.
I can confirm that this fixes #5946. I don't know much about the implementation so I can't check whether this is the only place where frag_coord
needs to be adjusted, but everything seems to work in practice.
i've left the 2d view analog untouched since we don't use frag_coord there. do we want to update that too? |
answering my own question, yes we need to update 2d since they source from the same rust-side Uniform struct (there's a separate PR to make 2d use the same wgsl view definition that would have naturally fixed this anyway). also added a util to convert @Builtin(position) to viewport uv since |
width: f32, | ||
height: f32, | ||
// viewport(x_origin, y_origin, width, height) | ||
viewport: vec4<f32>, |
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.
could you import from bevy_sprite::mesh2d_view_types
instead of redefining it here?
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.
Good idea, I think
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.
Or maybe it should really be a shader imports defined in bevy_render and imported everywhere else…
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 is done in #5535
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.
Very close to approval :)
width: f32, | ||
height: f32, | ||
// viewport(x_origin, y_origin, width, height) | ||
viewport: vec4<f32>, |
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.
Good idea, I think
width: f32, | ||
height: f32, | ||
// viewport(x_origin, y_origin, width, height) | ||
viewport: vec4<f32>, |
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.
Or maybe it should really be a shader imports defined in bevy_render and imported everywhere else…
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.
bors r+
# Objective fixes #5946 ## Solution adjust cluster index calculation for viewport origin. from reading point 2 of the rasterization algorithm description in https://gpuweb.github.io/gpuweb/#rasterization, it looks like framebuffer space (and so @bulitin(position)) is not meant to be adjusted for viewport origin, so we need to subtract that to get the right cluster index. - add viewport origin to rust `ExtractedView` and wgsl `View` structs - subtract from frag coord for cluster index calculation
Pull request successfully merged into main. Build succeeded: |
# Objective fixes bevyengine#5946 ## Solution adjust cluster index calculation for viewport origin. from reading point 2 of the rasterization algorithm description in https://gpuweb.github.io/gpuweb/#rasterization, it looks like framebuffer space (and so @bulitin(position)) is not meant to be adjusted for viewport origin, so we need to subtract that to get the right cluster index. - add viewport origin to rust `ExtractedView` and wgsl `View` structs - subtract from frag coord for cluster index calculation
# Objective fixes bevyengine#5946 ## Solution adjust cluster index calculation for viewport origin. from reading point 2 of the rasterization algorithm description in https://gpuweb.github.io/gpuweb/#rasterization, it looks like framebuffer space (and so @bulitin(position)) is not meant to be adjusted for viewport origin, so we need to subtract that to get the right cluster index. - add viewport origin to rust `ExtractedView` and wgsl `View` structs - subtract from frag coord for cluster index calculation
# Objective fixes bevyengine#5946 ## Solution adjust cluster index calculation for viewport origin. from reading point 2 of the rasterization algorithm description in https://gpuweb.github.io/gpuweb/#rasterization, it looks like framebuffer space (and so @bulitin(position)) is not meant to be adjusted for viewport origin, so we need to subtract that to get the right cluster index. - add viewport origin to rust `ExtractedView` and wgsl `View` structs - subtract from frag coord for cluster index calculation
Objective
fixes #5946
Solution
adjust cluster index calculation for viewport origin.
from reading point 2 of the rasterization algorithm description in https://gpuweb.github.io/gpuweb/#rasterization, it looks like framebuffer space (and so @bulitin(position)) is not meant to be adjusted for viewport origin, so we need to subtract that to get the right cluster index.
ExtractedView
and wgslView
structs