-
-
Notifications
You must be signed in to change notification settings - Fork 117
feature/enable vulkan raytracing extensions #1827
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
base: master
Are you sure you want to change the base?
Conversation
|
Change looks good but what happens if the user does not have a device that supports raytracing ? will it just safely ignore the changes? |
9b7c6a1 to
858107b
Compare
858107b to
7962fb8
Compare
Yes it will be safe. According to Qt documententation, it is safe to add deviceExtensions even if the physical device might not support them.
Also, I've moved the code for creating the vulkan device and enabling raytracing extensions to |
|
thanks! will take some time to review it today |
| return g_staticVulkanInstance; | ||
| } | ||
|
|
||
| // Returns true if the device supports ray tracing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the vulkan code should be in the
#if defined(QT_FEATURE_vulkan) && QT_CONFIG(vulkan) && __has_include(<vulkan/vulkan.h>)
guard as some platforms don't have vulkan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not really sure, but I think all the vulkan related code here is fully wrapped in the guard.
Let me know if I missed anything, happy to adjust further.
src/lib/score/gfx/Vulkan.cpp
Outdated
| #include <score/gfx/Vulkan.hpp> | ||
|
|
||
| #if defined(QT_FEATURE_vulkan) && QT_CONFIG(vulkan) && __has_include(<vulkan/vulkan.h>) | ||
| #include "QRhiGles2.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should go in here, and it should be with <qrhigles2.hpp> header formata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I've made a mistake here. I've changed it to correct header.
a402674 to
c16c15a
Compare
ScreenNode: add vulkan device extensions when initializing vulkan instances