Ugly set methods for arrays -> Add std::cref support for arrays in *createInfo setters about vulkan-hpp HOT 20 CLOSED

If one could design something like ArrayProxy that only could wrap lvalues, you'd have something way more fit for this purpose.

Actually std::cref wraps lvalues and dissalows temporaries.

from vulkan-hpp.

No, seriously, the functions to set some stuff in all those CreateInfo structures are derived from the members of those structures. And we don't want to filter those names to prevent any possible naming suprises.

Yeah I understand that. But, it's clear to me that some name changes have occurred to suit the C++ language (e.g. enumerations with e prefix). It's unusual for C++ to use Hungarian notation, so functions (and variables) like setPpFooBar look very foreign when read as "pointer to pointer of FooBar".

Honestly, C++ function names, IMHO, are more natural in snake-case, e.g. set_pp_foo_bar, which also looks pretty ridiculous.

The reason for the cumbersome array handling is, that we want those structures be as close to the original structures as possible. Just add some functionality, but no data.

I'm not suggesting that you add any data. I'm suggesting a convenient setter method. e.g.

	.setVertexBindingDescriptions(binding_descriptions)

Would be exactly identical to:

	.setVertexBindingDescriptionCount(binding_descriptions.size())
	.setPVertexBindingDescriptions(binding_descriptions.data())

If you want to have more intelligent classes, please have a look for example at https://github.com/nvpro-pipeline/VkHLF (which is not part of the Khronos repo!).

I'm familiar with that but I also feel like vulkan.hpp is sufficient for my needs. However I also feel it should also be as good as it can possibly be, within the limitations of accurately reflecting the standard/documentation.

from vulkan-hpp.

The problem is that accepting an ArrayProxy will result in accepting temporary objects too which will fail most likely, but not consistently if the stack isn't overwritten between defining the struct and using it. Due to this accepting setVertexBindingDescriptions({my temporary object}) is not an option.

It occurred to me that we can accept ArrayProxy, but overload for const C&&, the const rvalue references to the containers C (i.e. std::initializer_list, std::vector, std::array, and whatever else containers ArrayProxy accepts), and then mark these overloads as deleted.

Temporary objects of type T should bind to const rvalue reference (const T&&) more tightly (rather than be converted to an ArrayProxy), so this should prevent the use of temporary containers (either const or non-const).

from vulkan-hpp.

Found this one too :)

auto info = vk::PipelineLayoutCreateInfo()
	.setSetLayoutCount(set_layouts.size())
	.setPSetLayouts(set_layouts.data());

I know we can't be perfect given a certain level of automation, but perhaps this is an example of something bad?

from vulkan-hpp.

Here's another one. setPp :)

auto instance_create_info = vk::InstanceCreateInfo()
	.setEnabledLayerCount(_layers.size())
	.setPpEnabledLayerNames(_layers.data())
	.setPApplicationInfo(&application_info)
	.setEnabledExtensionCount(extensions.size())
	.setPpEnabledExtensionNames(extensions.data());

Honestly, in C++, it might be possible to drop the "p" and "pp" prefix altogether.

from vulkan-hpp.

Hi Samuel,

I totally agree with you. All those methods are, well, at least ugly. But that's the way, vk.xml planned it.

No, seriously, the functions to set some stuff in all those CreateInfo structures are derived from the members of those structures. And we don't want to filter those names to prevent any possible naming suprises.

The reason for the cumbersome array handling is, that we want those structures be as close to the original structures as possible. Just add some functionality, but no data. The way it's now, it's (hopefully) obvious who's the owner of the data (Not the structure!). If we would provide a container- or iterator-based interface for those classes, ownership of the data wouldn't be that clear. And we don't want those wrapper classes to allocate memory on their own.

If you want to have more intelligent classes, please have a look for example at https://github.com/nvpro-pipeline/VkHLF (which is not part of the Khronos repo!).

from vulkan-hpp.

The CreateInfo structures hold some pointers to some data. Therefore, the iterator-based interface is definitely not possible. Where should the corresponding pointer point to?
The std::vector-based interface would be possible, that's right. But the corresponding pointer would need to point 'into' the std::vector then, which would make me nervous. There are so many possibilities, that pointer could be rendered invalid by some seemingly innocent operation done on that vector later on.
That is, such a setter function would need to allocate and copy memory to make that work, which contradicts our zero-overhead approach.
On the other hand, when I just pass a pointer into some function, I don't expect any intelligence there and know I'm responsible for keeping the pointer valid.

