Make ExecutionTests able to pick a specific AgilitySDK #7963
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
TAEF will fail the test if LogError is used.
hekota
left a comment
hekota
left a comment
There was a problem hiding this comment.
Looks good! Just a few suggestions.
…e where it doesn't create anything.
Co-authored-by: Helena Kotas <[email protected]>
hekota
left a comment
hekota
left a comment
There was a problem hiding this comment.
LGTM! The PR description should be updated with the new name D3D12SDKSelector. :)
alsepkow
left a comment
alsepkow
left a comment
There was a problem hiding this comment.
LGTM. A few minor nits.
| /// | ||
| /// 1: auto-detect (fail if unable to use the auto-detected version) | ||
| /// | ||
| /// >1: use specified version |
There was a problem hiding this comment.
| /// >1: use specified version | |
| /// 1: use specified version |
There was a problem hiding this comment.
This is intended to say "values greater than 1 mean use the specified version". Does that change the suggestion?
| } | ||
|
|
||
| std::vector<UUID> Features; | ||
| static bool isExperimentalShadersEnabled() { |
There was a problem hiding this comment.
nit: Name sounds weird.
'areExperimentalShadersEnabled' or 'experimentalShadersEnabled' ?
There was a problem hiding this comment.
Yeah, I actually thought hard about this one and although it sounds weird I decided "is" is correct, since the thing we're enabling is the "experimental shaders" feature (and there's only one feature being enabled here).
| DebugController->EnableDebugLayer(); | ||
| HR = S_OK; | ||
| D3D12SDKSelector::~D3D12SDKSelector() { | ||
| if (DeviceFactory) { |
There was a problem hiding this comment.
Why do we need an explicit Release call?
There was a problem hiding this comment.
This code is a bit odd because of the bug we're working around. The requirement before calling FreeUnusedSDKs is that we've released all of the objects that were created from the SDK we're going to release - and the last one of those we have left at this point is the DeviceFactory, hence the explicit release.
(PR feedback)
The original AgilitySDK API only allowed paths relative to the process's executable to be used to specificy the location of the AgilitySDK. So, previously, if we wanted to use a specific AgilitySDK with the execution tests we had to arrange for the AgilitySDK's dlls to be placed in a subdirectory underneath TAEF's te.exe. This required a convoluted dance involving finding and copying TAEF executables somewhere that could be owned by the testing process.
This change makes it so we can use ID3D12DeviceFactory - which allows the AgilitySDK location to be specified using absolute paths.
A new class, D3D12SDKSelector, is added to help manage this, as well as falling back to using the old-style AgilitySDK and the inbox D3D12.