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

Z coordinate in shaders is not the same across Compatibility and Forward+/Mobile #100310

Open
aganm opened this issue Dec 12, 2024 · 3 comments
Open

Comments

@aganm
Copy link
Contributor

aganm commented Dec 12, 2024

Tested versions

Tested and reproducible in Godot 4.3 stable and Godot 4.4 dev6.

System information

Godot v4.3.stable - Windows 10.0.19045 - Vulkan (Forward+) - dedicated Radeon RX 580 Series (Advanced Micro Devices, Inc.; 31.0.21912.14)

Issue description

This issue explains the problem tiffany352/godot-starlight#2

To sum it up, the background shader gets rendered on top of a cube object as you can see in this image, notice the little white spots on top of the cube:

Image

The problem has been identified with this line:

https://github.com/tiffany352/godot-starlight/blob/bc7a4cc22b53fc42986a6c9c0fe369f86b8f3a9d/addons/starlight/Star.gdshader#L65

The solution is to change the way the background shader handles depth.

For me, this solution works for Compatibility renderer:

POSITION.z = -1.0 * projected.w;

Godot v4.3.stable - Windows 10.0.19045 - GLES3 (Compatibility) - Radeon RX 580 Series (Advanced Micro Devices, Inc.; 31.0.21912.14)

But Forward+/Mobile, I need to do that instead:

POSITION.z = 1.0 / projected.w;

Godot v4.3.stable - Windows 10.0.19045 - Vulkan (Forward+) - dedicated Radeon RX 580 Series (Advanced Micro Devices, Inc.; 31.0.21912.14)

Steps to reproduce

Open https://github.com/tiffany352/godot-starlight in Godot 4.3 and move the 3d viewport camera in front of the cube object in the scene, and observe how the background renders on the cube. Switch renderer between Forward+/Mobile and Compatibility.

Minimal reproduction project (MRP)

https://github.com/tiffany352/godot-starlight

@tetrapod00
Copy link
Contributor

I believe this is a documentation issue. I don't quite understand all the details well enough to fully explain them, but there are differences in the internal coordinate spaces of the Compatibility and the Forward+/Mobile renderers that won't be changed for compatibility reasons. One of those differences is the z value of the far clip plane. godotengine/godot-proposals#10273 gets into it a little.

In 4.4 we now have access to CLIP_SPACE_FAR (#95057) and preprocessor defines (#98549), so you now have the tools to write shaders that will work in all renderers.

In this case, if the comment is accurate and the goal is to always render at the far clip plane, you might be able to use:
POSITION.z = CLIP_SPACE_FAR;

I think that the existing docs issue godotengine/godot-docs#10204 in theory covers this case. But we should probably leave this issue open, to track specifically documenting the far clip z value.

@clayjohn clayjohn moved this from For team assessment to Up for grabs in Rendering Issue Triage Dec 12, 2024
@aganm
Copy link
Contributor Author

aganm commented Dec 13, 2024

Thank you for the addition.

I'm not the author of the original shader so I can't comment about the accuracy of the comment.

All I can say is that POSITION.z = CLIP_SPACE_FAR; unfortunately does not fix the issue (the background stars still render on top of the object and the wanted effect is that they render behind it).

@tetrapod00
Copy link
Contributor

tetrapod00 commented Dec 13, 2024

Take this with a grain of salt. I know enough to use shaders but my knowledge is still fuzzy.

I think that using CLIP_SPACE_FAR directly doesn't work because the w value of POSITION is not 1.0?

Here are four options to get a working version of the shader that works across renderers in 4.3 and 4.4. Tested in 4.4dev5, with the demo scene from the starlight project. I didn't bother testing the Mobile renderer.

// 4.2 and earlier.

// Before reverse-z, 1.0 is the far clip value in all renderers.
POSITION.z = 1.0 * projected.w;

// 4.3+ First option:

// OUTPUT_IS_SRGB is true in Compatibility, false otherwise.
// This is the solution you found.
if (OUTPUT_IS_SRGB) {
	POSITION.z = -1.0 * projected.w;
} else {
	POSITION.z = 1.0 / projected.w;
}

// 4.3+ Second option:

// After reverse-z, 1.0 is the near clip value in all renderers.
// -1.0 is the far clip value in Compatibility.
// 0.0 is the far clip value in Forward+/Mobile.

// Instead of dividing 1.0 to get a very small number, use 0.0 directly.
// Now we are using the same formula as we were before 4.3, just with different values.
if (OUTPUT_IS_SRGB) {
	POSITION.z = -1.0 * projected.w;
} else {
	POSITION.z = 0.0 * projected.w;
}

// 4.4+ First option:

// Use the new preprocessor define instead of the OUTPUT_IS_SRGB hack.
#if CURRENT_RENDERER == RENDERER_COMPATIBILITY
POSITION.z = -1.0 * projected.w;
#else
POSITION.z = 0.0 * projected.w;
#endif

// 4.4+ Second option:

// Use a single line again.
// CLIP_SPACE_FAR is -1.0 in Compatibility, and 0.0 otherwise.
POSITION.z = CLIP_SPACE_FAR * projected.w;

I hope this helps, and maybe someone more knowledgeable can confirm that this solution theoretically sound.

(Also, if this works please pass this on to the author. I just realized this is actually the second time I've helped someone port this shader to 4.3+)

tiffany352 added a commit to tiffany352/godot-starlight that referenced this issue Dec 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Up for grabs
Development

No branches or pull requests

4 participants