Add FramebufferCreateInfoBuilder::attachment_count

[kvark]

Currently, it only has attachments, which require the actual slice to be passed in. However, in the image-less framebuffer scenario, only the count needs to be set. So the builder isn't compatible with this right now.

[Ralith]

I've just been passing &[vk::ImageView::null(); N].

[kvark]

This works if you know the size at run-time. Obviously, there are workarounds, but I think the builder pattern has this ability in other places to specify the count without providing a slice, and here it's just an omission.

[MarijnS95]

but I think the builder pattern has this ability in other places to specify the count without providing a slice, and here it's just an omission.

@kvark Are you referring to other places within Ash, and if so, have an example? As far as I'm aware vk.xml annotates all array fields with the right length field so these should be uncommon if not inexsistent. In that sense this is not an intentional omission either, as it is usually invalid to set the size to some non-zero value while still passing a NULL pointer.

This works if you know the size at run-time

I think you meant compile-time? For runtime you can use the somewhat ugly heap allocation through &vec![vk::ImageView::null(); runtime_value].

[nickkuk]

Spec says that pAttachments field is ignored for imageless framebuffer, but attachmentCount field must still be filled with non zero value in that case.

[MarijnS95]

@nickkuk Correct. "pAttachments... If flags includes VK_FRAMEBUFFER_CREATE_IMAGELESS_BIT, this parameter is ignored." is why the above workaround works.

[nickkuk]

Yes, and now to create imageless framebuffer I use builder pattern like this:

    let mut framebuffer_create_info = vk::FramebufferCreateInfo::builder()
      .flags(vk::FramebufferCreateFlags::IMAGELESS)
      .render_pass(render_pass.vk_render_pass)
      .width(extent.width)
      .height(extent.height)
      .layers(1)
      .push_next(&mut framebuffer_attachments_create_info)
      .build();
    framebuffer_create_info.attachment_count = attachment_configs.len() as u32;

With @kvark's suggestion it will be

    let framebuffer_create_info = vk::FramebufferCreateInfo::builder()
      .flags(vk::FramebufferCreateFlags::IMAGELESS)
      .render_pass(render_pass.vk_render_pass)
      .width(extent.width)
      .height(extent.height)
      .layers(1)
      .push_next(&mut framebuffer_attachments_create_info)
      .attachment_count(attachment_configs.len() as u32)
      .build();

without mutability and ugly heap allocation.

[MarijnS95]

@nickkuk The effect of this suggestion is well-understood. I'm just saying that this isn't an intentional omission to make life hard, because it is generally "unsafe" to manually change counts without passing an appropriate slice-pointer, that is why the builders do not have these functions. An exception can probably be made for this struct, I'd accept such a PR.

[MarijnS95]

General advice: After implementing the suggestion, please don't call .build() unless absolutely necessary. Passing it to vkCreateFramebuffer auto-derefs the builder, without loosing lifetime info on the pointer(s) contained within VkFramebufferCreateInfo (pNext).

[nickkuk]

Sorry, @MarijnS95, I misunderstood your question. Another place where I had to set some count manually was vk::WriteDescriptorSet's field descriptor_count to bind VkWriteDescriptorSetAccelerationStructureKHR (I use nvidia extension for now, don't know about whether such manual set is needed for KHR).

[nickkuk]

General advice: After implementing the suggestion, please don't call .build() unless absolutely necessary.

@MarijnS95, yes, in my first imageless framebuffer example I actually forget that DerefMut works for changing fields without .build() too (as you suggested for vkCreateFramebuffer).

[MarijnS95]

Indeed vk::WriteDescriptorSet is one of these other weird cases where descriptor_count is shared across 3 array-pointers (CC #445, that might actually break this) and the array in VkWriteDescriptorSetAccelerationStructureKHR in the pNext field.

[kvark]

I looked through the other usages of count, and indeed descriptor_count may be the only non-trivial one. So we can consider attachment_count somewhat unique. The suggestion to do &vec![vk::ImageView::null(); runtime_value] is very dangerous since it breaks if one calls build() in unexpected ways. Overall, ash is still a fully unsafe API, so providing attachment_count isn't going to be anything extraordinary. I suppose you just don't want to make an exception for this, which is a reasonable concern.

[MarijnS95]

The suggestion to do &vec![vk::ImageView::null(); runtime_value] is very dangerous since it breaks if one calls build() in unexpected ways.

It is exactly the same as borrowing the temporary array (unless making it static or const):

error[E0716]: temporary value dropped while borrowed
  --> examples/src/bin/texture.rs:91:35
   |
91 |                     .attachments(&[vk::ImageView::null(); 3])
   |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^ creates a temporary which is freed while still in use
92 |                     .layers(1);
   |                               - temporary value is freed at the end of this statement
...
95 |                     .create_framebuffer(&frame_buffer_create_info, None)
   |                                         ------------------------- borrow later used here
   |
   = note: consider using a `let` binding to create a longer lived value

Both are useless except if creating the object/builder within the same expression as create_framebuffer.

I suppose you just don't want to make an exception for this

I very well want to make an exception for this as mentioned above, I just don't want to make it the default for other builders where slice-length and slice-pointer are completely tied together. Regardless of general unsafety, Ash builders try to hold ones hand to the best of our abilities when pointer-lifetime is concerned.

[kvark]

Instead of exposing this for all the builders, which will be rather useless, maybe we can make this one more special, like

builder.imageless_attachment_count(4)

which would set the attachment_count and add the imageless flag at the same time?

[nerditation]

Spec says that pAttachments field is ignored for imageless framebuffer, but attachmentCount field must still be filled with non zero value in that case.

I came across a very similar situation, in my case it were the scissorCount (and viewportCount) fields in the VkPipelineViewportStateCreateInfo struct. when using dynamic viewport state, the pViewPorts pointer is ignored, but the viewportCcount is always required. luckily I got a validation layer report.

interestingly, when I go back and fix the code, the PipelineViewportStateCreateInfoBuilder struct actually already has these builder/setter methods: scissor_count(self, u32) -> Self and viewport_count(self, u32) -> Self, which is surprising to me, as I don't recall seeing other struct with similar situation.

I looked through the other usages of count, and indeed descriptor_count may be the only non-trivial one. So we can consider attachment_count somewhat unique. [...]

after some digging, I found it was added in #143, where several special cases (including the scissor_count and viewport_count) were defined, so this attachment_count could just be another one, I think?