-
Notifications
You must be signed in to change notification settings - Fork 780
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
[d3d9] Fix a bunch of Wine test failures #3895
base: master
Are you sure you want to change the base?
Conversation
hi which apps are affected with this? |
I don't know if any apps are affected. I want to make DXVK play nice with the Wine tests. |
cd2b68b
to
90eb194
Compare
229 failures left in the visual category. |
We now pass all state block tests. |
8fc6d83
to
ae512ba
Compare
I think I'll try to land this first batch before looking into more of the tests. |
rs[D3DRS_POINTSPRITEENABLE] = FALSE; | ||
rs[D3DRS_POINTSCALEENABLE] = FALSE; | ||
rs[D3DRS_POINTSCALE_A] = bit::cast<DWORD>(1.0f); | ||
rs[D3DRS_POINTSCALE_B] = bit::cast<DWORD>(0.0f); | ||
rs[D3DRS_POINTSCALE_C] = bit::cast<DWORD>(0.0f); | ||
rs[D3DRS_POINTSIZE] = bit::cast<DWORD>(1.0f); | ||
rs[D3DRS_POINTSIZE_MIN] = bit::cast<DWORD>(1.0f); | ||
rs[D3DRS_POINTSIZE_MAX] = bit::cast<DWORD>(64.0f); | ||
rs[D3DRS_POINTSIZE_MAX] = bit::cast<DWORD>(limits.pointSizeRange[1]); |
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.
"The default render state value has to match what we report in the device caps."
And does that match what any DX9 implementation exposes? We should check that.
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.
MaxPointSize is 8192 in the D3D8 dump that Winter did. (With a NVIDIA GeForce4 Ti 4800 SE).
The Vulkan max point size range is generally much lower unless you're using an AMD GPU.
https://vulkan.gpuinfo.org/displaydevicelimit.php?name=pointSizeRange[1]&platform=linux
So considering that setting the max to 64.0 hasn't caused any issue so far, this should be fine?
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 what it's worth that still is the case for Nvidia, namely MaxPointSize is 8192 even with modern drivers/cards. AMD sets it somewhat lower, e.g. 256 on @Blisto91's 7900 XTX.
8ac8246
to
6a1cd59
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.
Couple questions.
In general I'd prefer to have unrelated fixes to be PR'd separately though, even if it means a fair bit of PR spam. Makes it easier to review and most of the commits in here would be good to go as-is anyway.
@@ -531,7 +533,7 @@ namespace dxvk { | |||
// Max Vertex Blend Matrix Index | |||
pCaps->MaxVertexBlendMatrixIndex = 0; | |||
// Max Point Size | |||
pCaps->MaxPointSize = 256.0f; | |||
pCaps->MaxPointSize = limits.pointSizeRange[1]; |
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.
Are games going to be fine with this not being an integer value / power-of-two? Vulkan drivers will report the upper limit of their fixed-point representation here, which is generally something of the format (2^n - 1)/(2^m)
.
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.
pCaps->MaxPointSize is a float, so you'd hope that games are fine if that's not an integer value. At least I don't have anything indicating they wouldn't be. We could ofc round it down to the nearest integer or power of two but right now I don't see any reason to do that so I'd prefer to wait until we hit a game that actually needs it.
Before we also reported a value that was potentially larger than what the Vulkan driver supports and didn't encounter any problem with that in a game either, so I don't think games actually check this value.
@@ -205,6 +205,10 @@ namespace dxvk { | |||
Vector4 dot0 = { m[0] * row0 }; | |||
float dot1 = (dot0.x + dot0.y) + (dot0.z + dot0.w); | |||
|
|||
if (unlikely(std::abs(dot1) <= 0.000001f)) { |
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.
Which part of the API relies on this and do we have a way to verify native behaviour?
Not the biggest fan of the design here since this is a utility function and we're not giving feedback to the caller.
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's specifically about calculating the normal matrix from the world matrix.
Wine tests what D3D9 does with a singular matrix here.
Would you prefer it if I check the determinant separately even if that means calculating it twice?
bool srcIsSurface = srcTextureInfo->GetType() == D3DRTYPE_SURFACE; | ||
bool dstIsSurface = dstTextureInfo->GetType() == D3DRTYPE_SURFACE; | ||
bool srcHasRTUsage = (srcTextureInfo->Desc()->Usage & (D3DUSAGE_RENDERTARGET | D3DUSAGE_DEPTHSTENCIL)) != 0; | ||
if (unlikely(!dstHasRTUsage && (!dstIsSurface || !srcIsSurface || srcHasRTUsage))) |
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.
What exactly does the D3DRTYPE_SURFACE
requirement accomplish here? What about textures with only one mip?
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 implementing the no stretching table:
Checking surface && !usage_rt && !usage_ds is the easiest way to check for offscreen plain surfaces.
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 also added a comment explaining it.
Also fixes a Wine test.
Fixes the diffuse alpha and the direction.
Fixes the Wine test "test_specular_lighting".
Fixes a Wine test and matches further testing on Windows.
The default render state value has to match what we report in the device caps. Fixes a Wine stateblock test.
Fixes the following Wine test failures:
StretchRect, lighting, specular_lighting
Actually matches the documentation here: https://learn.microsoft.com/en-us/windows/win32/api/d3d9/nf-d3d9-idirect3ddevice9-stretchrect