Beau test app #1839

beau-lunarg · 2024-10-27T19:15:28Z

No description provided.

ci-tester-lunarg · 2024-10-27T19:15:31Z

CI gfxreconstruct build # 5210 running.

ci-tester-lunarg · 2024-10-27T19:41:16Z

CI gfxreconstruct build # 5210 passed.

charles-lunarg

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.

charles-lunarg · 2024-10-28T15:38:17Z

test/test_apps/common/generate_dispatch.py

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.

charles-lunarg · 2024-10-28T15:46:51Z

test/test_apps/common/test_app_base.h

+/* 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


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.

charles-lunarg · 2024-10-28T15:49:54Z

test/test_apps/common/test_app_base.h

+    // 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


Could use enum class here :)

charles-lunarg · 2024-10-28T15:55:53Z

test/test_apps/multisample-depth/app.cpp

+    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;


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

test/test_apps/CMakeLists.txt

ci-tester-lunarg · 2024-11-04T21:02:34Z

CI gfxreconstruct build queued with queue ID 295110.

CMakeLists.txt

MarkY-LunarG · 2024-11-04T21:00:56Z

.gitmodules

@@ -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


The change says "ported vk boostrap" but I don't see it added as a submodule.

It wasn't usable without deep changes, so I ended up copying and reworking the code

MarkY-LunarG · 2024-11-04T21:10:44Z

external/Vulkan-Utility-Libraries/vk_enum_string_helper.h

@@ -0,0 +1,9617 @@
+// *** THIS FILE IS GENERATED - DO NOT EDIT ***


Why not use a submodule here?

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.

MarkY-LunarG · 2024-11-04T21:14:58Z

test/test_apps/common/test_app_base.h

@@ -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 {


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.

MarkY-LunarG · 2024-11-04T22:01:30Z

test/test_apps/triangle/triangle.cpp

-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());


Do you really need to use this-> everywhere here?

MarkY-LunarG · 2024-11-04T22:03:27Z

test/test_apps/common/test_app_base.cpp

+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);
+}



What's the benefit of splitting this up?

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 · 2024-11-04T22:49:13Z

CI gfxreconstruct build # 5287 running.

ci-tester-lunarg · 2024-11-04T23:20:59Z

CI gfxreconstruct build # 5287 failed.

ci-tester-lunarg · 2024-11-05T16:24:55Z

CI gfxreconstruct build queued with queue ID 296059.

ci-tester-lunarg · 2024-11-05T16:45:40Z

CI gfxreconstruct build # 5301 running.

ci-tester-lunarg · 2024-11-05T17:25:45Z

CI gfxreconstruct build # 5301 failed.

antonio-lunarg · 2024-11-05T20:14:21Z

test/test_apps/triangle/app.cpp

+    App() = default;
+
+  private:
+    VkQueue graphics_queue;


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

antonio-lunarg · 2024-11-05T20:24:40Z

test/test_apps/triangle/app.cpp

+
+GFXRECON_BEGIN_NAMESPACE(triangle)
+
+const int MAX_FRAMES_IN_FLIGHT = 2;


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 · 2024-11-08T12:52:34Z

CI gfxreconstruct build queued with queue ID 298367.

ci-tester-lunarg · 2024-11-08T12:55:01Z

CI gfxreconstruct build # 5324 running.

ci-tester-lunarg · 2024-11-08T13:24:29Z

CI gfxreconstruct build # 5324 failed.

ci-tester-lunarg · 2024-11-22T16:01:37Z

CI gfxreconstruct build queued with queue ID 308301.

ci-tester-lunarg · 2024-11-23T16:41:02Z

CI gfxreconstruct build # 308301 lost.

ci-tester-lunarg · 2024-11-25T01:14:59Z

CI gfxreconstruct build queued with queue ID 308594.

ci-tester-lunarg · 2024-11-25T01:18:39Z

CI gfxreconstruct build # 5375 running.

ci-tester-lunarg · 2024-11-25T01:56:39Z

CI gfxreconstruct build # 5375 failed.

antonio-lunarg · 2024-11-25T11:03:00Z

framework/decode/vulkan_replay_consumer_base.cpp

+{
+    GFXRECON_UNREFERENCED_PARAMETER(original_result);
+
+    assert((physical_device_info != nullptr) && (pDevice != nullptr) && !pDevice->IsNull() &&


GFXRECON_ASSERT

… and device creation

Fix shader copies and dll dependencies for test apps

Renamed Init to InitInfo

beau-lunarg requested a review from bradgrantham-lunarg October 27, 2024 19:15

beau-lunarg force-pushed the beau-test-app branch from 709441a to 4d432eb Compare October 27, 2024 19:44

charles-lunarg reviewed Oct 28, 2024

View reviewed changes

ziga-lunarg reviewed Nov 2, 2024

View reviewed changes

test/CMakeLists.txt Outdated Show resolved Hide resolved

ziga-lunarg reviewed Nov 2, 2024

View reviewed changes

test/test_apps/CMakeLists.txt Outdated Show resolved Hide resolved

ziga-lunarg mentioned this pull request Nov 2, 2024

Add tests for pipeline binaries, host image copy and shader objects #1864

Closed

MarkY-LunarG reviewed Nov 4, 2024

View reviewed changes

antonio-lunarg reviewed Nov 5, 2024

View reviewed changes

antonio-lunarg reviewed Nov 25, 2024

View reviewed changes

beau-lunarg added 5 commits November 25, 2024 15:00

Ported vk bootstrap for test apps

7eeff9e

Common test app cmake target

504fe3b

Move window functions to test app base

c9428cb

Move swapchain creating to test app base

4064bdf

Refactor framebuffer creation into test app base

bbf1505

beau-lunarg and others added 29 commits November 25, 2024 15:00

Move swapchain creation for init to test app base

ef30915

Test app base version of recreate swapchain

30207f2

Test app base class

5bb429e

Test apps can configure instance creation, physical device selection,…

16a5d0d

… and device creation

Cleanup triangle

d4ea864

Include cleanup

61d565d

Multisample depth test app

0535e34

Fix multiple frames

195b4f5

Triangle test with clean validation

09daba7

Clean validation for multisample

113b4c1

Format

ed054cd

Tricky format issue

0b2671a

Remove sdl submodule

6460723

Add SDL3 dependency through CMAKE

3f176d8

Fix shader copies and dll dependencies for test apps

Win32 conditional to copy dlls

d73ab90

Fix copyright names and years

f597959

Setup extensions from device builder

1ea0c1d

Fix formatting

054eba0

Strange formatting issue

2e91555

Removed overuse of this->.

f23a480

Renamed Init to InitInfo

Fix format issues

aabb040

Same formatting issue as before

9a708ad

Test pipeline binaries and host image copy

960d097

Add test for shader objects

f439d85

Change max frames to size_t

c4f14ac

Fix formatting in pipeline binaries test app

c77373d

Mark configure methods as override in pipeline-binaries test app

83962e6

Fix format issues in shader objects test app

bcba2ee

Add missing overrides in host image copy and shader objects test apps

8c45faa

beau-lunarg force-pushed the beau-test-app branch from 62202d8 to 8c45faa Compare November 25, 2024 23:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Beau test app #1839

Beau test app #1839

beau-lunarg commented Oct 27, 2024

ci-tester-lunarg commented Oct 27, 2024

ci-tester-lunarg commented Oct 27, 2024

charles-lunarg left a comment

charles-lunarg Oct 28, 2024

charles-lunarg Oct 28, 2024

charles-lunarg Oct 28, 2024

charles-lunarg Oct 28, 2024

ci-tester-lunarg commented Nov 4, 2024

MarkY-LunarG Nov 4, 2024

beau-lunarg Nov 5, 2024

MarkY-LunarG Nov 4, 2024

beau-lunarg Nov 5, 2024

MarkY-LunarG Nov 4, 2024

MarkY-LunarG Nov 4, 2024

beau-lunarg Nov 5, 2024

MarkY-LunarG Nov 4, 2024

beau-lunarg Nov 5, 2024

ci-tester-lunarg commented Nov 4, 2024

ci-tester-lunarg commented Nov 4, 2024

ci-tester-lunarg commented Nov 5, 2024

ci-tester-lunarg commented Nov 5, 2024

ci-tester-lunarg commented Nov 5, 2024

antonio-lunarg Nov 5, 2024

antonio-lunarg Nov 5, 2024

ci-tester-lunarg commented Nov 8, 2024

ci-tester-lunarg commented Nov 8, 2024

ci-tester-lunarg commented Nov 8, 2024

ci-tester-lunarg commented Nov 22, 2024

ci-tester-lunarg commented Nov 23, 2024

ci-tester-lunarg commented Nov 25, 2024

ci-tester-lunarg commented Nov 25, 2024

ci-tester-lunarg commented Nov 25, 2024

antonio-lunarg Nov 25, 2024

		@@ -0,0 +1,9617 @@
		// * THIS FILE IS GENERATED - DO NOT EDIT *

		@@ -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 {


		GFXRECON_BEGIN_NAMESPACE(triangle)

		const int MAX_FRAMES_IN_FLIGHT = 2;

Beau test app #1839

Are you sure you want to change the base?

Beau test app #1839

Conversation

beau-lunarg commented Oct 27, 2024

ci-tester-lunarg commented Oct 27, 2024

ci-tester-lunarg commented Oct 27, 2024

charles-lunarg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ci-tester-lunarg commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ci-tester-lunarg commented Nov 4, 2024

ci-tester-lunarg commented Nov 4, 2024

ci-tester-lunarg commented Nov 5, 2024

ci-tester-lunarg commented Nov 5, 2024

ci-tester-lunarg commented Nov 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ci-tester-lunarg commented Nov 8, 2024

ci-tester-lunarg commented Nov 8, 2024

ci-tester-lunarg commented Nov 8, 2024

ci-tester-lunarg commented Nov 22, 2024

ci-tester-lunarg commented Nov 23, 2024

ci-tester-lunarg commented Nov 25, 2024

ci-tester-lunarg commented Nov 25, 2024

ci-tester-lunarg commented Nov 25, 2024

Choose a reason for hiding this comment