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

cube: Make Volk requirement explicit #992

Merged
merged 1 commit into from
Jun 5, 2024
Merged

cube: Make Volk requirement explicit #992

merged 1 commit into from
Jun 5, 2024

Conversation

lunarpapillo
Copy link
Contributor

See also:

Cannot build DEMOS.sln
https://gitlab.khronos.org/vulkan/Vulkan-SDK-Packaging/-/issues/1417

Volk requires that VK_NO_PROTOTYPES be defined before vulkan.h or vulkan.hpp is included. Currently, the various flavors of vkcube hide this definition in the cube/CMakeLists.txt file, which can confuse users who may copy the source for their own use, and may require investigation to figure out why it doesn't "just work".

This change makes the #define explicit in the cube.c and cube.cpp source files, which should both be clearer and be more similar to how most applications use Volk.

@ci-tester-lunarg
Copy link

CI Vulkan-Tools build queued with queue ID 181327.

1 similar comment
@ci-tester-lunarg
Copy link

CI Vulkan-Tools build queued with queue ID 181327.

@ci-tester-lunarg
Copy link

CI Vulkan-Tools build # 1443 running.

@ci-tester-lunarg
Copy link

CI Vulkan-Tools build # 1443 failed.

@charles-lunarg
Copy link
Contributor

@ci-tester-lunarg
Copy link

CI Vulkan-Tools build queued with queue ID 186219.

1 similar comment
@ci-tester-lunarg
Copy link

CI Vulkan-Tools build queued with queue ID 186219.

@ci-tester-lunarg
Copy link

CI Vulkan-Tools build # 1453 running.

@ci-tester-lunarg
Copy link

CI Vulkan-Tools build # 1453 failed.

@ci-tester-lunarg
Copy link

CI Vulkan-Tools build queued with queue ID 186986.

1 similar comment
@ci-tester-lunarg
Copy link

CI Vulkan-Tools build queued with queue ID 186986.

@ci-tester-lunarg
Copy link

CI Vulkan-Tools build # 1455 running.

@ci-tester-lunarg
Copy link

CI Vulkan-Tools build # 1455 passed.

See also:

    Cannot build DEMOS.sln
    https://gitlab.khronos.org/vulkan/Vulkan-SDK-Packaging/-/issues/1417

Volk requires that VK_NO_PROTOTYPES be defined before vulkan.h or vulkan.hpp is
included.  Currently, the various flavors of vkcube hide this definition in the
cube/CMakeLists.txt file, which can confuse users who may copy the source for their
own use, and may require investigation to figure out why it doesn't "just work".

This change makes the #define explicit in the cube.c and cube.cpp source files,
which should both be clearer and be more similar to how most applications use Volk.

cube/CMakeLists.txt:
- remove

- vulkaninfo already had a #VK_NO_PROTOTYPES somewhere
- macOS DemoViewController includes of mvk_vulkan.h are confusing Volk and don't seem to be necessary,
  since cube.c and cube.cpp #include vulkan.h and vulkan.hpp, respectively
@ci-tester-lunarg
Copy link

CI Vulkan-Tools build queued with queue ID 187054.

1 similar comment
@ci-tester-lunarg
Copy link

CI Vulkan-Tools build queued with queue ID 187054.

@ci-tester-lunarg
Copy link

CI Vulkan-Tools build # 1456 running.

@ci-tester-lunarg
Copy link

CI Vulkan-Tools build # 1456 passed.

@lunarpapillo
Copy link
Contributor Author

Seems like the define needs to be added to these files as well: https://github.com/KhronosGroup/Vulkan-Tools/blob/main/cube/macOS/cube/DemoViewController.m https://github.com/KhronosGroup/Vulkan-Tools/blob/main/cube/macOS/cubepp/DemoViewController.mm

🤣 you figured this out before I did! Blast it, I didn't see your response here while I was working it out in the Vulkan-SDK-Packaging issue - would have saved me some time and kept me from pulling as much of my already sparse hair out...

@lunarpapillo
Copy link
Contributor Author

@charles-lunarg @mikes-lunarg I noted this in the issue, and am reiterating it here...

Is it better to remove the #include <MoltenVK/mvk_vulkan.h> line from DemoViewController, or better to keep it and just add a #define VK_NO_PROTOTYPES above it in that same file? (By "better", I think I mean: which demonstrates to the user the more correct usage?)

The point of doing this as a Vulkan-Tools change instead of an SDK change is to make the use of Volk more "proper". But I'm not sure which of the above is more "proper"...

@lunarpapillo
Copy link
Contributor Author

From @charles-lunarg :

Removing the mvk_vulkan.h is correct- that was an OLD way of using moltenvk anyhow (aka a "private" header from moltenvk that defined vulkan API things outside of the actual vulkan API headers)

So I'm sticking with the current implementation. I'll rebase to make sure nothing has staled.

@lunarpapillo lunarpapillo merged commit 5aac257 into main Jun 5, 2024
37 checks passed
@lunarpapillo lunarpapillo deleted the bob-volk-define branch June 5, 2024 20:23
@charles-lunarg
Copy link
Contributor

For anyone reading this comment thread, I would like to clarify that mvk_vulkan.h isn't "outdated" but rather contains stuff not in standard vulkan, and therefore isn't cross platform. vkcube & vkcubepp do not use anything in the header, so its safe to remove.

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

Successfully merging this pull request may close these issues.

3 participants