CLI Samples launcher + Vulkan Hello Triangle #493

TomAtkinsonArm · 2022-07-09T15:59:38Z

Description

Large diff as this contains commits from several branches

Currently only works on Ubuntu with XLIB enabled as I have not added every window and surface type yet

loading sample configs
candidate: /build/linux/cmd/samples_launcher/samples.json
selected: /build/linux/cmd/samples_launcher/samples.json
looking for: libsample__hello_sample.so - not found
looking for: libsample__hello_triangle.so
candidate: ./build/linux/samples/hello_triangle/lib/Debug/x86_64/libsample__hello_triangle.so
selecting: ./build/linux/samples/hello_triangle/lib/Debug/x86_64/libsample__hello_triangle.so


Available Samples: (1/2)

id:             hello_triangle
name:           Hello Triangle
description:    Vulkan Hello Triangle
compile target: sample__hello_triangle

Hello Triangle!

samples.json is maintained by CMake automatically and contains metadata for all registered samples. Samples may be available or not, we cant easily track which have been built previously and which haven't. The Sample Launcher deduces available samples by searching the current working directory for platform specific runtime libraries for a given sample. Available samples are then displayed in a list.

The code sample shows the launcher being run with hello_triangle. The runtime library is found as stated in the lookup log. The launcher then loads this sample and runs its sample_main.

After a little bit more work I ported Hello Triangle

Solved Problems

Compilation

Single samples can be compiled by using the sample__<sample_name> target. All samples can be compiled with vkb__samples. samples_launcher is only compiled when one or more samples are selected for compilation. A user should never need to manually compile samples_launcher.

As the framework is now split into components a sample and the launcher will only compile the targets needed. For instance, compiling the current samples yields sub second compile times. There is also no link step so serialized link times in our current builds will not occur in this version of the framework.

Samples

Samples no longer need to follow strict patterns for creation. Requirements for a sample are:

A file defining sample_main
vkb__register_sample - sample build target
vkb__register_sample_descriptor - sample metadata

From here samples can come in any shape or size. The most appropriate execution flow can be used for each sample. We may want to define a component which acts as a base sample to reduce boilerplate, but these building blocks do not need to be the same for all samples.

TODO

- condense launcher code
- add a default samples build test which makes sure the launcher can find all samples when building with vkb__samples

gpx1000 · 2022-07-19T06:31:24Z

bldsys/cmake/component_helpers.cmake

@@ -32,6 +32,8 @@ function(vkb__register_component)

        add_library("vkb__${TARGET_NAME}" STATIC ${TARGET_SRC})

+        set_property(TARGET "vkb__${TARGET_NAME}" PROPERTY POSITION_INDEPENDENT_CODE ON) # PIC required for shared lib linking


it is probably better to use set_compile_options("vkb_${TARGET_NAME}" PRIVATE POSITION_INDEPENDENT_CODE ON)
NB: prior to CMake 3.19, -fPIC didn't mean that pie is also by default mandated in cmake other than in Android builds. I think we're mandating a recent enough version for this to not be an issue. Just listing here as a just incase.

gpx1000 · 2022-07-19T06:42:48Z

bldsys/cmake/sample_helper.cmake

+        endif()
+
+        if(MSVC)
+            target_compile_options("sample__${TARGET_NAME}" PRIVATE /W4 /WX)


Dunno if you get any warnings in MSVC when compiling with this due to /W3 and /Md being set automatically D9025 might crop up. However, just in case it happens:
https://cmake.org/cmake/help/v3.15/policy/CMP0092.html
https://cmake.org/cmake/help/v3.15/policy/CMP0091.html
add:
if (POLICY CMP0091)
cmake_policy(SET CMP0091 NEW)
endif ()
if (POLICY CMP0092)
cmake_policy(SET CMP0092 NEW)
endif ()

* event bus * add AbstractChannel comment * response to feedback * add assert * assert * added observer expiration test

* fix folder structure in Visual Studio * add interface library support * review changes

* std filesystem more tests and fixes recursive tests more tests remove order specific checks from tests review comments * fix duplicated check * review changes

* event bus * response to feedback * add assert * assert * added observer expiration test * add pipelines * stages with custom fields * Pipeline -> EventPipeline * add comments

gpx1000 · 2022-11-14T20:21:49Z

bldsys/cmake/component_helpers.cmake

        message("ADDING STATIC: vkb__${TARGET_NAME}")

-        add_library("vkb__${TARGET_NAME}" STATIC ${TARGET_SRC})
+        add_library("vkb__${TARGET_NAME}" STATIC ${TARGET_SRC} ${TARGET_HEADERS})


think this is the same as in 490? Not sure of the advantage of adding headers explicitly to the cmake target.

gpx1000 · 2022-11-14T20:29:12Z

