Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Implement the GetRenderTargetSampleCount() intrinsic.

Merged Elizabeth Figura requested to merge zfigura/vkd3d:mr2 into master
2 unresolved threads

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
606 607 interface_info.combined_samplers = runner->combined_samplers;
607 608 interface_info.combined_sampler_count = runner->combined_sampler_count;
608 609
610 parameters[0].name = VKD3D_SHADER_PARAMETER_NAME_RASTERIZER_SAMPLE_COUNT;
611 parameters[0].type = VKD3D_SHADER_PARAMETER_TYPE_IMMEDIATE_CONSTANT;
612 parameters[0].data_type = VKD3D_SHADER_PARAMETER_DATA_TYPE_UINT32;
613 parameters[0].u.immediate_constant.u.u32 = runner->r.sample_count;
614
615 spirv_info.parameter_count = ARRAY_SIZE(parameters);
616 spirv_info.parameters = parameters;
  • Comment on lines +615 to +616

    I'm not really very confident with OpenGL, but shader_runner_gl.c has at some point a comment saying /* TODO: sample count and mask. */. I guess the sample count part can be deleted now, or is there something else to do?

  • Well, we don't set the sample count for resources, at least.

    I'm... not sure why I included this bit in the patch but nothing else. I'll try to add the rest.

  • Please register or sign in to reply
  • 103 110 uint8_t *data;
    104 111 size_t data_size, data_capacity;
    105 112 };
    106 113
    107 114 struct resource
    108 115 {
    109 unsigned int slot;
    110 enum resource_type type;
    111 enum resource_dimension dimension;
    112
    113 DXGI_FORMAT format;
    114 unsigned int texel_size;
    115 unsigned int width, height;
    116 unsigned int sample_count;
    116 struct resource_desc desc;
    117 117 };
    • Comment on lines -87 to 117

      I can live with this I guess, but the way these structures are laid out doesn't feel completely natural to me.

      • It's odd that the only field of struct resource is the resource description. In general it's a bit odd when structures have just one field, and in particular here you seem to be identifying a resource (which is an entity: two resources with the same descriptions are not the same resource) with its description (which is a collection of values: two identical descriptions are supposed to be completely interchangeable).
      • Conversely, field slot in struct resource_desc doesn't seem to belong to a description, at least according to the meaning that I give to this word (which seems to be similar to what Microsoft does).
      • For those two reasons, it seems natural to me that slot is moved from struct resource_desc to struct resource and struct resource_params. I can see why you might want to collapse the two things, but conceptually they're different for me.

      Anyway, that's just theoretical ramblings, and I don't want to block anything here. Feel free to ignore.

      • It's odd that the only field of struct resource is the resource description. In general it's a bit odd when structures have just one field, and in particular here you seem to be identifying a resource (which is an entity: two resources with the same descriptions are not the same resource) with its description (which is a collection of values: two identical descriptions are supposed to be completely interchangeable).

      It's awkward, but it felt more awkward to use the same struct for both, naming-wise.

      We really are passing a "resource" around as evidenced by e.g. struct vulkan_resource, it's just that the only common fields are the desc, and inheriting struct vulkan_resource from struct resource_desc looks wrong.

      On the other hand, constructing a resource from a "struct resource" (or putting that in struct resource_params) also looks wrong.

      • Conversely, field slot in struct resource_desc doesn't seem to belong to a description, at least according to the meaning that I give to this word (which seems to be similar to what Microsoft does).
      • For those two reasons, it seems natural to me that slot is moved from struct resource_desc to struct resource and struct resource_params. I can see why you might want to collapse the two things, but conceptually they're different for me.

      It doesn't match the way most APIs are designed, but our "API" is a little simpler—creating a resource in the first place implies choosing a "slot" for it. I don't see a conceptual problem with keeping it in the desc. On the other hand I don't have a problem with keeping it duplicated either.

    • Please register or sign in to reply
  • Is this better than reusing HLSL_RESOURCE_SAMPLE_INFO ?

  • Is this better than reusing HLSL_RESOURCE_SAMPLE_INFO ?

    I would assert it's no better or worse. Note though that it doesn't take a resource as an argument, which would be unique for a HLSL_IR_RESOURCE_LOAD.

  • added 4 commits

    • c04a800d - tests: Implement multisampling in the Vulkan renderer.
    • 8d36e58d - tests: Implement multisampling in the GL renderer.
    • dace261d - vkd3d-shader/hlsl: Implement the GetRenderTargetSampleCount() intrinsic.
    • cad6dfa9 - vkd3d-shader/hlsl: Implement output SV_Coverage.

    Compare with previous version

  • I split the Vulkan and GL multisample implementations, actually finished the GL side, and implemented output SV_Coverage to validate that part.

  • added 1 commit

    • adf4282f - vkd3d-shader/hlsl: Implement output SV_Coverage.

    Compare with previous version

  • added 6 commits

    • 19e7117c - tests/shader_runner: Use resource->width instead of resource->size for buffer width.
    • dcc1a963 - tests/shader_runner: Factor out a resource_desc structure.
    • 6b409dff - tests: Implement multisampling in the Vulkan renderer.
    • 301e327d - tests: Implement multisampling in the GL renderer.
    • 0d709f9d - vkd3d-shader/hlsl: Implement the GetRenderTargetSampleCount() intrinsic.
    • 01947dca - vkd3d-shader/hlsl: Implement output SV_Coverage.

    Compare with previous version

  • Giovanni Mascellani approved this merge request

    approved this merge request

  • Henri Verbeet approved this merge request

    approved this merge request

  • Henri Verbeet added 69 commits

    added 69 commits

    • 01947dca...76ae871f - 63 commits from branch wine:master
    • fa570ee1 - tests/shader_runner: Use resource->width instead of resource->size for buffer width.
    • fe21318b - tests/shader_runner: Factor out a resource_desc structure.
    • 73796760 - tests: Implement multisampling in the Vulkan renderer.
    • a5a75d45 - tests: Implement multisampling in the GL renderer.
    • 71a3d55e - vkd3d-shader/hlsl: Implement the GetRenderTargetSampleCount() intrinsic.
    • 59f77021 - vkd3d-shader/hlsl: Implement output SV_Coverage.

    Compare with previous version

  • Please register or sign in to reply
    Loading