Add default values to most dictionary fields - #241
Conversation
|
Looks like D3D12 also has some default values: https://docs.microsoft.com/en-us/windows/desktop/direct3d12/helper-structures-for-d3d12 |
grorg
left a comment
There was a problem hiding this comment.
Also saves us having to do some work in the API wrapper!
kainino0x
left a comment
There was a problem hiding this comment.
lgtm overall, with a bunch of comments
| GPUCompareFunction depthCompare = "always"; | ||
|
|
||
| GPUStencilStateFaceDescriptor stencilFront; | ||
| GPUStencilStateFaceDescriptor stencilBack; |
There was a problem hiding this comment.
If an app doesn't specify these, do they get the default values above? Or will we have to do that manually in implementations?
There was a problem hiding this comment.
I guess these should say = {}.
There was a problem hiding this comment.
Unfortunately doesn't seem to be valid idl, I tried it earlier. (https://w3c.github.io/webidl2.js/checker/)
kvark
left a comment
There was a problem hiding this comment.
I strongly believe that we shouldn't be over-opinionated here. It's still an explicit API that expects certain level of understanding from the user.
Defaults make sense for cases where "I don't want to think about it", "I don't know what it is", or "I don't care". A good example of this is depth bias options: defaults for them are perfectly fine because non-defaults mean something extra is done by the hardware. Same goes for other fixed-function blocks, such as depth or stencil (which is why the default depth/stencil comparison should be "always"). It can also be applied to texture parameters: until the user starts caring about the arrays, the number of layers can default to 1, and same for the number of mipmap levels. Interestingly, the logic can also be projected to the load/store operations: if the user doesn't care, the operations should preserve the contents by default.
Aside stand the parameters where the user has to select something, as opposed to a binary opt-in: texture format, front face, or things like the clear values for color/depth. I believe this is where we should be drawing a line: not provide defaults for these.
|
Discussed at 18 Mar 2019 teleconference |
|
(Sorry if this isn't ready yet.) Any chance we could get the last few contentious defaults split to another PR so we can land this asap? BTW, here is the parsing rule for default values - but it's fine to have invalid IDL for now. |
| sequence<GPURenderPassColorAttachmentDescriptor> colorAttachments; | ||
| GPURenderPassDepthStencilAttachmentDescriptor? depthStencilAttachment; | ||
| required sequence<GPURenderPassColorAttachmentDescriptor> colorAttachments; | ||
| GPURenderPassDepthStencilAttachmentDescriptor? depthStencilAttachment = null; |
There was a problem hiding this comment.
Our idl parser doesn't allow dictionary types to have a ?. (I think it wants GPURenderPassDepthStencilAttachmentDescriptor depthStencilAttachment = null maybe.) But webidl2.js does allow it, so I'm not sure if this is indicative of a problem.
|
Discussed at the 1 April Teleconference |
kainino0x
left a comment
There was a problem hiding this comment.
TODO:
- remove
GPUBufferUsage.ALL - require
GPUTextureDescriptor.format - require
GPURenderPassColorAttachmentDescriptor.{load,store}Op - require
GPURenderPassDepthStencilAttachmentDescriptor.{depth,stencil}{Load,Store}Op - require
GPURenderPassDepthStencilAttachmentDescriptor.clearDepth(@kvark will open a PR to refactor it) - Merge!
We will consider adding more defaults later.
|
|
||
| dictionary GPUInputStateDescriptor { | ||
| GPUIndexFormat indexFormat; | ||
| required GPUIndexFormat indexFormat; |
There was a problem hiding this comment.
Can this be optional (throw an error if drawIndexed is called and this is not set)? For the case where drawIndexed is never called with this pipeline.
There was a problem hiding this comment.
I think it's worth considering; we ran into this issue when we were writing demos too. Let's do it in another issue/PR after this one, though.
|
ping: can we get this updated/rebased and merged? |
|
Rebased. |
|
@kvark FYI, I think all of your comments are addressed. You can re-review when you're back if you want, but merging this now. |
Implements Validation Tests For StoreOp
Default values make authors' lives easier.