bldsys/cmake/sample_helper.cmake

-        SHADER_FILES_GLSL
-            ${TARGET_SHADER_FILES_GLSL})
+    get_property(DESCRIPTORS GLOBAL PROPERTY SAMPLE_DESCRIPTORS)
+    list(APPEND DESCRIPTORS "{\"id\":\"${TARGET_ID}\",\"name\":\"${TARGET_NAME}\", \"description\": \"${TARGET_DESCRIPTION}\", \"library_name\": \"${TARGET_LIB}\"}")


could potentially use jq or https://github.com/sbellus/json-cmake or some such to make this a usable thing at the cmake level. however, this works.

Does this allow for creating json structs in cmake? Or just parsing?

TomAtkinsonArm · 2022-11-24T16:11:54Z

@SaschaWillems @gary-sweet @asuessenbach can you please take a look at this PR when you get a chance? Looking for general feedback on how these components fit together and how samples can be written. The idea is to remove all the constraints on samples so that they can be more flexible in the future.

An example of this is sample_main which is all that is required to create a sample. Or EventPipeline which allows a sample to define its own lifecycle. "Frameworks" can be extracted into a series of default configurations for these components as most components are configurable or extendable

asuessenbach · 2022-11-30T16:12:41Z

First of all: the framework compiles without any issues with VS2022.
But when I start the samples_launcher, I get the following message:

loading sample configs
[vfs/std_filesystem.cpp:208] path does not exist: C:\git\Vulkan-Samples\build\VS2022_64\cmd\samples_launcher\build

And, indeed, that path does not exist. I found the file samples.json directly in the samples_launcher directory; there's no build directory there.
For a thorough review, I might need some more time. After all, this is pretty big.

asuessenbach · 2022-12-13T13:40:36Z

cmd/samples_launcher/include/config.hpp

+};
+
+// Load a config from a binary buffer
+components::StackErrorPtr load_config_from_json(const std::vector<uint8_t> &data, Config *config);


I think, we agreed to switch to std::exceptions here, instead of components::StackErrorPtr?
(Funnily, components::StackErrorPtr derives from std::exception, but is not used by the throw/catch mechanism of exceptions!)

asuessenbach · 2022-12-13T13:45:41Z

cmd/samples_launcher/src/main.cpp

+
+	std::cout << "loading sample configs\n";
+
+	std::vector<std::string> files;


Maybe rename files to config_files, or so?

asuessenbach · 2022-12-13T13:57:59Z

cmd/samples_launcher/src/main.cpp

+
+		std::cout << "looking for: " << library_name;
+
+		std::vector<std::string> candidates;


Do you expect more than one candidate per sample? But you just use the first one.
Maybe the search in the sample_files could then be done by std::find_if:

auto sample_it = std::find_if(sample_files.begin(), sample_files.end(), [&library_name](auto const &sample_file) { return vfs::helpers::get_file_name(sample_file) == library_name; }); if (sample_it == sample_files.end()) { std::cout << " - not found\n"; continue; }

asuessenbach · 2022-12-13T14:05:43Z

cmd/samples_launcher/src/main.cpp

+		return EXIT_SUCCESS;
+	}
+
+	for (auto &info : available_samples)


Maybe use std::find_if, again?

auto sample_it = std::find_if(available_samples.begin(), available_samples.end(), [&args](auto &sample_info) { return sample_info.sample.id == args[0]; }); if (sample_it == available_samples.end()) { std::cout << "Missing sample " << args[0] << "\n"; }

asuessenbach · 2022-12-13T14:34:28Z

components/encoding/include/components/encoding/json.hpp

+
+#define HANDLE_JSON_ERROR_START() \
+	try                           \
+	{


I would not hide scope opening in a macro. Functions like marshall_json below look somewhat awkward then.
Maybe just some HANDLE_JSON_ERROR representing the catch block would be enough?

asuessenbach · 2022-12-13T16:33:13Z

cmd/samples_launcher/include/config_marshalers.hpp

+}
+
+template <>
+class JsonMarshaler<config::Sample>


The termini Marshaler and UnMarshaler are a bit alien to me.
Maybe something like Serializer/Deserializer or Encoder/Decoder would be a bit more descriptive?

asuessenbach · 2022-12-13T17:38:05Z

