Comments (20)
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 acceptingsetVertexBindingDescriptions({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 delete
d.
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.
Related Issues (20)
- Question: VulkanHpp VULKAN_HPP_DEFAULT_DISPATCH_LOADER_DYNAMIC usage with VulkanMemoryAllocator HOT 1
- Feature request: appending to a StructureChain HOT 4
- compilation error when including vulkan_enums.hpp by itself HOT 2
- Allow destructuring of ResultValue with std::tie HOT 3
- Including `vulkan.hpp` causes compilation error with Clang HOT 5
- Feature request: Specialize std::hash for most types HOT 1
- std::min clashes with macros in windows.h HOT 5
- 'vk::ObjectDestroyShared<vk::Image>': no appropriate default constructor available HOT 3
- LayerSettingEXT.setValues HOT 1
- ArrayProxy with designated initializers
- `Null non-type template argument must be cast to template parameter type` errors in vulkan_handles.hpp with SDK 1.3.283. HOT 5
- vk::PipelineLayoutCreateInfo and vk::raii::DescriptorSetLayout HOT 2
- C++20 module does not expose *Flags{KHR,EXT}
- Move semantics for non-raii handles HOT 1
- SharedCommandBuffer constructor without SharedCommandPool parameter causes assertion fail on destroy HOT 4
- [c++20 module] getDispatchLoaderStatic is not available HOT 2
- [c++20 module] functions from vk::detail:: pulled into vk:: HOT 2
- RAII with Vulkan Memory Allocator (VMA)? HOT 5
- Confusing deprecation of extensions HOT 1
- IWYU pragmas for vulkan_{enums,funcs,structs}.hpp HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from vulkan-hpp.