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

Rework CMake validation layer options and add flags for best practices and sync validation #737

Merged

Conversation

SaschaWillems
Copy link
Collaborator

@SaschaWillems SaschaWillems commented Jul 1, 2023

Description

This PR adds two new CMake options related to validation layers:

  • VKB_VALIDATION_LAYERS_BEST_PRACTICES
    Enables the best practices layer.
  • VKB_VALIDATION_LAYERS_SYNCHRONIZATION
    Enables the synchronization validation layer.

These flags let developers easily toggle what part of the validation layers are active without having to e.g. use vkconfig from SDK.

Enabling one of these flags also implicitly enables validation layers. This was also retrofitted for the already existing VKB_VALIDATION_LAYERS_GPU_ASSISTED. So as soon as one of these flags is set, validation layers are enabled. This makes them less confusing to use.

General Checklist:

Please ensure the following points are checked:

  • My code follows the coding style
  • I have reviewed file licenses
  • I have commented any added functions (in line with Doxygen)
  • I have commented any code that could be hard to understand
  • My changes do not add any new compiler warnings
  • My changes do not add any new validation layer errors or warnings
  • I have used existing framework/helper functions where possible
  • My changes do not add any regressions
  • I have tested every sample to ensure everything runs correctly
  • This PR describes the scope and expected impact of the changes I am making

Note: The Samples CI runs a number of checks including:

  • I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)
  • My changes build on Windows, Linux, macOS and Android. Otherwise I have documented any exceptions

tomadamatkinson
tomadamatkinson previously approved these changes Jul 3, 2023
@JoseEmilio-ARM
Copy link
Collaborator

LGTM, but curious, how is this necessary now, #245 suggested these messages should already have been visible? Oh, it looks like it required the user enabling some settings externally, whereas this PR enables them programmatically 👍

JoseEmilio-ARM
JoseEmilio-ARM previously approved these changes Jul 4, 2023
Copy link
Collaborator

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

Small things to consider on this one. It's a good change and we need it.

framework/CMakeLists.txt Show resolved Hide resolved
framework/core/hpp_instance.cpp Show resolved Hide resolved
Copy link
Contributor

@asuessenbach asuessenbach left a comment

Choose a reason for hiding this comment

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

While introducing an option for the BEST_PRACTICES layer, we could also introduce an option for SYNCHRONIZATION_VALIDATION ?
Which would make implicit enabling of VALIDATION_LAYER even more important.

bldsys/cmake/global_options.cmake Outdated Show resolved Hide resolved
docs/build.md Outdated Show resolved Hide resolved
framework/core/hpp_instance.cpp Show resolved Hide resolved
@SaschaWillems
Copy link
Collaborator Author

I deliberately didn't change how and when the general layer flag is enabled and just expanded the way that the GPU assisted layer works.

But sure, I could integrate all of your feedback and rescope the PR. Will take a look at that.

@SaschaWillems SaschaWillems changed the title Add CMake option to enable best practices layer Rework CMake validation layer options and add flags for best practices and sync validation Jul 10, 2023
@marty-johnson59 marty-johnson59 merged commit 4fddec6 into KhronosGroup:main Jul 31, 2023
23 checks passed
@asuessenbach asuessenbach mentioned this pull request Aug 10, 2023
18 tasks
@SaschaWillems SaschaWillems deleted the best_practices_layers branch August 18, 2023 16:24
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