[Impeller] make color source a variant instead of a closure. #51853
Conversation
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
LGTM modulo moving the visitor to the cc file and naming it apropos what it does, not what it is. Thanks.
impeller/aiks/color_source.h
Outdated
| RuntimeEffectData, | ||
| std::monostate>; | ||
|
|
||
| struct ColorSourceDataVisitor { |
There was a problem hiding this comment.
This should be moved into the .cc file in an anonymous namespace.
|
|
||
| struct Paint; | ||
|
|
||
| struct LinearGradientData { |
There was a problem hiding this comment.
If we had Clone() calls for these contents, you wouldn't have to duplicate the data they need in structs. So the variant would just be std::variant<LinearGradientContents, RadialGradientContents, ...>.
GetContents would then just call Clone(), but storing it in a variant would allow you to visit the actual type.
Just a suggestion, feel free to punt since this is good.
There was a problem hiding this comment.
Oh, so rather than have the data at all, just make color source contents cloneable. That makes sense but would also require more refactors, since we store this on paint and do a lot of type based dispatching.
There was a problem hiding this comment.
Though we could also change ColorSourceContents to remove the inheritance and change its behavior based on the variant its given.
impeller/aiks/color_source.h
Outdated
| RuntimeEffectData, | ||
| std::monostate>; | ||
|
|
||
| struct ColorSourceDataVisitor { |
There was a problem hiding this comment.
| struct ColorSourceDataVisitor { | |
| struct CreateContentsVisitor { |
Since that's what it does.
jonahwilliams
added
the
autosubmit
…146180) flutter/engine@0da1b2e...5fc83bc 2024-04-03 [email protected] [Impeller] make color source a variant instead of a closure. (flutter/engine#51853) 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],[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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
2 participants
This would also me to create a type safe visitor to pull out the data required for #51778