-
Notifications
You must be signed in to change notification settings - Fork 6k
[Flutter GPU] Texture binding, index binding, attachments, depth state. #48386
Conversation
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.
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[]; |
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.
extern unsigned char kFlutterGPUTextureFragIPLR[]; | |
extern unsigned char kFlutterGPUTextureFragIPLR[]; | |
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.
Done.
lib/gpu/lib/src/render_pass.dart
Outdated
|
||
base class RenderTarget { | ||
RenderTarget( | ||
{List<ColorAttachment>? colorAttachments, this.depthStencilAttachment}) { |
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.
{List<ColorAttachment>? colorAttachments, this.depthStencilAttachment}) { | |
{List<ColorAttachment> colorAttachments = const <ColorAttachment>[], this.depthStencilAttachment}) { |
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.
Done. Nice one!
lib/gpu/lib/src/render_pass.dart
Outdated
if (colorAttachments != null) { | ||
colorAttachments = colorAttachments; | ||
return; | ||
} | ||
colorAttachments = <ColorAttachment>[]; |
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.
if (colorAttachments != null) { | |
colorAttachments = colorAttachments; | |
return; | |
} | |
colorAttachments = <ColorAttachment>[]; |
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.
Done.
lib/gpu/lib/src/render_pass.dart
Outdated
late List<ColorAttachment> colorAttachments; | ||
DepthStencilAttachment? depthStencilAttachment; |
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.
Seems like these should be final and the overall class const
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List<ColorAttachment> colors = []; | |
if (colorAttachment != null) { | |
colors.add(colorAttachment); | |
} |
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.
Done.
4c21321
to
9de4662
Compare
9de4662
to
e831520
Compare
…139037) flutter/engine@ebebb25...292a921 2023-11-27 bdero@google.com [Flutter GPU] Texture binding, index binding, attachments, depth state. (flutter/engine#48386) 2023-11-27 jonahwilliams@google.com [Impeller] use spec constant for decal support in morph filter. (flutter/engine#48288) 2023-11-27 jonahwilliams@google.com [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 jacksongardner@google.com,rmistry@google.com,zra@google.com 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.
…lutter#139037) flutter/engine@ebebb25...292a921 2023-11-27 bdero@google.com [Flutter GPU] Texture binding, index binding, attachments, depth state. (flutter/engine#48386) 2023-11-27 jonahwilliams@google.com [Impeller] use spec constant for decal support in morph filter. (flutter/engine#48288) 2023-11-27 jonahwilliams@google.com [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 jacksongardner@google.com,rmistry@google.com,zra@google.com 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