Skip to content

some thoughts #2

Description

@archo5

index buffers

const VkBindIndexBuffer3InfoKHR bind_info{
.sType = VK_STRUCTURE_TYPE_BIND_INDEX_BUFFER_3_INFO_KHR,
.addressRange = {
.address = range.address,
.size = range.size,
},
.addressFlags = address_flags,
.indexType = type == IndexType::uint16 ? VK_INDEX_TYPE_UINT16 : VK_INDEX_TYPE_UINT32,
};
commands->state->fn.cmd_bind_index_buffer(commands->command_buffer, &bind_info);

even assuming this were to become a driver one day (as opposed to a vulkan wrapper), would it not:

  • be still inefficient to revalidate the index buffer on every draw call (debug build perf matters)
  • have many copies of the index buffer reference in the command buffer (when rendering many instances without instancing, e.g. with different materials)
  • get in the way of rendering many different meshes with the same index buffer if one wanted to do that (or even just a mesh with many parts)

dynamic state

vkCmdSetViewport(commands->command_buffer, 0, 1, &viewport);
vkCmdSetScissor(commands->command_buffer, 0, 1, &scissor);

there appears to be no way to set custom viewports/scissor rectangles?

const VkPipelineRasterizationStateCreateInfo rasterization{
.sType = VK_STRUCTURE_TYPE_PIPELINE_RASTERIZATION_STATE_CREATE_INFO,
.polygonMode = VK_POLYGON_MODE_FILL,
.cullMode = to_vk(rasterization_state.cull),
.frontFace = rasterization_state.cull == CullMode::counter_clockwise ? VK_FRONT_FACE_CLOCKWISE : VK_FRONT_FACE_COUNTER_CLOCKWISE,
.depthBiasEnable = rasterization_state.depth_bias_constant != 0.0f ||
rasterization_state.depth_bias_clamp != 0.0f ||
rasterization_state.depth_bias_slope != 0.0f,
.depthBiasConstantFactor = rasterization_state.depth_bias_constant,
.depthBiasClamp = rasterization_state.depth_bias_clamp,
.depthBiasSlopeFactor = rasterization_state.depth_bias_slope,
.lineWidth = 1.0f,
};

also no way to set rasterizer state at runtime?

PSOs

const VkResult pso_result = vkCreateGraphicsPipelines(device->device, VK_NULL_HANDLE, 1, &pso_info, nullptr, &result->pso);

thoughts on using VK_EXT_shader_object instead?

delete queue

found this in the implementation:

detail::DeleteQueue delete_queue;

given that the implementation seems to need it anyway, why not expose the same queue to the user?

GpuCpuRange size

uint64_t size = 0; // Bytes, independent of T.

a very minor point but why not include in the name that it's the size in bytes (e.g. size_bytes), so it's clearly visible anywhere it's used?
also makes me wonder what the other sizes mean
for some you can infer the meaning from the definition but for others (e.g. GpuRange) it's not possible, need to look at uses instead

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions