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

Fix VRS issues #68710

Merged
merged 1 commit into from
Nov 18, 2022
Merged

Fix VRS issues #68710

merged 1 commit into from
Nov 18, 2022

Conversation

BastiaanOlij
Copy link
Contributor

@BastiaanOlij BastiaanOlij commented Nov 16, 2022

This fixes 3 2 VRS issues:

Adhere to min and max texel size (done)

In various places we were hardcoding a 16x16 texel size. This seemed to be the accepted default size but some cards now deviate from this. We're still attempting to default to this but we now adhere to the minimum and maximum texel size provided by the GPU driver.
At some point we should make the default size settable but for now this suffices.

Improve provided VRS map (done but needs tweaking)

Values in the VRS map are hard to work with as we need values between rgb(0,0,0) and rgb(16,0,0). This is fine for generated maps (like XR maps) but when the map is provided as an image this is very user unfriendly.

As we're copying the user provided map into our internal VRS map to adjust size and go to the appropriate data format, we can do a color map. The suggestion was to encode the X texel density in red and Y texel density in green.

A value of 0 would be 1, 128 would be 2 and 255 would be 4 ergo:

  • 1x1 = rgb(0, 0, 0)
  • 1x2 = rgb(0, 85, 0)
  • 2x1 = rgb(85, 0, 0)
  • 2x2 = rgb(85, 85, 0)
  • 2x4 = rgb(85, 170, 0)
  • 4x2 = rgb(170, 85, 0)
  • 4x4 = rgb(170,170, 0)
  • 4x8 = rgb(170, 255, 0)
  • 8x4 = rgb(255,170, 0)
  • 8x8 = rgb(255,255, 0)

Note that 1x4, 4x1, 1x8, 8x1, 2x8 and 8x2 are not supported
Also 4x8, 8x4 and 8x8 are only supported on some GPUs

I'm not sure what GPUs do when unsupported values appear in the density map.

Lastly, we might need to tune the shader a bit in case the float -> uint conversion isn't rounding properly.
Also Godot might treat the colors in the texture as sRGB and do an sRGB->Linear conversion, that may cause issues.

@BastiaanOlij BastiaanOlij added this to the 4.0 milestone Nov 16, 2022
@BastiaanOlij BastiaanOlij self-assigned this Nov 16, 2022
@BastiaanOlij
Copy link
Contributor Author

Ok, I've got the color conversion working, there are probably a few little tweaks needed to dial it in.
Attached is a map that should go up to 4x4:
vrs

I still can't reproduce the issue of VRS being applied to 2D rendering and can't find anything that would explain that it would apply it. Very weird.

@Calinou
Copy link
Member

Calinou commented Nov 16, 2022

Tested on Windows 10 with updated project (AMD Radeon RX 6900 XT, Adrenalin 22.9.1): test_vrs.zip

VRS disabled

godot windows editor x86_64_Zv9PXsZLSs

VRS enabled

godot windows editor x86_64_y2Q8myoU1W

The new density map is working well, but VRS still affects 2D rendering. Edges are also still affected by VRS, also sporting an antialiased look even though all forms of antialiasing are disabled in the project.

I've tried disabling sky debanding out of curiosity to see if it had any effect, but it didn't change how VRS looked.

@BastiaanOlij
Copy link
Contributor Author

@Calinou I suspect the anti-aliasing look is just how VRS is implemented on your hardware. Instead of duplicating the fragment output for the given density block, it looks like its filtering to the neighboring pixels. It's definitely not something we're doing. Possibly worth asking the AMD guys to see if this is indeed something in their VRS implementation. If it is indeed a feature I think its a nice one as it prevents the lower density areas from getting a pixelated look.

I'm still clueless on why the 2D parts get the same treatment, this makes no sense. As far as I can tell the 2D rendering is done with a framebuffer that doesn't have VRS enabled so it must somehow retain something. Remember we're rendering 3D content into the RGBA16F buffers, this is all done with VRS enabled, we then copy that during the tonemapping stage into the render target buffers, and it is in those that we render the 2D content. So it should all be separated.

This is what it looks like on mine:
image

I'm not sure what the weird dithering on the background is but zooming in you can clearly see that it's not applying a filter or aliasing and that the 2D rendering is working ok.

@BastiaanOlij
Copy link
Contributor Author

I'm parking the issue with VRS being wrongfully applied to 2D rendering because at the moment we have no idea why this is happening sometimes (or on some hardware) and so we can merge the two things we did fix .

@BastiaanOlij BastiaanOlij marked this pull request as ready for review November 16, 2022 23:56
@BastiaanOlij BastiaanOlij requested review from a team as code owners November 16, 2022 23:56
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

The code looks good to me, but I am concerned about the remapping. Does this mean that users will have to discover that remapping somehow in order to use the VRS map? Or is the remapping just for visualization purposes? If users are supposed to supply the remapped values, we need to do something more to communicate what the VRS texture expects.

@BastiaanOlij
Copy link
Contributor Author

@clayjohn this is because its nearly impossible to manually create a VRS map with the actual values used seeing we're working with rgb(0,0,0) - rgb(16,0,0), so 16 shades of pitch black.

This will need to be documented in how VRS is used, which is what started this whole journey when @Calinou attempted that.

@Calinou
Copy link
Member

Calinou commented Nov 17, 2022

I'm currently in the process of writing VRS docs. I'll make sure to document this and include an example VRS texture 🙂

That said, we should still document the expected color mapping in the class reference.

In doc/classes/Viewport.xml's vrs_texture property description and doc/classes/ProjectSettings.xml's rendering/vrs/texture description, copy the same paragraph at the end:

The texture must use a lossless compression format so that colors can be matched precisely. The following VRS densities are mapped to various colors, with brighter colors representing a lower level of shading 
precision:
[codeblock]
- 1x1 = rgb(0, 0, 0)     - #000000
- 1x2 = rgb(0, 85, 0)    - #005500
- 2x1 = rgb(85, 0, 0)    - #550000
- 2x2 = rgb(85, 85, 0)   - #555500
- 2x4 = rgb(85, 170, 0)  - #55aa00
- 4x2 = rgb(170, 85, 0)  - #aa5500
- 4x4 = rgb(170, 170, 0) - #aaaa00
- 4x8 = rgb(170, 255, 0) - #aaff00 - Not supported on all hardware
- 8x4 = rgb(255,170, 0)  - #ffaa00 - Not supported on all hardware
- 8x8 = rgb(255,255, 0)  - #ffff00 - Not supported on all hardware
[/codeblock]

Hexadecimal color codes are included for easy copy-pasting across software.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Okay, that makes sense to me as long as authoring the image is not harder than it was before this should be good to go

@akien-mga akien-mga merged commit 58cb11b into godotengine:master Nov 18, 2022
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants