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

Usability improvements for hello-vulkan sample #983

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kocdemir
Copy link

@kocdemir kocdemir commented Feb 26, 2024

Move class functions into a cpp file in a separate commit to improve readability for the project.

Remove redundant functions and calls
Report validation layer messages to logcat
Auto-disable validation layers when not found
Feature support checks to avoid validation failures
Rename some function and variables to fix linter and name shadowing warnings
Extra logging to help with debugging

Remove redundant functions and calls
Report validation layer messages to logcat
Auto-disable validation layers when not found
Feature support checks to avoid validation failures
Rename some function and variables to fix linter and name shadowing warnings
Extra logging to help with debugging
@kocdemir kocdemir changed the title Move member functions to a cpp file in hello-vulkan sample Usability improvements for hello-vulkan sample Feb 26, 2024
@kocdemir
Copy link
Author

@DanAlbert I've separated the previous PR into 2 commits in this one to ease the review. Thanks

@DanAlbert
Copy link
Member

It's probably going to be a while before I have the time to review this. I can't review and merge half of a PR. If the 7 things this PR was doing were 7 PRs, I'd probably be able to deal with one or two a day. If it's one PR it's going to have to wait until I have time to review the whole thing, and I don't even know Vulkan. It won't happen any time soon.

@kocdemir
Copy link
Author

Sure, this is not urgent and not changing the behavior much. Original owner of the sample, @GOOG-sergiu, can help with reviewing Vulkan related changes. Thank you!

Copy link
Contributor

@GOOG-sergiu GOOG-sergiu left a comment

Choose a reason for hiding this comment

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

LGTM

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.

5 participants