Skip to content

Conversation

@SaschaWillems
Copy link
Collaborator

@SaschaWillems SaschaWillems commented Aug 6, 2025

As noted in #126 code style is all over the place, making things hard to read.

This PR adds a clang-format file (the same as used in our samples) and applies that to all code files.

It also adds a note on clang-format to the contribution guide.

Format compute shader chapter as an example
@asuessenbach
Copy link
Contributor

Besides lots of other settings I'm using with the Vulkan-Hpp project, something like
ColumnLimit : 160
helps very much in getting some nice formatting, preventing arbitrary long lines.

Just for reference, I've attached the clang-format from Vulkan-Hpp as a text file: clang-format.txt

@SaschaWillems
Copy link
Collaborator Author

As agreed on the last call: I'll apply the clang format to all files and update this PR.

@SaschaWillems SaschaWillems marked this pull request as ready for review August 17, 2025 17:10
@SaschaWillems
Copy link
Collaborator Author

No idea why the Android build fails.

@asuessenbach
Copy link
Contributor

No idea why the Android build fails.

clang_format has sorted the includes. Now, vulkan_android.h is included before vulkan_core.h in 34_android.cpp.

@SaschaWillems
Copy link
Collaborator Author

Oh, that was prob. stupid. Will redo this PR without reordering the includes :/

@asuessenbach
Copy link
Contributor

asuessenbach commented Aug 18, 2025

Oh, that was prob. stupid.

No, that was not stupid. It's vulkan, that is stupid.
I would prefer to just exclude those two #includes from reordering/formatting.

Or maybe just #include <vulkan/vulkan.h>, instead.

@SaschaWillems
Copy link
Collaborator Author

That would be the better option. Didn't realize that the tutorial did explicitly include the core/platform specific headers. That indeed does not make much sense and should be changed indeed. Also feels odd that every chapter explicitly includes "vk_platform.h".

Copy link
Contributor

@gpx1000 gpx1000 left a comment

Choose a reason for hiding this comment

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

looks like merge conflicts. Could you address?

@gpx1000
Copy link
Contributor

gpx1000 commented Aug 25, 2025

Probably a good idea to add it to CI in same way we do in Samples so it fails if maintainers forget ;-).

@SaschaWillems
Copy link
Collaborator Author

Having CI builds for all supported platforms would be very helpful indeed.

@gpx1000
Copy link
Contributor

gpx1000 commented Aug 25, 2025

Well we already have CI build for all platforms, this should just be the same copyright/clang check that Samples uses. https://github.com/KhronosGroup/Vulkan-Samples/blob/main/.github/workflows/check.yml take that and update pathing for here should just work. hopefully? Maybe add the ignore stuff as in what we use there.

# Conflicts:
#	attachments/07_image_views.cpp
#	attachments/08_graphics_pipeline.cpp
#	attachments/09_shader_modules.cpp
#	attachments/10_fixed_functions.cpp
#	attachments/12_graphics_pipeline_complete.cpp
#	attachments/14_command_buffers.cpp
#	attachments/15_hello_triangle.cpp
#	attachments/16_frames_in_flight.cpp
#	attachments/17_swap_chain_recreation.cpp
#	attachments/18_vertex_input.cpp
#	attachments/19_vertex_buffer.cpp
#	attachments/20_staging_buffer.cpp
#	attachments/21_index_buffer.cpp
#	attachments/22_descriptor_layout.cpp
#	attachments/23_descriptor_sets.cpp
#	attachments/24_texture_image.cpp
#	attachments/25_sampler.cpp
#	attachments/26_texture_mapping.cpp
#	attachments/27_depth_buffering.cpp
#	attachments/28_model_loading.cpp
#	attachments/29_mipmapping.cpp
#	attachments/30_multisampling.cpp
#	attachments/31_compute_shader.cpp
#	attachments/32_ecosystem_utilities.cpp
#	attachments/33_vulkan_profiles.cpp
#	attachments/34_android.cpp
#	attachments/35_gltf_ktx.cpp
#	attachments/36_multiple_objects.cpp
#	attachments/37_multithreading.cpp
# Conflicts:
#	attachments/07_image_views.cpp
#	attachments/08_graphics_pipeline.cpp
#	attachments/09_shader_modules.cpp
#	attachments/10_fixed_functions.cpp
#	attachments/12_graphics_pipeline_complete.cpp
#	attachments/14_command_buffers.cpp
#	attachments/15_hello_triangle.cpp
#	attachments/16_frames_in_flight.cpp
#	attachments/17_swap_chain_recreation.cpp
#	attachments/18_vertex_input.cpp
#	attachments/19_vertex_buffer.cpp
#	attachments/20_staging_buffer.cpp
#	attachments/21_index_buffer.cpp
#	attachments/22_descriptor_layout.cpp
#	attachments/23_descriptor_sets.cpp
#	attachments/24_texture_image.cpp
#	attachments/25_sampler.cpp
#	attachments/26_texture_mapping.cpp
#	attachments/27_depth_buffering.cpp
#	attachments/28_model_loading.cpp
#	attachments/29_mipmapping.cpp
#	attachments/30_multisampling.cpp
#	attachments/31_compute_shader.cpp
#	attachments/32_ecosystem_utilities.cpp
#	attachments/33_vulkan_profiles.cpp
#	attachments/34_android.cpp
#	attachments/35_gltf_ktx.cpp
#	attachments/36_multiple_objects.cpp
#	attachments/37_multithreading.cpp
Explicitly exclude some Android specific includes from ordering
@SaschaWillems
Copy link
Collaborator Author

I have re-enabled include ordering (and toggle it off for the few android includes we have) and have (re)applied Clang Format to all source files. Also added a note on ClangFormat to the contribution guide.

The earlier we can merge this, the easier things get, esp. for maintainers ;)

# Conflicts:
#	attachments/15_hello_triangle.cpp
#	attachments/16_frames_in_flight.cpp
#	attachments/17_swap_chain_recreation.cpp
#	attachments/18_vertex_input.cpp
#	attachments/19_vertex_buffer.cpp
#	attachments/20_staging_buffer.cpp
#	attachments/21_index_buffer.cpp
#	attachments/22_descriptor_layout.cpp
#	attachments/23_descriptor_sets.cpp
#	attachments/24_texture_image.cpp
#	attachments/25_sampler.cpp
#	attachments/26_texture_mapping.cpp
#	attachments/27_depth_buffering.cpp
#	attachments/28_model_loading.cpp
#	attachments/29_mipmapping.cpp
#	attachments/30_multisampling.cpp
#	attachments/31_compute_shader.cpp
#	attachments/32_ecosystem_utilities.cpp
#	attachments/33_vulkan_profiles.cpp
#	attachments/35_gltf_ktx.cpp
#	attachments/36_multiple_objects.cpp
#	attachments/37_multithreading.cpp
#	attachments/38_ray_tracing.cpp
@SaschaWillems
Copy link
Collaborator Author

@marty-johnson59 , @gpx1000 : I have fixed the merge conflicts and reapplied clang-format to all files. From my perspective, this is now ready to be merged as discussed on the last docs call.

@gpx1000 gpx1000 merged commit dfe3cba into main Nov 8, 2025
5 checks passed
@SaschaWillems
Copy link
Collaborator Author

Thx :)

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.

4 participants