search

PixarAnimationStudios / OpenUSD

Universal Scene Description
http://www.openusd.org
Other
5.45k stars 1.13k forks source link

Include file name collision in vk_mem_alloc.h #3032

Closed pgrossomoreira closed 1 month ago

pgrossomoreira commented 1 month ago

Hi. I'm creating a conan package for OpenUSD and I'm running into the following header name collision:

pxr/imaging/hgiVulkan/vk_mem_alloc.h includes <vma/vk_mem_alloc.h>. However, my distribution of vulkan memory allocator simply provides vk_mem_alloc.h instead of vma/vk_mem_alloc.h, so I removed the vma/ part of the include path from OpenUSD's header file. As a result, OpenUSD's vk_mem_alloc.h now includes itself instead of including the intended Vulkan header.

This issue will go away if I just rename OpenUSD's vk_mem_alloc.h to a different name, but I'm not sure if I can do that without breaking something in OpenUSD's build system. For instance, I see that vk_mem_alloc is mentioned in pxr/imaging/hgiVulkan/CMakeLists.txt. I'm afraid that there might be unintended side effects from renaming the file.

Can I just rename pxr/imaging/hgiVulkan/vk_mem_alloc.h to something else and change pxr/imaging/hgiVulkan/CMakeLists.txt accordingly without any ramifications? I don't understand what the PUBLIC_CLASSES section of that cmake file does.

Thank you.

jesschimein commented 1 month ago

Filed as internal issue #USD-9524

pgrossomoreira commented 1 month ago

@jesschimein Thanks but I didn't mean to report any issue. I'm just asking a question. Can you provide any estimate for when someone will be able to respond? Thank you.

spiffmon commented 1 month ago

I'll try to get an expert to weigh in, but can you alternately create a vma subdirectory in your vulkan header install and put a link inside it to ../vk_mem_alloc.h (if that's the only offensive header)?

davidgyu commented 1 month ago

HgiVulkan includes that header via #include <vma/vk_mem_alloc.h> since that matches the install location of the header within the VulkanSDK (since 1.3.216) so I like @spiffmon's suggestion that you should support that include path in your packaging as well.

We could certainly consider renaming hgiVulkan/vk_mem_alloc.h and I think it's likely that we eventually will make it a private header file within the HgiVulkan implementation, but I think we would continue to include the implementation header using its VulkanSDK install location.

jesschimein commented 1 month ago

Hi @pgrossomoreira, I'll close the issue! :)

There's a really great forum at https://forum.aousd.org/ that's a wonderful (and our preferred) place for questions. We have a pretty small team monitoring issues here and hundreds of lovely folks from the broader USD and Hydra community in the forum. I hope it can be a helpful resource!