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

[spirv] Convert to use 2 as the depth value for OpTypeImage #1261

Merged
merged 1 commit into from
May 11, 2018

Conversation

antiagainst
Copy link
Contributor

2 means no indication as to whether this is a depth or non-depth
image.

Reverted "Hack OpSampledImage for depth-comparison sampling".

This reverts commit 0f165c6.

2 means no indication as to whether this is a depth or non-depth
image.

Reverted "Hack OpSampledImage for depth-comparison sampling".

This reverts commit 0f165c6.
@AppVeyorBot
Copy link

Copy link
Contributor

@ehsannas ehsannas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but you may want to hold off merging until we're sure things render properly using new drivers.

@antiagainst
Copy link
Contributor Author

Yep

@kennyalive
Copy link

Works fine with NVIDIA drivers 397.31 on GTX 1060. Also the workaround that was reverted produced visual artifacts (shadows were broken in some scenarios). I'm going to check how it works with AMD hardware this week and also for NVIDIA with hotfix drivers released after 397.31. Thank you for your work.

@oscarbg
Copy link

oscarbg commented May 9, 2018

@kennyalive perhaps is better to test with nvidia vulkan dev drivers as that is will be integrated in future branches like 399 or 400 series.. now latest is 397.54..
AMD are you referring on new branch (18.10) known as 17.4.1 right?
also for completeness will you be able to test on latest Intel Windows Vulkan 1.1 driver (24.20.100.6025 WHQL)?

@tafuri
Copy link
Contributor

tafuri commented May 9, 2018

There are now tests for this issue in the CTS KhronosGroup/VK-GL-CTS#88 also. Which should accelerate conformance.

@kennyalive
Copy link

kennyalive commented May 9, 2018

@oscarbg Tested today with 397.54. It seems shadows works as expected. Probably won't be able to check intel compatibility in the near future. Not sure about AMD drivers version, I'm going to do some tests on Friday and I'll provide an update.

@kennyalive
Copy link

The shadows also present on AMD Vega 64. Currently I have public 18.3.4 drivers installed with updated amdvlk64.dll vulkan driver, we use dev version released on 08.05.2018.

@antiagainst
Copy link
Contributor Author

Cool! Thanks for your diligence, @kennyalive! I'll merge this then.

@antiagainst antiagainst merged commit babfc75 into microsoft:master May 11, 2018
@antiagainst antiagainst deleted the depth-cmp-sample branch May 11, 2018 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants