-
-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
[3.x] Fix fragcolor write locations in scene shaders #92070
Conversation
// Use a temporary in the rest of the shader. | ||
// This is for drivers that have a performance drop | ||
// when the output is read during the shader. | ||
gl_FragColor = frag_color; |
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.
Moved this because it turns out RENDER_DEPTH
is actually depth only pass, and we don't want to write to color in depth only pass.
(TURNS OUT - unless we are using RGBA shadows 馃榾 ).
// Write to the final output once and only once. | ||
// Use a temporary in the rest of the shader. | ||
// This is for drivers that have a performance drop | ||
// when the output is read during the shader. | ||
frag_color_final = frag_color; | ||
|
||
#endif //USE_MULTIPLE_RENDER_TARGETS //ubershader-runtime |
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.
This comment may be incorrect, should be // not USE_MULTIPLE_RENDER_TARGETS
...
// Write to the final output once and only once. | ||
// Use a temporary in the rest of the shader. | ||
// This is for drivers that have a performance drop | ||
// when the output is read during the shader. | ||
frag_color_final = frag_color; | ||
|
||
#endif //USE_MULTIPLE_RENDER_TARGETS //ubershader-runtime | ||
|
||
#endif //RENDER_DEPTH //ubershader-runtime |
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.
This comment seems incorrect, should be // not RENDER_DEPTH_ONLY
.
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.
This looks good. The problem comes from here where we define diffuse_buffer to be frag_color:
godot/drivers/gles3/shaders/scene.glsl
Line 927 in bb342cb
#define diffuse_buffer frag_color |
So the main output of the shader ultimately writes to frag_color (our intermediate color value). Accordingly, we need to assign it to frag_color_final outside the branch
7824f4d
to
ecd0680
Compare
Fixed one last tweak neither of us noticed - the GLES2 change would have broken RGBA shadows because it never wrote to Have fixed this. |
Thanks! |
This will need careful checking by @clayjohn as I'm not super familiar with all the parts of the scene shaders.
Fixes #91967
Discussion
In retrospect, I'm not totally surprised I got these in slightly the wrong place, the
#ifdef
arrangements especially in the scene shaders are very difficult to follow (manually). I would welcome suggestions for glsl editor that will follow them sensibly, geany can identify from opening, but not from closing. Other than that I'm relying onfind_in_files
.What makes things particularly difficult is that the comments in the scene shaders appear to be out of sync:
presumably should be
not RENDER_DEPTH
.This appears to have happened several times in the shaders because files originally contained e.g.
then an
#else
was inserted, and the ending comment became incorrect.I suspect these shaders could do with a second pass PR correcting these comments (I haven't done in the same PR as it seems two different problems).
Additionally, I worked out that
RENDER_DEPTH
seems actually for the z pre-pass and shadow maps. If so perhaps a better define would beRENDER_DEPTH_ONLY
, because depth is also rendered during normal passes, so the define is confusing.Also,
//ubershader-runtime
seems to be put on every closing#endif
, and I have no idea why, perhaps I can figure out from the ubershader PR. 馃榿