Decoder args#3007
Conversation
jzulauf-lunarg
left a comment
jzulauf-lunarg
left a comment
There was a problem hiding this comment.
Looking really interesting. A couple thoughts. Doesn't handle the stack relative reference case patterns, but those only exist in D3D and OpenXR...
| format::HandleId descriptorSet, | ||
| format::HandleId descriptorUpdateTemplate, | ||
| DescriptorUpdateTemplateDecoder* pData) | ||
| virtual void Process_vkUpdateDescriptorSetWithTemplate(const ApiCallInfo& call_info, |
There was a problem hiding this comment.
Here and all following,
-
The naming is redundant. Process(ApiCallInfo, UpdateDescriptorSetWithTemplateArgs) is unambiguous, though one could argue, less readable.
-
The *Args seem to be insufficiently scoped in framework::decode, I'd be tempted to put them into a namespace.
There was a problem hiding this comment.
How about args::UpdateDescriptorSetWithTemplate?
| StructPointerDecoder<Decoded_VkRayTracingPipelineCreateInfoKHR>* pCreateInfos, | ||
| StructPointerDecoder<Decoded_VkAllocationCallbacks>* pAllocator, | ||
| HandlePointerDecoder<VkPipeline>* pPipelines) | ||
| virtual void Process_vkCreateRayTracingPipelinesKHR(const ApiCallInfo& call_info, |
There was a problem hiding this comment.
Interface design question: Why not include the return value in the arg pack ... it's the expected return, correct?
There was a problem hiding this comment.
My rationale is that a return value is not an argument, but there is no real benefit with keeping it separate, on the contrary it is convenient to include it imo.
| UpdateDescriptorSetWithTemplateArgs& args) | ||
| { | ||
| Generate_vkUpdateDescriptorSetWithTemplate(device, descriptorSet, descriptorUpdateTemplate, pData); | ||
| Generate_vkUpdateDescriptorSetWithTemplate(args.device, args.descriptorSet, args.descriptorUpdateTemplate, &args.pData); |
There was a problem hiding this comment.
Take a look at how the DispatchVistor::DispatchArgs used GetTuple and std::apply to avoid the manual unpacking.
As the *Args are code gen'd adding a tuple accessor(s) would be straight forward.
There was a problem hiding this comment.
I opted for passing args directly.
|
|
||
| for param in params: | ||
| decoded_type = self.make_decoded_param_type(param) | ||
| body += f" {decoded_type} {param.name};\n" |
There was a problem hiding this comment.
This is where the GetTuple -ish generation would go... or even a method to call the appropriate Processes_(consumer, ...)
There was a problem hiding this comment.
I generated it, but its signature is slightly different than what functions exect.
While GetTuple returns references to the fields of the args structure, most functions expect pointers, rather than references.
|
@antonio-lunarg I didn't see the updated generate source associated with python you pushed for XR... |
I am only focusing on Vulkan to limit the scope of this PR. |
antonio-lunarg
force-pushed
the
antonio-decoder-args
branch
4 times, most recently
from
f9e7fcb to
b5ee615
Compare
antonio-lunarg
added
the
approved-to-run-ci
antonio-lunarg
force-pushed
the
antonio-decoder-args
branch
from
b5ee615 to
57fe18e
Compare
2 participants
No description provided.