[Impeller] move everything needed by the code gen template to core #40801
Conversation
There was a problem hiding this comment.
This file and impeller/core/vertex_buffer_builder.cc are basically empty, should they be removed?
There was a problem hiding this comment.
vertex_buffer_builder.cc was meant to be deleted.
But this follows the style of other TUs in this repository (particularyl in impeller)
| //------------------------------------------------------------------------------ | ||
| /// @brief An interface for binding resources. This is implemented by | ||
| /// |Command| and |ComputeCommand| to make CPU resources available | ||
| /// to the GPU. |
There was a problem hiding this comment.
It'd be more accurate to say it makes GPU resources (via handles) available to a given command's pipeline, since you can bind device private resources that never had a host buffer too (like we do with subpass render targets).
There was a problem hiding this comment.
Since this is causing friction for the Flutter-at-Google team I'm going to merge this and follow up with a doc fix.
| /// to the GPU. | ||
| /// | ||
| struct ResourceBinder { | ||
| virtual ~ResourceBinder() = default; |
There was a problem hiding this comment.
I'm not strongly opposed to adding this pure virtual interface, but it may cause some confusion/friction down the road for whoever ends up adding the next command type.
We're sort of lucky that the bindings match up between raster and compute commands right now, but if we add support for ray trace commands, the bindings won't perfectly match up anymore because we'll need to bind acceleration structures, and so we'll probably wind up needing to refactor anything that comes to rely on this polymorphism.
There was a problem hiding this comment.
I think in that case we'd make a RayTraceCommand not implement ResourceBinder and instead implement some other type from core.
There was a problem hiding this comment.
Alternatively, in that scenario we could make ResourceBinder not be pure virtual.
There was a problem hiding this comment.
I'm not sure I understand. What is the purpose of ResourceBinder? If it's not being used as a generic interface for commands, what is it useful for?
… core (flutter#40801)" (flutter#40811)" This reverts commit d34dddb.
…123833) flutter/engine@571c5de...c0e7cd5 2023-03-31 [email protected] [Impeller] iOS/macOS: Only wait for command scheduling prior to present (flutter/engine#40781) 2023-03-31 [email protected] Roll Skia from 9b2e538f1367 to f6c1eefd4600 (4 revisions) (flutter/engine#40807) 2023-03-30 [email protected] Revert "[Impeller] move everything needed by the code gen template to core (#40801)" (flutter/engine#40811) 2023-03-30 [email protected] [Impeller] Delete dead code from reflector.cc (flutter/engine#40805) 2023-03-30 [email protected] [Impeller] move everything needed by the code gen template to core (flutter/engine#40801) 2023-03-30 [email protected] `ui_web` library (flutter/engine#40608) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…lutter#123833) flutter/engine@571c5de...c0e7cd5 2023-03-31 [email protected] [Impeller] iOS/macOS: Only wait for command scheduling prior to present (flutter/engine#40781) 2023-03-31 [email protected] Roll Skia from 9b2e538f1367 to f6c1eefd4600 (4 revisions) (flutter/engine#40807) 2023-03-30 [email protected] Revert "[Impeller] move everything needed by the code gen template to core (flutter#40801)" (flutter/engine#40811) 2023-03-30 [email protected] [Impeller] Delete dead code from reflector.cc (flutter/engine#40805) 2023-03-30 [email protected] [Impeller] move everything needed by the code gen template to core (flutter/engine#40801) 2023-03-30 [email protected] `ui_web` library (flutter/engine#40608) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
4 participants
This makes it so the code_gen_template stuff doesn't have to refer to anything in
renderer, which Google's internal build system is complaining about even though GN is not (maybe because of thenognchecklines on those includes in the template).Moves the following from renderer to core:
SamplerandSamplerDescriptorCreates a new type in core called
ResourceBinderwhich is now implemented by bothComputeCommandandCommand.