-
Notifications
You must be signed in to change notification settings - Fork 2.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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
D3D11 resources refactor and DX11.1 feature detection fixes #8258
D3D11 resources refactor and DX11.1 feature detection fixes #8258
Conversation
I don't think using D3D11 (minus .1) will change the black box behavior - that is a result of missing logic op support. The only reason it "worked" prior to the 11.1 change was because of the "emulation" of logic op using blend modes, which was only correct for a few of the operators. I don't see any point in re-introducing such a hack for a 10-year old OS which even MS will no longer be supporting as of next year. However, if we're crashing there, then fixing that would be nice. I don't have any Win7 boxes anymore, so I've never tested it ;) If assuming D3D11.1 implies logic op support is incorrect, you'd probably want to use Although, that page says |
I would drop "logic operations in blend state", nobody is going to know what that means, or care. I would either drop that part or simplify it to "The Direct3D 11 renderer requires features that your system configuration blah blah blah" |
I don't intend to re-introduce a hack. Code relying on DX11.1 is currently executed conditionally, but it in fact requires a stronger guarantee, that is it requires DX11.1 with logic op. I only intend to fix those checks, not introduce hacky code paths. I don't care what happens with this skipped, as long as it does not crash. However, incidentally, skipping DX11.1 code chunk "fixes" this issue - that's how the game looks when I don't instantiate a DX11.1 device (which is effectively a single line change):
That's exactly how I'll do it - I already ran tests and it gives expected results.
True, I guess the only page which got updated is this, which clearly states Win7 with a platform update receives partial support (no logic ops for Win7 guys): Later I'll also uninstall a platform update on Win7 to see if Dolphin still crashes without it (as mentioned in the issue I linked initially). I don't think anyone should support this per se, but we can all agree that it should not crash, but instead try to show the user a descriptive message, just like when d3dcompiler_47.dll is not present. |
7037363
to
bdbeaae
Compare
Ownership fixes I pushed now streamline |
e9d7802
to
798ecb5
Compare
What do you think about dropping the "temporary device" completely, and making GetAAModes() just use the global device? This would also disable things like bounding box for older hardware. I'm thinking something like: void VideoBackend::FillBackendInfo()
{
// Are we booting a game or just filling the info for the options dialog?
const bool booting_game = static_cast<bool>(D3D::device);
if (!booting_game)
{
// Create a temporary device used to populate optional backend features.
if (!D3D::Create(g_Config.iAdapter, false))
WARN_LOG(VIDEO, "Failed to create temporary D3D device");
}
// [...]
if (D3D::device)
{
g_Config.backend_info.AAModes = D3D::GetAAModes();
g_Config.backend_info.bSupportsLogicOp = D3D::LogicOpSupported();
g_Config.backend_info.bSupportsST3CTextures = ...
// [...]
}
if (!booting_game)
{
// Clean up the temporary device used for populating features.
D3D::Close();
}
} |
I would love to agree with that, but there is a problem with this when polling for logic op support (so I know whether or not to show the warning message). What I would do instead is ensure This will also potentially rquire changes to |
I don't see why Returning the features from It's kinda highlighting another flaw though - if D3D12 was selected and a user switched to D3D11, the adapter index may not line up, and it'd end up querying the incorrect device. For this, I think displaying the warning message as a text box in the configuration dialog would be better than a message box. A text box would be invariant to system/hardware changes too. e.g. if you switched GPUs, and the new GPU didn't support Vulkan, or was missing logic op support, the warning message could indicate that without having to switch to and from another backend. Edit: For this, I'm thinking something like returning the struct from FillBackendInfo(), and constructing the warning message entirely in the frontend. After all, the logic op issue applies to OpenGL ES as well ;) |
Those start to sound pretty invasive, do you think it should be part of this PR? I think at this point it should be split, to ease review and potentially get it merged quicker (as @JMC47 wants it in ASAP). Then those could be addressed as another, lower priority PR in the future. EDIT: |
65b927f
to
46075a8
Compare
…o be used with WRL ComPtr
…rom a new GetWarningMessage in video backend - will be needed for DX11.1 feature set warnings
46075a8
to
ed79d74
Compare
… only if supported Previously code assumed that if DX11.1 runtime is supported, logic ops will, but Windows 7 SP1 with a Platform Update supports DX11.1 runtime without logic ops. This created pretty jarring visual artifacts, which now should be gone OR replaced with much less jarring errors.
…ng to D3D11 backend on Windows 7
ed79d74
to
baa9636
Compare
This PR reworks feature set detection in Direct3D 11 backend. Currently false assumptions seem to be present - namely logic op support is assumed to be there when DX11.1 is present. This is not true, and it leads to graphical artifacts on Windows 7 in eg. in Super Mario Wii.
Effectively implements:
https://bugs.dolphin-emu.org/issues/10482
The plan for DX11 on Windows 7 is to allow the user to continue using it (with forementioned glitch fixed), but let the user know that artifacts may occur. Warning currently looks like this:
With this implemented, Mario Kart Wii looks significantly more playable on Windows 7. It probably has glitches elsewhere, but since logic op is not hacked/emulated it's bound to happen.
Before:
https://imgur.com/VBEcorP
After:
https://imgur.com/jvzuCdn