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

Document need for normal roughness texture conversion for CompositorEffects #9591

Closed
pink-arcana opened this issue Jul 11, 2024 · 7 comments · Fixed by godotengine/godot#95773
Labels
area:class reference Issues and PRs about the class reference, which should be addressed on the Godot engine repository enhancement

Comments

@pink-arcana
Copy link

pink-arcana commented Jul 11, 2024

Your Godot version: 4.3 beta 2

Issue description: (I'm working backwards from the source code for this, so apologies if I get the details wrong.)

As of godotengine/godot#86316, it appears that the normal roughness texture is stored in an optimized format. In order for it to work correctly with users' Spatial shaders, it looks like a conversion function (normal_roughness_compatibility()) is applied when the user samples the texture.

normal_roughness_texture before:
normal_before

normal_roughness_texture after normal_roughness_compatibility():
normal_after

In a CompositorEffect/compute shader, you have direct access to the texture that is stored in GPU memory. In my testing, applying the normal_roughness_compatibility() function to it manually in the compute shader gives you the same normals data that the Spatial shaders see.

The issue is that it is very difficult to discover, since you must first realize that there is a problem with the normal roughness texture (as you can see, it just looks a bit degraded), and then find this function in the source code.

Perhaps the need for this conversion can be documented along with the existing instructions for accessing the normal roughness texture in the CompositorEffect documentation linked below?

URL to the documentation page (if already existing): https://docs.godotengine.org/en/latest/classes/class_compositoreffect.html#class-compositoreffect-property-needs-normal-roughness

11/07/24: Edited for clarity and screenshots.

@tetrapod00
Copy link
Contributor

tetrapod00 commented Jul 26, 2024

This issue also seems to affect the older fullscreen quad postprocessing effects. There are commonly used outline shaders such as this one that rely on the old behavior of the normal buffer, and whose behavior changed between 4.2 and 4.3.

Is it possible for the reverse-Z blogpost to receive an edit with information about the normal buffer change, or a link to the documentation about it? Many of the users looking to update their old postprocessing effects will need both pieces of information, and are already able to find their way to the blogpost.

Edit: this case was addressed in godotengine/godot#94812.

@skyace65 skyace65 added the area:class reference Issues and PRs about the class reference, which should be addressed on the Godot engine repository label Jul 29, 2024
@tetrapod00
Copy link
Contributor

@pink-arcana Do you have a GLSL compute shader snippet using the normal_roughness_compatibility() function that you have confirmed works? I tried looking in https://github.com/pink-arcana/godot-distance-field-outlines but didn't see anything.

Working on getting this documented.

@pink-arcana
Copy link
Author

Thanks for creating the PR! In that repo, I renamed it to unpack_normal_roughness(). That's probably why it's hard to find, sorry! It's otherwise just copied directly from the original normal_roughness_compatibility().

I didn't test it vigorously. It was just (1) displaying the resulting normal image and verifying it doesn't have the artifacts, and then (2) using it in a simple normal sobel for outline detection, and it seemed to find the expected outlines. I don't think there's a reason to expect any problems with it, though, as long as the function is the same?

@tetrapod00
Copy link
Contributor

Just wanted to avoid documenting something unconfirmed. That's enough confirmation for me, since I also confirmed it myself in the other case of Spatial shaders before it was patched. Thanks!

@pink-arcana
Copy link
Author

pink-arcana commented Aug 18, 2024

Totally get it! I also want to be sure it's correct.

I just created a branch for that repo that uses normals. It's very basic but I think it works as expected (some artifacts in shallow areas from using a normal sobel alone). If you download that branch, play the demo, and choose the CE option, it will use normals for outline detection. I also output the result of the conversion function to a debug image that you can see in RenderDoc. This is pretty much the opposite of an MRP, but hopefully it's useful!

Edited to add before/after and collapse the screenshots.

Screenshots

Screenshot 2024-08-18 151027

Screenshot 2024-08-18 151103

Normal roughness texture - before/after

Before:

Screenshot 2024-08-18 152533

After:

Screenshot 2024-08-18 152538

@tetrapod00
Copy link
Contributor

Thanks, that's very helpful!
I'm personally satisfied with this level of confirmation already, but I'll check out the branch if more is required.

@pink-arcana
Copy link
Author

Added a before/after of the normal texture to the last comment, since that's probably the more helpful view!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:class reference Issues and PRs about the class reference, which should be addressed on the Godot engine repository enhancement
Projects
None yet
3 participants