Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LayerSettingEXTBuilder sets value_count field incorrectly #290

Open
max-kamps opened this issue Sep 11, 2024 · 2 comments
Open

LayerSettingEXTBuilder sets value_count field incorrectly #290

max-kamps opened this issue Sep 11, 2024 · 2 comments

Comments

@max-kamps
Copy link

For the VkLayerSettingEXT struct, you are expected to pass a type enum, an array size, and a pointer to an array.
For example, to pass an array of 3 uint32s, you would set

.type = VK_LAYER_SETTING_TYPE_UINT32_EXT;
.valueCount = 3;
.pValues = &{42, 413, 6502};

In this example, pValues points to a 12 byte long array, holding 3 uint32 values.

However, this combination of settings is (almost) impossible through the LayerSettingEXTBuilder API. That is because it accepts a &[u8] as the values, and uses its length as the value count. In other words, it always assumes that the number of values is equal to the number of bytes:

pub fn values(mut self, values: &'b [u8]) -> Self {
    self.value.value_count = values.len() as u32;
    self.value.values = values.as_ptr().cast();
    self
}

(It is technically possible to "trick" the function, by creating a fake 3-element u8 slice pointing at the values, but clearly that is not the intended way to use it 😄)

To correctly set the value, it would either have to accept a slice of generic type, or have separate methods to set the value count and pointer.

@max-kamps
Copy link
Author

I have thought about it more, and came up with a third solution:
It would also be possible to have a separate values method for each possible type, which sets both the type_, value_count, and values appropriately. For example

pub fn values_uint32(mut self, values: &'b [u32]) -> Self {
    self.value.type_ = vk::LayerSettingTypeEXT::UINT32;
    self.value.value_count = values.len() as u32;
    self.value.values = values.as_ptr().cast();
    self
}

That could actually be quite nice, since it eliminates the error of setting the wrong type_ accidentally.

@KyleMayes
Copy link
Owner

KyleMayes commented Sep 12, 2024

Thank you for opening an issue!

This is definitely a problem with the code generation which I will fix.

The Vulkan XML specification (https://github.com/KhronosGroup/Vulkan-Docs/blob/main/xml/vk.xml) which these builders are generated from often indicates that some fields in Vulkan structs indicate the length of other array/pointer fields in the struct. To avoid repetition and a whole class of mistakes, the generated builders bundle the setting of these fields together so that there's no possibility that the slices provided to the builder could disagree with the lengths. (you may know this already but I like writing stuff like this down so I can remember it later)

However, this struct is a bit unusual in that it has a void* field (pValues) dependent on the length of another field (valueCount) but the "true" length of that field needs to be multiplied by the value implied by another field (type).

I like your idea of separate methods for each type.
It would require some specialization in the code generation for this struct, but it seems appropriate here.

I will try to look into this in the next day or two.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants