[Flutter GPU] Texture binding, index binding, attachments, depth state.#48386
[Flutter GPU] Texture binding, index binding, attachments, depth state.#48386bdero merged 10 commits intoflutter:mainfrom
Conversation
jonahwilliams
left a comment
There was a problem hiding this comment.
Some nits on the Dart API
Overall I dislike the pattern of validating in the constructor of an Object via thrown exception. Not sure what the best alternative would be.
lib/gpu/fixtures.h
Outdated
| extern unsigned char kFlutterGPUTextureVertIPLR[]; | ||
|
|
||
| constexpr unsigned int kFlutterGPUTextureFragIPLRLength = 800; | ||
| extern unsigned char kFlutterGPUTextureFragIPLR[]; No newline at end of file |
There was a problem hiding this comment.
| extern unsigned char kFlutterGPUTextureFragIPLR[]; | |
| extern unsigned char kFlutterGPUTextureFragIPLR[]; | |
lib/gpu/lib/src/render_pass.dart
Outdated
|
|
||
| base class RenderTarget { | ||
| RenderTarget( | ||
| {List<ColorAttachment>? colorAttachments, this.depthStencilAttachment}) { |
There was a problem hiding this comment.
| {List<ColorAttachment>? colorAttachments, this.depthStencilAttachment}) { | |
| {List<ColorAttachment> colorAttachments = const <ColorAttachment>[], this.depthStencilAttachment}) { |
lib/gpu/lib/src/render_pass.dart
Outdated
| if (colorAttachments != null) { | ||
| colorAttachments = colorAttachments; | ||
| return; | ||
| } | ||
| colorAttachments = <ColorAttachment>[]; |
There was a problem hiding this comment.
| if (colorAttachments != null) { | |
| colorAttachments = colorAttachments; | |
| return; | |
| } | |
| colorAttachments = <ColorAttachment>[]; |
lib/gpu/lib/src/render_pass.dart
Outdated
| late List<ColorAttachment> colorAttachments; | ||
| DepthStencilAttachment? depthStencilAttachment; |
There was a problem hiding this comment.
Seems like these should be final and the overall class const
There was a problem hiding this comment.
Done. I converted RenderTarget.singleColor into a non-const factory. I think that's necessary, but let me know if I'm missing some const magic.
lib/gpu/lib/src/render_pass.dart
Outdated
| return new RenderTarget( | ||
| colorAttachments: colors, | ||
| depthStencilAttachment: depthStencilAttachment); | ||
| } |
There was a problem hiding this comment.
With Dart's limited const expression support you won't be able to make this const, but it doesn't need to be a factory either.
It seems odd that the colorAttachment argument to this object is nullable?
| return new RenderTarget( | |
| colorAttachments: colors, | |
| depthStencilAttachment: depthStencilAttachment); | |
| } | |
| return RenderTarget( | |
| colorAttachments: [ | |
| if (colorAttachment != null) | |
| colorAttachment | |
| ], | |
| depthStencilAttachment: depthStencilAttachment); | |
| } |
There was a problem hiding this comment.
Hmm yeah I guess there's no point in making this nullable. Switched back to constructor.
lib/gpu/lib/src/render_pass.dart
Outdated
| List<ColorAttachment> colors = []; | ||
| if (colorAttachment != null) { | ||
| colors.add(colorAttachment); | ||
| } |
There was a problem hiding this comment.
| List<ColorAttachment> colors = []; | |
| if (colorAttachment != null) { | |
| colors.add(colorAttachment); | |
| } |
4c21321 to
9de4662
Compare
9de4662 to
e831520
Compare
flutter/engine@ebebb25...292a921 2023-11-27 [email protected] [Flutter GPU] Texture binding, index binding, attachments, depth state. (flutter/engine#48386) 2023-11-27 [email protected] [Impeller] use spec constant for decal support in morph filter. (flutter/engine#48288) 2023-11-27 [email protected] [Impeller] OES extension does not apply to regular textures for decal support (flutter/engine#48388) 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
A pattern that I like better (but don't necessarily love) is to have a factory constructor return an "invalid" instance that stashes a description of the error state somewhere when something goes wrong during construction. Sort of like this: https://github.com/flutter/engine/blob/main/tools/pkg/engine_build_configs/lib/src/build_config.dart#L92. Then the caller can decide whether to throw an exception after checking |
|
I think it would be more reasonable to throw expceptions than to use an isValid bit. My problem with the valid bit is that it is easy to ignore/forget, and then the developer only gets the error when the invalid object is passed to a different method instead of at the original invalidation. I would also investigate how WebGPU does validation for render target/attachment descriptions. |
…e. (flutter#48386) Now rendering textured 3D models! * Combined depth+stencil attachment. * Allow for multiple color attachments. * Add blend mode configuration. * Fix uniform ordering for vertex shaders. * Texture binding and sampling options. * Index buffer binding. * Depth configuration.
…9037) flutter/engine@ebebb25...292a921 2023-11-27 [email protected] [Flutter GPU] Texture binding, index binding, attachments, depth state. (flutter/engine#48386) 2023-11-27 [email protected] [Impeller] use spec constant for decal support in morph filter. (flutter/engine#48288) 2023-11-27 [email protected] [Impeller] OES extension does not apply to regular textures for decal support (flutter/engine#48388) 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
|
This is part of the Flutter GPU MVP umbrella bug: flutter/flutter#130921 |
Now rendering textured 3D models!
Screen.Recording.2023-11-26.at.9.17.51.AM.mov
TextureVertex
TextureFragment