Conversation
|
The alpha-to-coverage now found a new home in the |
|
The whole lot of renames now follow in the tailing commits. Hopefully, we'll not need to mess with it for a while after that. |
kainino0x
left a comment
There was a problem hiding this comment.
Overall I like these new names.
| GPUDepthStencilStateDescriptor depthStencilState; | ||
| required GPUOutputMergerStateDescriptor outputMergerState; | ||
| GPUVertexInputState vertexInput = {}; | ||
| required GPUProgrammableStage vertexStage; |
There was a problem hiding this comment.
The wording above calls everything a "stage" and calls vertex/fragment specifically "shaders". Should this, too? (This could affect compute pipelines too.)
There was a problem hiding this comment.
it would be unfortunate to say GPUprogrammableShader...
Perhaps, what we have here is fine? Non-programmable stages have "state", but programmable stages have shaders.
Would you see required GPUProgrammableStage vertexShader as an improvement?
There was a problem hiding this comment.
I think it would be fine, but I also think "vertexStage" is fine.
(And the pipeline organization is great!) |
| GPURasterizerState rasterizer = {}; | ||
| GPUProgrammableStage fragmentStage; | ||
| GPUDepthStencilState depthStencil; | ||
| required GPUOutputMergerState outputMerger; |
There was a problem hiding this comment.
IMHO outputMerger is a bit weird, because looking at D3D12 (from which the terminology comes) OM is only used for OMSetStencilRef OMSetBlendColor OMSetDepthBounds and OMSetRenderTargets. The D3D12_GRAPHICS_PIPELINE_STATE_DESC has AlphaToCoverage in the BLEND_DESC.
The extra indirection with the outputMerge.colorOutputs is a bit big for something that's so useful. Maybe we could keep the colorOutputs directly in GPURenderPipelineDescriptor and put all the multisampling state together in a multisample sub-descriptor. It doesn't necessarily follow the ordering of the graphics pipeline, but puts all the information you care about for multisampling in the same place.
There was a problem hiding this comment.
I like the current revision (.colorWrite.outputs), but if we wanted to remove the indirection, we could put alphaToCoverageEnabled inside the GPUColorOutputState and allow it to be set only on index 0.
There was a problem hiding this comment.
Right, that's an interesting option. I'd prefer to not go this way, since we may want to add more fields in the write output structure, e.g. with extensions, and we don't want to be forced to put them elsewhere.
|
What about calling |
|
Resolution: Everyone seems happy with aspect based; @kvark to make a PR to propose it concretely. |
This updates the invariant duplicate test to have both positive and negative cases so we have a control that other syntax changes don't hide issues with this test.
Moved out of #1293 and reworked according to the latest input from the editors.
Closes #936
TL;DR:
GPUPrimitiveAssemblyStateDescriptor, which contains the primitive topology with the strip index format.and alpha-to-coverageinto the rasterization stateGPURenderPipelineDescriptoraccording to the order in the pipelineNote: we probably want to move the alpha-to-coverage after the fragment shader
Preview | Diff