-
-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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 proximity fade's world_pos
is calculated incorrectly when in gl_compatibility
#89966
Conversation
if (OS::get_singleton()->get_current_rendering_method() == "gl_compatibility") { | ||
code += " depth_tex = depth_tex * 2.0 - 1.0;"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make this part be introduced by the shader preprocessor, instead of a C++ condition? This way, if you convert the BaseMaterial3D to a ShaderMaterial (making its shader code fixed), it'll keep working if you switch rendering methods afterwards. This is also important to handle situations where a project can fall back to Compatibility if Vulkan isn't supported.
(Also, I thought we already had automatic depth correction when using the Compatibility rendering method, so we wouldn't have to do this.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed it would be great if the depth_texture was driver agnostic, so the same shader code would work regardless of renderer. Or a preprocessor macro that converts it into 0-1 (and does nothing if Vulkan).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry for my previous explaination of this problem, let me put it more clearly: depth is 0-1 but the ndc is different in opengl and vulkan. It's introduced in #54285, it changed to vulkan way of ndc so it caused this issue.
Visual shader also use an if here, see #74910. But we'd better not use this approach as Calinou mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 4.4+ we now have a way to do this with preprocessor defines. I made a PR: #100220.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 4.4+ we now have a way to do this with preprocessor defines. I made a PR: #100220.
Seems pretty reasonable. Closing this pr now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your approach may still end up being the right one, actually. It looks like preprocessor macros don't quite work right now in the generated shaders in materials. We'll see if that ends up being considered a bug or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like preprocessor macros don't quite work right now in the generated shaders in materials.
I suppose this can be tweaked a little? Not sure it's a bug or not, but worth investigating.
depth_tex
is set incorrectly in gl_compatibility rendering methodworld_posx
is calculated incorrectly when in gl_compatibility rendering method
world_posx
is calculated incorrectly when in gl_compatibility rendering methodworld_pos
is calculated incorrectly when in gl_compatibility
Fixes #89942
OpenGL ndc's z range is-1.0 to 1.0 by default while the Vulkan is 0.0 to 1.0.