-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
Forbid passing multiview sampler to the custom function in shaders #72300
Forbid passing multiview sampler to the custom function in shaders #72300
Conversation
My feeling is that this might be necessary until we figure out a better solution. It would be really nice to automatically detect XR and non-XR usage, but I have a feeling that it will be too complex for now |
This does feel like a very ugly hack around the problem. One thing we could do is to check Also could we change the check that if we detect a function being called using a parameter we've deemed needing multiview support, that we tell the user to create a sampler2Darray version of the function being called, if it doesn't exist? |
Keep in mind that in XR we're always dealing with a mixed environment. Plus someone adding a function like the OP did, that function might be used for both normal textures and for textures that require multiview access in the same shader. |
It's not a hack - I would call it a plug until we found a way to handle it properly. |
Just to repeat this here as it should be part of this fix, as i mentioned in the issue, we can make this smarter so that our restriction only applies if multiview is enabled. This requires a few small changes:
With these changes our restriction will only be needed, and only apply, if we're compiling multiview shaders. |
@clayjohn @BastiaanOlij You seem to have agreed in the issue that this is a good short-term solution (on top of Clay's own old PR, which has since been merged). What changes, if any, are expected from @Chaosus now? Or should we bump it? |
@Chaosus I agree with Bastiaan's proposed improvements to this PR. Together they will make this limitation only impact users who are working on XR. For reference, these are the two changes needed:
|
Sounds like clay covered it. If we can make those small changes, this should be ready to go. |
20f455e
to
fc0a822
Compare
@BastiaanOlij, @clayjohn OK, I think I've done it, please recheck. |
Shader changes look good. Just a little nitpick on the singleton change, though this is my personal opinion on the subject, I don't know what the official stance on this is but I've always added separate singleton members to subclasses to ensure we're accessing a pointer that will remain null when a different subclass is instantiated. |
fc0a822
to
10b5c87
Compare
@BastiaanOlij OK, I made some changes, check again. |
10b5c87
to
e7d6db6
Compare
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.
Seeing get_singleton
is now on the base class we could replace RendererCompositorRD::get_singleton()
and RendererCompositorGLES3::get_singleton()
with RendererCompositor::get_singleton()
. But I feel I'm really taking nitpicking to the next level now :)
32678d7
to
c7558ef
Compare
c7558ef
to
94831c7
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Weird, I thought this PR would only have impact on the 3D renderer, not on the 2D renderer, but I could be wrong |
I'll try to do more testing, it might have been an unrelated bug present in |
Yeah my segfault was unrelated to this PR, sorry for the false alarm. It happens in the So this should be good to go. But I already started the build for 4.0 RC 3, so please don't merge for now, we'll see in the production team how to time this. |
Thanks! |
My attempt to fix #72290, if @BastiaanOlij has another idea of how to fix it that would be cool, and I will close this PR.