-
Notifications
You must be signed in to change notification settings - Fork 123
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
Beau test app #1839
base: dev
Are you sure you want to change the base?
Beau test app #1839
Conversation
… and device creation
CI gfxreconstruct build # 5210 running. |
CI gfxreconstruct build # 5210 passed. |
709441a
to
4d432eb
Compare
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.
Overall looks to be what I expect.
As the vk-bootstrap author, I want to suggest so many minor things because they were written for the 'library' which can't assume how the code will be used - whereas here we can make that assumption and remove it. But for the time being that is just fine, it won't hurt us to have.
This looks to be a fine base to create bespoke test cases with. None of my comments are fixes, just comments about the code.
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 happy to look into rewriting this using the existing python codegen so that its consistent with the rest of the repo. Because vk-bootstrap is its own repo, it re-implements a lot of logic that can be removed by using gfxr common codegen.
/* If headless mode is false, by default vk-bootstrap use the following logic to enable the windowing extensions | ||
|
||
#if defined(_WIN32) | ||
VK_KHR_win32_surface | ||
#elif defined(__linux__) | ||
VK_KHR_xcb_surface | ||
VK_KHR_xlib_surface | ||
VK_KHR_wayland_surface | ||
#elif defined(__APPLE__) | ||
VK_EXT_metal_surface | ||
#elif defined(__ANDROID__) | ||
VK_KHR_android_surface | ||
#elif defined(_DIRECT2DISPLAY) | ||
VK_KHR_display | ||
#endif |
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.
vk-bootstrap auto enables surface extensions which is fine for everything BUT linux. The code enables xcb, xlib, & wayland surface extensions if available. So its up to SDL to choose the right extension. Wanted to mention this behavior in case you think the code should enable only 1 surface extension at a time.
// Convenient named constants for passing to set_desired_min_image_count(). | ||
// Note that it is not an `enum class`, so its constants can be passed as an integer value without casting | ||
// In other words, these might as well be `static const int`, but they benefit from being grouped together this way. | ||
enum BufferMode |
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.
Could use enum class
here :)
VkQueue graphics_queue; | ||
VkQueue present_queue; | ||
|
||
std::vector<VkImage> depth_images; | ||
std::vector<VmaAllocation> depth_image_allocations; | ||
std::vector<VkImageView> depth_image_views; | ||
|
||
std::vector<VkImage> render_targets; | ||
std::vector<VmaAllocation> render_target_allocations; | ||
std::vector<VkImageView> render_target_views; | ||
|
||
std::vector<VkFramebuffer> framebuffers; | ||
|
||
VkRenderPass render_pass; | ||
VkPipelineLayout pipeline_layout; | ||
VkPipeline graphics_pipeline; | ||
|
||
VkCommandPool command_pools[MAX_FRAMES_IN_FLIGHT]; | ||
|
||
size_t current_frame = 0; |
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.
These objects are going to be useful for a lot of test cases, would be worth sub classing TestAppBase with something like TestAppRenderLoopBase
which adds the basic renderpass, color target, depth target, etc. Akin to Vulkan-Samples.
Fine to have this here for a first draft while we figure out what is "actually" common and what needs to be bespoke code per test.
Fix shader copies and dll dependencies for test apps
CI gfxreconstruct build queued with queue ID 295110. |
.gitmodules
Outdated
@@ -7,3 +7,6 @@ | |||
[submodule "external/SPIRV-Reflect"] | |||
path = external/SPIRV-Reflect | |||
url = https://github.com/KhronosGroup/SPIRV-Reflect.git | |||
[submodule "external/SDL"] | |||
path = external/SDL | |||
url = https://github.com/libsdl-org/SDL.git |
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.
The change says "ported vk boostrap" but I don't see it added as a submodule.
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.
It wasn't usable without deep changes, so I ended up copying and reworking the code
@@ -0,0 +1,9617 @@ | |||
// *** THIS FILE IS GENERATED - DO NOT EDIT *** |
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.
Why not use a submodule here?
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.
SDL3 was originally a submodule. In the meeting last week it was mentioned that we were moving to FetchContent where possible, so I switched it.
@@ -943,6 +943,18 @@ VkShaderModule readShaderFromFile(DispatchTable const& disp, const std::string& | |||
|
|||
#define VERIFY_VK_RESULT(message, result) { if (result != VK_SUCCESS) throw gfxrecon::test::vulkan_exception(message, result); } | |||
|
|||
struct Init { |
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.
Nit: I'd like this to be InitInfo
or something a little more "object" not action named. Just really a nit because looking at it in the code, I wouldn't easily be able to tell from the name that it's a struct/class.
test/test_apps/triangle/triangle.cpp
Outdated
void create_command_buffers(gfxrecon::test::Init& init, RenderData& data) { | ||
data.command_buffers.resize(data.framebuffers.size()); | ||
void Triangle::create_command_buffers() { | ||
this->command_buffers.resize(this->framebuffers.size()); |
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.
Do you really need to use this->
everywhere here?
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.
Fixed
void device_initialization_phase_1(const std::string& window_name, Init& init) | ||
{ | ||
init.window = gfxrecon::test::create_window_sdl(window_name.data(), true, 1024, 1024); | ||
} | ||
|
||
gfxrecon::test::InstanceBuilder instance_builder; | ||
init.instance = instance_builder.use_default_debug_messenger().request_validation_layers().build(); | ||
void device_initialization_phase_2(InstanceBuilder const& instance_builder, Init& init) | ||
{ | ||
init.instance = instance_builder.build(); | ||
|
||
init.inst_disp = init.instance.make_table(); | ||
|
||
init.surface = gfxrecon::test::create_surface_sdl(init.instance, init.window); | ||
} | ||
|
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.
What's the benefit of splitting this up?
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.
Between phase 1 and 2, there is an opportunity for test apps to customize instance creation via the InstanceBuilder.
Between phases 2 and 3, apps can customize physical device selection via the PhysicalDeviceSelector.
Between phase 3 and 4, apps can customize device creation via the DeviceBuilder.
Betwen phase 4 and 5, apps can customize swapchain creation via the SwapchainBuilder.
This is threaded together in TestAppBase::run. However, I left open the possiblity of test apps not using TestAppBase and left the device_initialization method from VkBootstrap as an alternative. That method does not allow for the customizations above. I used these "phase" functions to make sure the two implementations were similar. In the future I could see making them part of the public interface if people need customization but don't want to use TestAppBase. Or we could force TestAppBase and collapse the functionality into TestAppBase::run.
CI gfxreconstruct build # 5287 running. |
CI gfxreconstruct build # 5287 failed. |
Renamed Init to InitInfo
CI gfxreconstruct build queued with queue ID 296059. |
CI gfxreconstruct build # 5301 running. |
CI gfxreconstruct build # 5301 failed. |
App() = default; | ||
|
||
private: | ||
VkQueue graphics_queue; |
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 guess we can follow these guidelines adding an underscore at the end to member variable names, so it's more clear when they are used in methods, therefore this->
could be dropped with no issues.
test/test_apps/triangle/app.cpp
Outdated
|
||
GFXRECON_BEGIN_NAMESPACE(triangle) | ||
|
||
const int MAX_FRAMES_IN_FLIGHT = 2; |
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.
nit: I would use size_t
here as it is used for sizing the command_pools
array and current_frame
is size_t
as well.
CI gfxreconstruct build queued with queue ID 298367. |
CI gfxreconstruct build # 5324 running. |
CI gfxreconstruct build # 5324 failed. |
No description provided.