KhronosGroup / Vulkan-Docs

The Vulkan API Specification and related tools
Other
2.79k stars 467 forks source link

`#include "vk_video/…"` makes `vulkan_beta.h` unusable without an additional include path in build settings #1769

Open Triang3l opened 2 years ago

Triang3l commented 2 years ago

The vulkan.h file normally can be included from anywhere without any changes to the build settings of the project. However, vulkan_beta.h includes headers like "vk_video/vulkan_video_codec_h264std.h", but the vk_video directory is not located in the same directory as vulkan_beta.h — rather, it's placed one level above. This makes it impossible to include vulkan_beta.h without adding a new include search path to the project build settings.

The same issue was present in the header files inside vk_video — they used to include "vk_video/vulkan_video_codecs_common.h" previously, also relative to the includes of the whole SDK rather than the directory itself.

Can the vk_video headers be included by their paths relative to the actual files they are included in (such as "../vk_video/vulkan_video_codec_h264std.h" in vulkan_beta.h, and "vulkan_video_codecs_common.h" inside the vk_video directory itself)?

Specifically, I'd also want to avoid relying on the include directories specified in the build system's search paths (and most importantly their order) completely, as in our projects we have a local copy of the Vulkan headers of the version we're targeting (though we'll likely switch to this repository as a submodule soon, as I've been made aware of the existence of this repository) so the project can be predictably built regardless of the Vulkan SDK version is installed in the system.

DethRaid commented 2 years ago

I'd also want to avoid relying on the include directories specified in the build system's search paths

I don't understand. What is the problem with relying on your build system to specify include paths?

Triang3l commented 2 years ago

I don't understand. What is the problem with relying on your build system to specify include paths?

The dependency on ordering of the search paths. I have already seen a build system doing weird things with include search directories, who knows what a potential OS toolchain that itself contains Vulkan headers, for example, may bring — and I want to have control over which specific headers will be included, to make sure they're not older than my target version.

In case of Vulkan, it also seems unnecessary to use the build include search paths, as no long ../ chains are needed, and there doesn't seem to be any maintainability improvement from that (while resulting in breaking building of existing projects that use VK_ENABLE_BETA_EXTENSIONS), so far the headers are either in the same directory or in the sibling.

Additionally, not all build systems have transitive include directory support — and the situation here becomes even worse when you're generating the build scripts using a tool like Premake. In my case, I have Vulkan usage spread across modules (initialization/UI, rendering and multiple other modules related to them). In all the modules, I'm including vulkan.h through an additional header in the UI module (that also contains the setup of VK_USE_PLATFORM_, VK_ENABLE_BETA_EXTENSIONS and VK_NO_PROTOTYPES, as well as my custom class for accessing the device and other Vulkan objects) — however, I have to specify the Vulkan-Headers include search directory in the settings of all those modules — it's extremely inconvenient, with a lot of duplication in build scripts, and revealing the internals of one module to everything using it. Specifically, to use include our ui/vulkan/vulkan_provider.h from xenia-ui-vulkan, we had to add the include path to the following modules in our project:

oddhack commented 2 years ago

Moving to Vulkan-Docs. Vulkan-Headers is purely a repository for generated files, no development takes place here.

oddhack commented 2 years ago

@zlatinski can you bring this up (again) in the Video TSG? Unfortunately at this point whatever is done or not done, somebody is going to have a build setup that's affected. But the extensions are still provisional. I'm inclined towards making vk_video a directory under vulkan/ rather than parallel to it.

Triang3l commented 2 years ago

1573 (didn't notice it initially — closed as resolved, but it looks like the related changed were related to vk_platform.h only, not the video headers) also covers the same issue in another project (and #1610 is also related). I think keeping vk_video a separate directory, while using file-relative #include paths, may resolve the issue while preserving backward compatibility (though I'm not sure if including the video headers directly, as opposed to via vulkan.h, is valid at all — and if not, possibly no issues should be caused by moving vk_video under vulkan, which, I think, would be more logically justified, as video is a part of the Vulkan API rather than a separate, independently usable component or a utility library).

oddhack commented 2 years ago

The vk_video headers are generated from a separate XML description and shouldn't have dependencies on the vulkan/ headers. But they have no apparent use other than passing into some of the video APIs, so why someone would want to include them separately isn't obvious.

zlatinski commented 2 years ago

Can the vk_video headers be included by their paths relative to the actual files they are included in (such as "../vk_video/vulkan_video_codec_h264std.h" in vulkan_beta.h, and "vulkan_video_codecs_common.h" inside the vk_video directory itself)?

I think this can easily be done. However, the reasons we keep these files outside the vulkan/ are:

In addition, l do not think adding an additional build path for the vk_video include headers is a problem for any build system.

TheMostDiligent commented 1 year ago

After updating the headers, I also started getting build errors due to the same problem as in original issue #1573: vulkan_core.h is not a self-contained header anymore and can't be included without specifying additional include directories, which is incorrect.

Our project does not use vk_video, however it fails to compile because of references to vk_video/vulkan_video_codec_h264std.h

mackron commented 1 year ago

I had whinge about this a few weeks ago. The whole separate headers thing is utterly ridiculous and just adds unnecessary complications for people who don't care the video stuff. My opinion is that the video stuff should all be part of vulkan.h like everything else. What's so special about vk_video?

TheMostDiligent commented 1 year ago

The headers should at the very least use relative paths when including other headers. Referencing headers with respect to parent directory is really strange

timprepscius commented 5 months ago

"Referencing headers with respect to parent directory is really strange" I second this. Because you shouldn't need to.

If vk_video is "part of core" it should be within that directory. If you need to access from "not core" it should use <> instead of ""

At least that is my personal opinion.

Also, could you make a define option that excludes anything like this. I don't want to have to mess with project files from minor version to minor version.

I realize now that this issue pertains to vulkan_beta.h, but I'm actually referring to the most current release.

oddhack commented 5 months ago

@aqnuep suggest you bring this back onto the Video TSG agenda. It was never resolved and is obviously still causing some people issues.

aqnuep commented 5 months ago

Will do. Thanks for pinging me, @oddhack!

zlatinski commented 4 months ago

It is a common software practice to use namespace folders for C++ include files. The software has to configure the top-level directory where those name-space directories are present as the include directory (for ex. -I/usr/include). In this case, we've got vulkan/ and vk_video/ namespaces, so software should use the namespace directory along with the included filename. For example, if the headers are installed at /usr/include directory, then this directory should be used as the compiler include path, and then vulkan/vulkan.h and vk_video/vulkan_video_codec_h264std.h should be used within the C++ files. Alternately, the compiler's include directory can be set at the namespace levels (for ex. /usr/include, /usr/include/vulkan and /usr/include/vk_video) where the file names without the namespace directories can be used directly.

timprepscius commented 4 months ago

I'll fix my builds to work with whatever you do.

My current understanding for includes is: for all public include "" should be a relative path, and <> should be an external library path. never use <> for an internal, never use "" for an external

hmm.. do I believe what I just wrote... I think so.

for private includes, you can do whatever you want, because no one besides you/project interact with them

anyway.

it doesn't really matter.