Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@bdero
Copy link
Member

@bdero bdero commented Jul 22, 2022

Fixes flutter/flutter#105156.

In Impeller's Playground tests on MacOS, we import the impeller reflection headers that were generated for Metal, which have good binding locations. However, the GLES headers have all 0 for their binding locations. The result was that the binding map would only ever get populated with 1 item when using the binding utilities in the header. Ran into this in impeller-cmake on Windows.

Not sure what the best way to test this is... one option that comes to mind is checking the fields in the output metadata with nlohmann/json?

@bdero bdero requested review from chinmaygarde and zanderso July 22, 2022 09:12
@bdero bdero self-assigned this Jul 22, 2022
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@bdero bdero force-pushed the bdero/fix-gles-bindings branch from feae651 to fcb1c87 Compare July 22, 2022 09:59
@chinmaygarde
Copy link
Member

I've run into this before as well. And I think the best way to fix this is to eventually use spirv_cross::CompilerMSL everywhere we generate the reflection information. That is a subclass of CompilerGL and contains a superset of the information. That way, we can put this MSL specific information in a separate field and not have it collide with GL information. When multiple backends are enabled, have an umbrella include that just includes the first of the headers contains reflection information. Like so:

// In impeller/foo/bar.frag.h
#if IMPELLER_ENABLE_METAL
#include "impeller/foo/msl/bar.frag.h"
#elif IMPELLER_ENABLE_OPENGL
#include "impeller/foo/opengl/bar.frag.h"
...

This umbrella can be generated by the GN template as well. The reflected information is the same in all headers. So, it doesn't matter which one we pick.

I believe @iskakaushik is running into the same issue with Vulkan as well.

We didn't run into this earlier because we supported fewer platform-backend combinations. But I think we should solve this problem generically now to reduce friction from other backends.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

[Impeller] GLES backend: Binding is incorrect with multiple uniform slots are present

2 participants