from vulkan-hpp.

The CreateInfo structures hold some pointers to some data. Therefore, the iterator-based interface is definitely not possible. Where should the corresponding pointer point to?

It's possible to get a pointer to the data and a length from an iterator. But you are right, it's cumbersome and iterators are not guaranteed to be random access. So, let's scrap this idea. I just suggested it as part of brain storming the initial concept.

The std::vector-based interface would be possible, that's right. But the corresponding pointer would need to point 'into' the std::vector then, which would make me nervous. There are so many possibilities, that pointer could be rendered invalid by some seemingly innocent operation done on that vector later on.

The same logic applies to everyone who uses std::vector and passes in std::vector().data() and std::vector().size().

I understand what you mean when you say you are nervous about such an operation being unsafe or breaking due to invalid usage of std::vector and that's a totally reasonable point.

That is, such a setter function would need to allocate and copy memory to make that work, which contradicts our zero-overhead approach.

I'm not asking you to violate this constraint in any way, I agree that it is good idea to have zero overhead.

On the other hand, when I just pass a pointer into some function, I don't expect any intelligence there and know I'm responsible for keeping the pointer valid.

That's a pretty big assumption with C/C++ in any library :) Most code, if you pass in a pointer and size, you expect it to take a copy if it needs it. e.g. write, send, etc. But in general, the contract of the Vulkan API is pretty clear.

from vulkan-hpp.

I was just reading the updated README/API documentation and saw this:

Passing Arrays to Functions: The ArrayProxy

The Vulkan API has several places where which require (count,pointer) as two function arguments and C++ has a few containers which map perfectly to this pair. To simplify development the Vulkan-Hpp bindings have replaced those argument pairs with the ArrayProxy template class which accepts empty arrays and a single value as well as STL containers std::initializer_list, std::array and std::vector as argument for construction. This way a single generated Vulkan version can accept a variety of inputs without having the combinatoric explosion which would occur when creating a function for each container type.

So, does this mean what I've asked for is already implemented?

from vulkan-hpp.

Ah, so ArrayProxy is only used for function arguments but not for the structs used as part of function calls.

I believe it would be great to extend this concept to things like setVertexBindingDescriptions.

from vulkan-hpp.

Ah, so ArrayProxy is only used for function arguments but not for the structs used as part of function calls.

Correct.

I believe it would be great to extend this concept to things like setVertexBindingDescriptions.

Same for me. One could come to the conclusion that the developer has to ensure that the STL container lifetime has to be the same as the one for the struct. The problem is that accepting an ArrayProxy will result in accepting temporary objects too which will fail most likely, but not consistently if the stack isn't overwritten between defining the struct and using it. Due to this accepting setVertexBindingDescriptions({my temporary object}) is not an option.

We could remove the const modifier to prevent this accidental use, but in this case we'd also remove all other valid const container uses.

Copying the data is a natural thought, but in this case we'd have to change the struct to know whether we have to free or not to free the arrays - and changing the struct will potentially other issues.

from vulkan-hpp.

ArrayProxy is a poor choice of type for filling struct fields as it's designed to allow transparent wrapping of temporaries for the duration of a function call. This feature is a liability when it comes to a use site that fundamentally requires the wrapper data to outlive the call.

If one could design something like ArrayProxy that only could wrap lvalues, you'd have something way more fit for this purpose.

from vulkan-hpp.

Same for me. One could come to the conclusion that the developer has to ensure that the STL container lifetime has to be the same as the one for the struct.

C++ never provided this guarantee. But, some languages DO, e.g. Rust, which tracks object lifetime explicitly. So, I think it's not the responsibility of the library to ensure this, but avoiding common problems wouldn't be a bad idea.

from vulkan-hpp.

If one could design something like ArrayProxy that only could wrap lvalues, you'd have something way more fit for this purpose.

That should be possible?

from vulkan-hpp.

