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

Enable Vulkan validation layers for shell_test #17684

Merged
merged 5 commits into from Apr 17, 2020

Conversation

gw280
Copy link
Contributor

@gw280 gw280 commented Apr 13, 2020

I'm not 100% clear on what the implied relationship between IsDebuggingEnabled() and ValidationLayersEnabled() should be here, but I've made validation layers strictly imply that debugging is enabled.

Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

I'm not 100% clear on what the implied relationship between IsDebuggingEnabled() and ValidationLayersEnabled() should be here..

Yeah. There seems to be no point to IsDebuggingEnabled now. Let's just get rid of it.

@@ -10,16 +10,24 @@

namespace vulkan {

static bool sValidationLayersEnabled = false;
Copy link
Member

Choose a reason for hiding this comment

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

Let's stay away from static variables. This makes it hard to perform validation on a per Flutter application level basis. This flag can be store in VulkanApplication instead and queried when the layers are being setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@gw280 gw280 force-pushed the gwright-enable-validation branch from 6d9909d to 92307ea Compare April 16, 2020 23:40
@gw280
Copy link
Contributor Author

gw280 commented Apr 16, 2020

I'm not 100% clear on what the implied relationship between IsDebuggingEnabled() and ValidationLayersEnabled() should be here..

Yeah. There seems to be no point to IsDebuggingEnabled now. Let's just get rid of it.

Done

@gw280 gw280 requested a review from chinmaygarde April 17, 2020 00:00
Copy link
Contributor

@iskakaushik iskakaushik left a comment

Choose a reason for hiding this comment

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

LGTM

@gw280 gw280 added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 17, 2020
@fluttergithubbot fluttergithubbot merged commit 9f31d81 into flutter:master Apr 17, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
5 participants