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

Beau test app #1839

Open
wants to merge 42 commits into
base: dev
Choose a base branch
from
Open

Beau test app #1839

wants to merge 42 commits into from

Conversation

beau-lunarg
Copy link
Contributor

No description provided.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5210 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5210 passed.

Copy link
Contributor

@charles-lunarg charles-lunarg left a 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.

Copy link
Contributor

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.

Comment on lines +269 to +283
/* 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
Copy link
Contributor

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
Copy link
Contributor

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 :)

Comment on lines +47 to +66
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;
Copy link
Contributor

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.

test/CMakeLists.txt Outdated Show resolved Hide resolved
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 295110.

CMakeLists.txt Outdated Show resolved Hide resolved
.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
Copy link
Contributor

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.

Copy link
Contributor Author

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 ***
Copy link
Contributor

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?

Copy link
Contributor Author

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 {
Copy link
Contributor

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.

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());
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Comment on lines 2354 to 2367
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);
}

Copy link
Contributor

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?

Copy link
Contributor Author

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-tester-lunarg
Copy link

CI gfxreconstruct build # 5287 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5287 failed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 296059.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5301 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5301 failed.

App() = default;

private:
VkQueue graphics_queue;
Copy link
Contributor

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/common/test_app_base.h Show resolved Hide resolved

GFXRECON_BEGIN_NAMESPACE(triangle)

const int MAX_FRAMES_IN_FLIGHT = 2;
Copy link
Contributor

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-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 298367.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5324 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5324 failed.

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.

7 participants