Sept. 13, 2023
3:53 a.m.
On Wed Sep 13 08:53:39 2023 +0000, Henri Verbeet wrote:
> Just a couple of nits:
> > ```diff
> > +typedef enum D3D12_DRED_ALLOCATION_TYPE
> > +{
> > ...
> > + D3D12_DRED_ALLOCATION_TYPE_INVALID = 0xFFFFFFFF
> > +} D3D12_DRED_ALLOCATION_TYPE;
> > ```
> Could we make that lower case hexadecimal?
> > ```diff
> > + D3D12_RESOURCE_ALLOCATION_INFO GetResourceAllocationInfo1(
> > + UINT visible_mask,
> > + UINT num_resource_descs,
> > + const D3D12_RESOURCE_DESC *resource_descs,
> > + D3D12_RESOURCE_ALLOCATION_INFO1 *resource_allocation_info1);
> > ```
> We'd typically use "resource_desc_count" instead of "num_resource_descs"
> above. There are a couple of other "num_*" parameters in this MR as
> well. Ultimately I'm not too bothered about these, so don't worry about
> it if you feel changing them is too much effort, but if you did change
> them that would certainly be welcome.
> > ```diff
> > + void CreateSamplerFeedbackUnorderedAccessView(
> > + ID3D12Resource *targeted_resource,
> > + ID3D12Resource *feedback_resource,
> > + D3D12_CPU_DESCRIPTOR_HANDLE dest_descriptor);
> > ```
> Similarly, we'd typically use "dst_descriptor" above and for similar parameters.
> > ```diff
> > +interface ID3D12Device6 : ID3D12Device5
> > +{
> > + HRESULT SetBackgroundProcessingMode(
> > + D3D12_BACKGROUND_PROCESSING_MODE Mode,
> > + D3D12_MEASUREMENTS_ACTION MeasurementsAction,
> > + HANDLE hEventToSignalUponCompletion,
> > + BOOL*pbFurtherMeasurementsDesired);
> > +}
> > ```
> I'd suggest something like the following:
> ```
> interface ID3D12Device6 : ID3D12Device5
> {
> HRESULT SetBackgroundProcessingMode(
> D3D12_BACKGROUND_PROCESSING_MODE mode,
> D3D12_MEASUREMENTS_ACTION action,
> HANDLE event,
> BOOL *further_measurements_desired);
> }
> ```
> > ```diff
> > +cpp_quote("#define D3D12_ENCODE_BASIC_FILTER(min, mag, mip,
> reduction) \\")
> > +cpp_quote(" ((D3D12_FILTER) ( \\")
> > +cpp_quote(" (((min) & D3D12_FILTER_TYPE_MASK) <<
> D3D12_MIN_FILTER_SHIFT) | \\")
> > +cpp_quote(" (((mag) & D3D12_FILTER_TYPE_MASK) <<
> D3D12_MAG_FILTER_SHIFT) | \\")
> > +cpp_quote(" (((mip) & D3D12_FILTER_TYPE_MASK) <<
> D3D12_MIP_FILTER_SHIFT) | \\")
> > +cpp_quote(" (((reduction) & D3D12_FILTER_REDUCTION_TYPE_MASK) << D3D12_FILTER_REDUCTION_TYPE_SHIFT)))")
> > +
> > +cpp_quote("#define D3D12_ENCODE_ANISOTROPIC_FILTER(reduction) \\")
> > +cpp_quote(" ((D3D12_FILTER) ( \\")
> > +cpp_quote(" D3D12_ANISOTROPIC_FILTERING_BIT | \\")
> > +cpp_quote(" D3D12_ENCODE_BASIC_FILTER(D3D12_FILTER_TYPE_LINEAR, \\")
> > +cpp_quote(" D3D12_FILTER_TYPE_LINEAR, \\")
> > +cpp_quote(" D3D12_FILTER_TYPE_LINEAR, \\")
> > +cpp_quote(" reduction))) ")
> > ```
> We'd typically put the operators at the start of the next line; compare
> e.g. the existing D3D12_DECODE_IS_ANISOTROPIC_FILTER and
> D3D12_ENCODE_SHADER_4_COMPONENT_MAPPING macros.
Thanks; I believe I've addressed all of these comments now:
- I made the `0xFFFFFFFF` lowercase (and added a trailing comma)
- I changed all of the `num_foo` arguments into `foo_count`
- Changed parameters `dest_foo` into `dst_foo` and `source_foo` into `src_foo`
- Applied the suggested renamings on `ID3D12Device6`
- Changed the `D3D12_ENCODE_BASIC_FILTER` and `D3D12_ENCODE_ANISOTROPIC_FILTER` macros to use operators at the start of the line, and removed a case of trailing unnecessary whitespace there.
--
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/332#note_45174