components/events/src/input_manager.cpp

 		{
-			touch.position.x = event.pos_x - touch.position.x;
-			touch.position.y = event.pos_y - touch.position.y;
+			touch.position.x = event.x - touch.position.x;


What is this supposed to do?
It's unconditionally overwritten next.

asuessenbach · 2022-12-13T17:48:26Z

components/events/tests/event_bus.test.cpp

+
+	inline size_t unobserved_each_event_count() const
+	{
+		size_t unprocessed_events{0};


Maybe

return std::accumulate( m_each_callbacks.begin(), m_each_callbacks.end(), 0, []( size_t acc, auto const & callback ) { return acc + callback.second->queue_size(); } );

And similar for the other count functions.

asuessenbach · 2022-12-13T18:02:05Z

components/events/tests/event_pipeline.test.cpp

+	// on a second pass we expect only the "then" stages to be processed
+	execution_index = 2;
+
+	pipeline.process();


I don't see how the correct order is verified. Could you please elaborate a bit?

asuessenbach · 2022-12-13T18:05:18Z

components/events/tests/event_pipeline.test.cpp

+
+	REQUIRE(one_execution_count == 1);
+	REQUIRE(two_execution_count == 4);
+	REQUIRE(three_execution_count == 4);


I see, the number of calls of each event is checked, but how is the correct order verified?

asuessenbach

On Windows using VS, I had to set the Working Directory to the root of the repository in order to find everything. I don't see that mentioned anywhere.

asuessenbach · 2022-12-14T08:44:33Z

components/platform/include/components/platform/dl.hpp

+#	undef IMPORT
+#	define EXPORT __declspec(dllexport)
+#	define IMPORT __declspec(dllimport)
+#endif


Maybe better to read:

#if defined(_WIN32) # define EXPORT __declspec(dllexport) # define IMPORT __declspec(dllimport) #else # define EXPORT # define IMPORT #endif

asuessenbach · 2022-12-14T08:48:18Z

components/platform/include/components/platform/dl.hpp

+ * @param name library name
+ * @return std::string OS library name
+ */
+std::string os_library_name(const std::string &name);


Mixing functions taking a const std::string & with functions taking a const char * in the same header seems counter-intuitive. I would stay with one of them, keeping the same level of abstraction.

asuessenbach · 2022-12-14T08:56:59Z

components/platform/include/components/platform/platform.hpp

+	 */
+	struct PlatformContext
+	{
+		virtual std::vector<std::string> arguments() const = 0;


Why do you make this function pure virtual?
Probably all Context types just hold the arguments in a vector, that is it could be as well be moved into PlatformContext. Then, you don't even need that accessor function.

asuessenbach · 2022-12-14T09:01:16Z

components/platform/include/components/platform/platform.hpp

+	 *
+	 * @return int status code
+	 */
+	int platform_main(components::PlatformContext *);


Shouldn't platform_main take a const &, or maybe a const *?

asuessenbach · 2022-12-14T09:07:32Z

components/platform/src/dl.cpp

+	std::stringstream lib_name;
+	lib_name << os_library_prefix() << name << os_library_postfix();
+	return lib_name.str();
+}


No need to use a std::stringstream here. Simply
return os_library_prefix() + name + os_library_postfix();

asuessenbach · 2022-12-14T10:33:46Z

samples/hello_triangle/src/device.cpp

+
+	if (!validate_extensions(required_device_extensions, device_extensions))
+	{
+		throw std::runtime_error("Required device extensions are missing, will try without.");


What do you mean with "will try without" ?

asuessenbach · 2022-12-14T10:47:36Z

samples/hello_triangle/src/instance.cpp

+
+bool validate_extensions(const std::vector<const char *> &         required,
+                         const std::vector<VkExtensionProperties> &available)
+{


Maybe:

// inner find_if returns true if the extension was not found // outer find_if returns true if none of the extensions were not found, that is if all extensions were found return std::find_if(required.begin(), required.end(), [&available](auto required_extension) { return std::find_if(available.begin(), available.end(), [&required_extension](auto const &available_extension) { return strcmp(available_extension.extensionName, required_extension) == 0; }) == available.end(); }) == required.end();

asuessenbach · 2022-12-14T10:54:20Z

samples/hello_triangle/src/instance.cpp

+bool validate_layers(const std::vector<const char *> &     required,
+                     const std::vector<VkLayerProperties> &available)
+{
+	for (auto extension : required)


Maybe:

// inner find_if returns true if the layer was not found // outer find_if returns true if none of the layers was not found, that is if all layers were found return std::find_if(required.begin(), required.end(), [&available](auto required_layer) { return std::find_if(available.begin(), available.end(), [&required_layer](auto const &available_layer) { return strcmp(available_layer.layerName, required_layer) == 0; }) == available.end(); }) == required.end();

asuessenbach · 2022-12-14T10:58:10Z

samples/hello_triangle/src/instance.cpp

+{
+	LOGI("Initializing vulkan instance.");
+
+	if (volkInitialize())


Maybe:
if (volkInitialize() != VK_SUCCESS)

asuessenbach · 2022-12-14T11:11:49Z

samples/hello_triangle/src/swapchain.cpp

+		}
+
+		format.format = VK_FORMAT_UNDEFINED;
+		for (auto &candidate : formats)


Some std::find_if here?

TomAtkinsonArm added 7 commits July 8, 2022 12:32

event bus

79b4aca

add AbstractChannel comment

dfc9bc8

window interface

ac98922

headless window

d3c88f5

GLFW window

9b6b72e

add VKB_TEST_GLFW

eedc5b5

fix glfw errors

e9dd230

TomAtkinsonArm self-assigned this Jul 9, 2022

TomAtkinsonArm added the framework This is relevant to the framework label Jul 9, 2022

TomAtkinsonArm force-pushed the samples_launcher branch from 5184c1f to fa323b8 Compare July 9, 2022 17:32

TomAtkinsonArm changed the title ~~WIP: CLI Samples launcher~~ CLI Samples launcher Jul 9, 2022

TomAtkinsonArm requested review from SaschaWillems, asuessenbach, gary-sweet, gpx1000, JoseEmilio-ARM and wasimabbas-arm July 9, 2022 17:47

TomAtkinsonArm force-pushed the samples_launcher branch from fa323b8 to e385ce4 Compare July 9, 2022 21:51

TomAtkinsonArm changed the title ~~CLI Samples launcher~~ CLI Samples launcher + Vulkan Hello Triangle Jul 9, 2022

TomAtkinsonArm force-pushed the samples_launcher branch 2 times, most recently from 1a3f091 to 0311ace Compare July 10, 2022 23:15

add samples_launcher window improvements

0716f29

TomAtkinsonArm force-pushed the samples_launcher branch from 0311ace to 6f61eb1 Compare July 10, 2022 23:25

TomAtkinsonArm mentioned this pull request Jul 18, 2022

Event Bus #485

Merged

gpx1000 reviewed Jul 19, 2022

View reviewed changes

TomAtkinsonArm and others added 5 commits July 29, 2022 22:05

Event Bus (KhronosGroup#485)

ea72984

* event bus * add AbstractChannel comment * response to feedback * add assert * assert * added observer expiration test

Visual Studio Folders (KhronosGroup#496)

0ea815b

* fix folder structure in Visual Studio * add interface library support * review changes

STD Filesystem VFS (KhronosGroup#495)

c4415a1

* std filesystem more tests and fixes recursive tests more tests remove order specific checks from tests review comments * fix duplicated check * review changes

Event Pipelines (KhronosGroup#487)

8063599

* event bus * response to feedback * add assert * assert * added observer expiration test * add pipelines * stages with custom fields * Pipeline -> EventPipeline * add comments

GLFW window

6b1ec4e

TomAtkinsonArm added 21 commits November 14, 2022 14:45

CUSTOM_MAIN

a7bc144

test for correct context

4efe9fb

CMake WIN32

ba43c05

dynamic samples

b61794d

add more comments

bf2305d

use window.h

064508a

windows fix

31228be

fix macos lib name

78d102e

CUSTOM_MAIN

a33514e

main

3cafb23

launcher base

2f72500

config

f9f3aa0

add samples

866fd64

rebase fixes

60bcd34

deduce available samples

66ea368

hello triangle

7975b06

add sleep to prevent resource drain

11a0da8

windows build fixes

3b31fc9

partial fs fixes

f1f86e9

windows builds and runs hello_triangle

5ad8d09

fix unused param warnings

311b2b9

TomAtkinsonArm force-pushed the samples_launcher branch from 6637af6 to 311b2b9 Compare November 14, 2022 14:55

gpx1000 reviewed Nov 14, 2022

View reviewed changes

asuessenbach reviewed Dec 13, 2022

View reviewed changes

asuessenbach reviewed Dec 14, 2022

View reviewed changes

tomadamatkinson mentioned this pull request Feb 26, 2023

WIP: Vulkan Framework v2.0 #620

Closed

tomadamatkinson marked this pull request as draft May 22, 2023 22:56

tomadamatkinson closed this Jul 3, 2023

		@@ -32,6 +32,8 @@ function(vkb__register_component)

		add_library("vkb__${TARGET_NAME}" STATIC ${TARGET_SRC})

		set_property(TARGET "vkb__${TARGET_NAME}" PROPERTY POSITION_INDEPENDENT_CODE ON) # PIC required for shared lib linking


		std::cout << "loading sample configs\n";

		std::vector<std::string> files;


		std::cout << "looking for: " << library_name;

		std::vector<std::string> candidates;

CLI Samples launcher + Vulkan Hello Triangle #493

CLI Samples launcher + Vulkan Hello Triangle #493

Uh oh!

Conversation

TomAtkinsonArm commented Jul 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Solved Problems

Compilation

Samples

TODO

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomAtkinsonArm commented Nov 24, 2022

Uh oh!

asuessenbach commented Nov 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asuessenbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TomAtkinsonArm commented Jul 9, 2022 •

edited

Loading