Skip to content

Fix error/typo in SMAA shader#14338

Merged
mockersf merged 2 commits intobevyengine:mainfrom
Luracasmus:patch-1
Jul 15, 2024
Merged

Fix error/typo in SMAA shader#14338
mockersf merged 2 commits intobevyengine:mainfrom
Luracasmus:patch-1

Conversation

@Luracasmus
Copy link
Contributor

Objective

  • Actually use the value assigned to d_xz, like in the original SMAA implementation. This not already being the case was likely a mistake when converting from HLSL to WGSL

Solution

  • Use d_xz.x and d_xz.y instead of d.x and d.z

Testing

  • Quickly tested on Windows 11, x86_64-pc-windows-gnu 1.79.0 with the latest NVIDIA drivers. App runs with SMAA enabled and everything seems to work as intended
  • I didn't observe any major visual difference between this and the previous version, though this should be more correct as it matches the original SMAA implementation

Copy link
Contributor

@rparrett rparrett left a comment

Choose a reason for hiding this comment

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

Makes sense to me. This sort of nonsense is necessary due to gpuweb/gpuweb#737.

@alice-i-cecile alice-i-cecile requested a review from pcwalton July 15, 2024 16:22
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Jul 15, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14.1 milestone Jul 15, 2024
@rparrett
Copy link
Contributor

Checked the output of anti_aliasing with high and ultra SMAA, and it is identical before/after. Not sure if this is expected for that scene or indicative of some other bug.

@mockersf mockersf added this pull request to the merge queue Jul 15, 2024
Merged via the queue into bevyengine:main with commit 9a1a84d Jul 15, 2024
@Luracasmus Luracasmus deleted the patch-1 branch July 17, 2024 13:24
mockersf pushed a commit that referenced this pull request Aug 2, 2024
# Objective

- Actually use the value assigned to `d_xz`, like in [the original SMAA
implementation](https://github.com/iryoku/smaa/blob/master/SMAA.hlsl#L960).
This not already being the case was likely a mistake when converting
from HLSL to WGSL

## Solution

- Use `d_xz.x` and `d_xz.y` instead of `d.x` and `d.z`

## Testing

- Quickly tested on Windows 11, `x86_64-pc-windows-gnu` `1.79.0` with
the latest NVIDIA drivers. App runs with SMAA enabled and everything
seems to work as intended
- I didn't observe any major visual difference between this and the
previous version, though this should be more correct as it matches the
original SMAA implementation
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 S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants