-
Notifications
You must be signed in to change notification settings - Fork 6k
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
[Impeller] partially remove remap sampler support #39147
Conversation
I could also fixup the location data in the generated IPLR file but I'm unusure if we're using it for anything else. |
@@ -210,6 +207,36 @@ bool RuntimeEffectContents::Render(const ContentContext& renderer, | |||
} | |||
} | |||
|
|||
size_t sampler_index = 0; | |||
FML_DCHECK(minimum_sampler_index >= 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't this always be true? Given that the type is size_t
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RIGHT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
auto label is removed for flutter/engine, pr: 39147, due to - The status or check suite Linux Android AOT Engine has failed. Please fix the issues identified (or deflake) before re-applying this label. |
@@ -332,34 +332,6 @@ void main() async { | |||
shader.dispose(); | |||
}); | |||
|
|||
// This test can't rely on actual pixels rendered since it needs to run on a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the test on line 275 sufficient? Does that test need to have a sampler or two thrown in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't apply either way, its not running on impeller or metal?
…119279) * 99270b81c Roll Fuchsia Mac SDK from _H53AyDxR9Pm2TbwN... to KLzrg8eFD9m5J9xa-... (flutter/engine#39155) * 9c20d2afb Roll Fuchsia Linux SDK from pGX7tanT1okL8XCg-... to ggI4a2jNo5aj6Rp6w... (flutter/engine#39157) * 664e7d3ae Roll Skia from 3d0e2ad41911 to 0e1cb7bc43dc (1 revision) (flutter/engine#39156) * ca38bb2e6 Roll Skia from 0e1cb7bc43dc to 5e363a69efbe (3 revisions) (flutter/engine#39158) * ce3bfa124 Roll Dart SDK from f61211592196 to 160774c04493 (2 revisions) (flutter/engine#39159) * 36cb5c8ce Roll Dart SDK from 160774c04493 to 5a8b34f040f5 (1 revision) (flutter/engine#39160) * ba188d7ca Update infrastructure python code to be compatible with python 2 and python 3 (flutter/engine#39133) * 8baff3e90 Roll Skia from 5e363a69efbe to ad7955820dc6 (2 revisions) (flutter/engine#39161) * ed9fc3610 Roll Skia from ad7955820dc6 to e9ac244d61b2 (6 revisions) (flutter/engine#39165) * cb6df8585 Roll Dart SDK from 5a8b34f040f5 to 4a8615b7e3ed (1 revision) (flutter/engine#39167) * d85ec02b0 [fuchsia] Migrate to new RealmBuilder API (flutter/engine#39163) * 939012600 [Impeller] partially remove remap sampler support (flutter/engine#39147)
Part of flutter/flutter#119173. The runtime IPLR contains all of the infromation to handle samplers without remaping. Its just a bit awkward to use at this point.
As a follow up will make location data take into account this offset and delete the flag (after rolling into tool).
Separately, we should investigate if enabling vulkan regressed any other functionality on macOS.