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 depthDescriptor.bindMS condition #5039
Conversation
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 whole line of code was added by the depth priming PR. If you look at the VR only #ifdef above, this is exactly the same code, and used to be VR specific code.
I think we should revert the bindMS setting to what it was before the depth priming PR, which seems to have introduced the issue, so just removing this line completely
Are there any issues with that?
| @@ -988,7 +988,7 @@ void CreateCameraRenderTarget(ScriptableRenderContext context, ref RenderTexture | |||
| depthDescriptor.bindMS = depthDescriptor.msaaSamples > 1 && !SystemInfo.supportsMultisampleAutoResolve && (SystemInfo.supportsMultisampledTextures != 0); | |||
| #endif | |||
|
|
|||
| depthDescriptor.bindMS |= depthDescriptor.msaaSamples > 1 && primedDepth && !SystemInfo.supportsMultisampleAutoResolve && (SystemInfo.supportsMultisampledTextures != 0); | |||
| depthDescriptor.bindMS |= depthDescriptor.msaaSamples > 1 && !SystemInfo.supportsMultisampleAutoResolve && (SystemInfo.supportsMultisampledTextures != 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.
this is the line of code to remove I mentioned in my comment (which I posted in the wrong place ops :) )
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.
for reference, this is the PR that added this line potentially causing the regression:
#3997
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.
LGTM, thanks for the changes!
|
@thomas-zeng Any update on this ? This has been causing certain platform tests to fail for a while. |
|
Sorry @NicoLeyman for dropping the ball. I have greenlighted the PR. |
… into urp/fix-depth-msaa-condition
|
Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed. URP Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure. |
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.
Approving without testing. The issue was triggered in Yamato, and it is fixed now.
|
Issues on Yamato were not caused by this PR. |
Purpose of this PR
Some Scarlet tests have started failing with the exception: "A non-multisampled texture being bound to a multisampled sampler. Disabling in order to avoid undefined behavior. Please enable the bindMS flag on the texture."
This change fixes the issue.. The BindMS flag was incorrectly disabled on some platforms due to the depthPriming setting.