[Impeller] Allow selectively disabling Metal or Vulkan in the shader bundle template. #44551
Conversation
| if (defined(invoker.enable_vulkan)) { | ||
| enable_vulkan = invoker.enable_vulkan | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider asserting (enable_metal || enable_opengles || enable_vulkan)
There was a problem hiding this comment.
Not clear to me why the playground is getting included in the fuchsia builds. I need to look into this deeper.
There was a problem hiding this comment.
Not sure, but wondering if the deps here also need an impeller_supports_rendering guard:
https://github.com/flutter/engine/blob/main/BUILD.gn#L184
There was a problem hiding this comment.
I'm not sure I really understand what's goign on here, but maybe just (!impeller_supports_rendering || enable_metal || enable_opengles || enable_vulkan)?
bdero
force-pushed
the
bdero/impellerc-allow-disabling-metal-vulkan
branch
2 times, most recently
from
6ca6822 to
ad82c85
Compare
|
Closing. Filed flutter/flutter#132793 to address the pain with Fuchsia. |
|
This should pass now with #44925 |
bdero
force-pushed
the
bdero/impellerc-allow-disabling-metal-vulkan
branch
from
248244f to
0fcf9e9
Compare
|
@bdero - consider merging to head somehow, your force push appears to have backed out the commit that fixes the fuchsia failures. |
bdero
force-pushed
the
bdero/impellerc-allow-disabling-metal-vulkan
branch
from
0fcf9e9 to
2389cac
Compare
|
The commit I pushed was https://github.com/flutter/engine/commits/0fcf9e99bdcec70a6402399b04c629396a8d4a63, which includes your change and doesn't reverse it in a future diff. (I've since pushed again after rebasing again and removing an unrelated change) I'm wondering if the assert is just bad and this is just due to my poor understanding of GN templates. Like, if the template evaluation happens for all templates eagerly at import time rather than lazily at rule evaluation time, the assert isn't going to work. |
|
I think Github was showing me a stale diff at one point when I made that comment. Weird. Something else is causing the playground target to get included here. |
|
I think the takeaway from the sync was that we are not going to continue investigating why this was failing as we are no longer blocked. May I close this? |
|
Closing this as stale. |
4 participants
We already have an option for GL to avoid generating compute shaders for GL -- but for the special case of shaders that use GL textures, we don't need to build these for Vulkan or Metal.