I concur that having functions in the style of void setVertexBindingDescriptions(vk::ArrayProxy<const vk::VertexInputBindingDescription> bindings). The fundamental problem as I understand it is that there's no guarantee that the pointer in question is valid past the point of the function call.

Take for example the following code....

        void setVertexBindingDescriptions(vk::ArrayProxy<const vk::VertexInputBindingDescription> bindings) {
            setPVertexBindingDescriptions(bindings.data());
            setVertexBindingDescriptionCount(bindings.size());
        }

This code works perfectly well if you have the following usage

        std::vector<vk::VertexInputBindingDescription> bindingDescriptions;
        vkx::PipelineVertexInputStateCreateInfo pipelineInput;
        pipelineInput.setVertexBindingDescriptions(vertices.bindingDescriptions);

as long as the pipelineInput is used before bindingDescriptions leaves scope. If bindingDescriptions leaves scope, then pipelineInput is holding a dangling pointer and you're screwed. Additionally, the function is invalid if you start to use some of functionality of ArrayProxy

pipelineInput.setVertexBindingDescriptions({ vk::VertexInputBindingDescription{} }); is instantly dangerous because the pointer into the initializer list is invalid as soon as the function returns.

The only solution would be to have PipelineVertexInputStateCreateInfo have ownership of the memory. This could be accomplished by having a member vector like so...

    struct PipelineVertexInputStateCreateInfo : public vk::PipelineVertexInputStateCreateInfo {
        ...
        std::vector<vk::VertexInputBindingDescription> vertexBindingDescriptions;
    };

You could then have a your hypothetical setVertexBindingDescriptions member implemented like this...

        void setVertexBindingDescriptions(vk::ArrayProxy<const vk::VertexInputBindingDescription> bindings) {
            vertexBindingDescriptions = toVector(bindings);
            setPVertexBindingDescriptions(vertexBindingDescriptions.data());
            setVertexBindingDescriptionCount(vertexBindingDescriptions.size());
        }

This assumes a hypothetical template<typename T> std::vector<T> toVector(const vk::ArrayProxy<const T>& proxy) which wouldn't be too hard to implement.

However, I can understand why the maintainers would be reluctant to take this approach. First, it necessarily adds a bunch of std::vector<...> members to all the structures that take parameters like this. Now everyone using vulkan.hpp has to pay for the construction and destruction of these vectors even if they aren't using this coding style. Second, it potentially adds confusion as to where the data is stored, especially if coding styles are mixed in a given codebase.

One possible solution would be a preprocessor directive that defaults to off, but can be turned on by people who want it... thus you could have something like the follwing

    struct PipelineVertexInputStateCreateInfo : public vk::PipelineVertexInputStateCreateInfo {
        ...
#if VK_EASY_VECTORS
        std::vector<vk::VertexInputBindingDescription> vertexBindingDescriptions;

        void setVertexBindingDescriptions(vk::ArrayProxy<const vk::VertexInputBindingDescription> bindings) {
            vertexBindingDescriptions = toVector(bindings);
            setPVertexBindingDescriptions(vertexBindingDescriptions.data());
            setVertexBindingDescriptionCount(vertexBindingDescriptions.size());
        }
#endif
        ...
    };

@mtavenrath, @asuessenbach what would you think of a pull request adding that kind of optional functionality?

from vulkan-hpp.

@mmaldacker I don't think a std::cref approach would work. First, allowing temporaries should be a goal. I would like to be able to write

pipelineInput.setVertexBindingDescriptions({ 
    vk::VertexInputBindingDescription{ ... },
    ...
});

Second, just because you've disallowed temporaries doesn't mean you're protected against scoping problems.

from vulkan-hpp.

@jherico If I got it right, the only scoping problems you get with cref are the ones the current interface already has. So it does not make ownership problems worse than they already are, but it increases the convenience by a lot. Unless I'm missing something here, I'm all in favor of accepting constant references to std::vectors, or really anything with a .size() and .data() method.

from vulkan-hpp.

I agree with @jseidel that adding std::cref support will fix the issues why we didn't add the ArrayProxy support to struct setters. We're going to evaluate the required changes and implications of this change.

from vulkan-hpp.

I started working on it a while ago here: https://github.com/mmaldacker/Vulkan-Hpp

from vulkan-hpp.

Nice work!

from vulkan-hpp.