Direct3D 12: Fix shader model support check for devices not aware of the highest ones #89038
Conversation
There was a problem hiding this comment.
Looks fine to me. I'll let @DarioSamo give the final approval
There was a problem hiding this comment.
Looks fine to me, but I think we should be explicit about what happens if the loop fails on every iteration. It is possible for D3D12 to support only SM 5.1 under feature level 11.
If we don't intend to support that, perhaps it should explicitly fail if none of the options it checked could be retrieved, as right now it'll continue as if it succeeded (e.g. when every iteration returns invalid arg).
|
Done. |
|
|
||
| auto _d3d_sm_to_str = [](D3D_SHADER_MODEL p_sm) { | ||
| return vformat("%d.%d", (p_sm >> 4), (p_sm & 0xf)); | ||
| }; |
There was a problem hiding this comment.
I'd use a macro instead of a lambda for this
There was a problem hiding this comment.
If this isn't within the reasonable usage of lambdas, I don't know what is. I've changed it, but I'm curious about what's your rationale here.
There was a problem hiding this comment.
It doesn't really benefit from being a method I'd say, it doesn't do anything a macro can't do in this case
For this case a macro is perfectly suited as it's just reusing a code snippet, so more a matter of using the right tool for the job rather than not using a lambda specifically, I'd argue the same with not using a static method or a class method
More generally one of my main issues with lambdas is that they hurt navigability of code, by introducing a scope-local method declaration, if you just find in the middle of a method some invocation of a method and you might want to look at what it does under the hood then finding it as a conventional method is relatively easy (assuming you know the codebase enough to recognize more well established methods, but that's unavoidable as a cause of confusion) you just look for a method declaration, that's easy to scan for, but with a lambda it can be almost anywhere in the local scope as long as it's before the current line, to me that ambiguity is a drawback of it
There was a problem hiding this comment.
I guess in the future we'll be able to come up with a ruleset that lets us take advantage of lambdas in a reasonable manner, without the current uncertainty about what the review outcome will be. I believe lambdas can make the codebase better if wisely used. The ability to tell that something is a lambda by its name seems indeed like an interesting part of that.
There was a problem hiding this comment.
Yeah I was thinking of a naming standard like prefixing with l_ or f_, that would help with that part
|
Thanks! |
Following the advice in the note at https://learn.microsoft.com/en-us/windows/win32/api/d3d12/ns-d3d12-d3d12_feature_data_shader_model.
Without this, e.g., a device that is only aware of up to Shader Model 6.4, will fail the check for 6.6, which is the only one we were doing. With this, a lower SM is checked each time that happens until it can make sense of the request and therefore tell what's the maximum it supports.