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

[Merged by Bors] - adjust cluster index for viewport origin #5947

Closed
wants to merge 5 commits into from

Conversation

robtfm
Copy link
Contributor

@robtfm robtfm commented Sep 11, 2022

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

@jakobhellermann jakobhellermann added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels Sep 11, 2022
Copy link
Contributor

@jakobhellermann jakobhellermann left a 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.

@robtfm
Copy link
Contributor Author

robtfm commented Sep 11, 2022

i've left the 2d view analog untouched since we don't use frag_coord there. do we want to update that too?

@robtfm
Copy link
Contributor Author

robtfm commented Sep 13, 2022

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 let uv = (position.xy - view.viewport.xy) / view.viewport.zw; is a bit obtuse for a user-facing example

Comment on lines -9 to +10
width: f32,
height: f32,
// viewport(x_origin, y_origin, width, height)
viewport: vec4<f32>,
Copy link
Member

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea, I think

Copy link
Contributor

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…

Copy link
Contributor Author

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

Copy link
Contributor

@superdump superdump left a 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 :)

crates/bevy_pbr/src/render/utils.wgsl Outdated Show resolved Hide resolved
Comment on lines -9 to +10
width: f32,
height: f32,
// viewport(x_origin, y_origin, width, height)
viewport: vec4<f32>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea, I think

Comment on lines -9 to +10
width: f32,
height: f32,
// viewport(x_origin, y_origin, width, height)
viewport: vec4<f32>,
Copy link
Contributor

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…

Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot pushed a commit that referenced this pull request Sep 15, 2022
# 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
@bors bors bot changed the title adjust cluster index for viewport origin [Merged by Bors] - adjust cluster index for viewport origin Sep 15, 2022
@bors bors bot closed this Sep 15, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 19, 2022
# 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
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# 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
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# 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
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-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Camera with Viewport has blocky lighting bugs (clustered forward rendering related?)
4 participants