[impeller] drawAtlas #35009

jonahwilliams · 2022-07-29T19:22:28Z

Not implemented:

cull rect
Per-vert color + blending. Sort of approximated it by drawing texture if no color is present and doing dstIn (for one particular reason if color is present).

jonahwilliams · 2022-08-02T16:44:49Z

Still needs tests, and for some reason looks a little funky

flutter-dashboard · 2022-08-02T17:42:44Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

bdero

This is great! I spotted a possible issue with the transformed positions, but everything else is nits.

bdero · 2022-08-02T17:39:39Z

impeller/entity/contents/atlas_contents.h

+
+  std::shared_ptr<Texture> GetTexture() const;
+
+  void SetXForm(std::vector<Matrix> xform);


Here and everywhere else: Maybe "SetTransforms"? We've been using the term "transform" for any transformation matrices.

bdero · 2022-08-02T17:55:49Z

impeller/aiks/canvas.cc

+                       std::vector<Matrix> xform,
+                       std::vector<Rect> tex,
+                       std::vector<Color> colors,
+                       int count,


Looks like this count param is unused here.

(Although that might just be because it's WIP)

Removed, unneeded since I'm using std::vector here

bdero · 2022-08-02T18:12:37Z

impeller/entity/contents/atlas_contents.cc

+
+#include <iostream>
+#include <optional>
+#include "impeller/geometry/path_builder.h"


Nit: Add space between std and impeller headers.

bdero · 2022-08-02T18:13:00Z

impeller/entity/contents/atlas_contents.cc

+#include "impeller/renderer/formats.h"
+#include "impeller/renderer/sampler_library.h"
+#include "impeller/renderer/vertex_buffer_builder.h"
+#include "linear_gradient_contents.h"


Unintended import?

Cleaned up imports

bdero · 2022-08-02T18:29:43Z

impeller/entity/contents/atlas_contents.cc

+  VertexBufferBuilder<VS::PerVertexData> vertex_builder;
+  {
+    vertex_builder.Reserve(texture_coords_.size() * 6);
+    constexpr size_t indexes[6] = {0, 1, 2, 1, 2, 3};


Nit: indices

bdero · 2022-08-02T18:35:01Z

impeller/entity/contents/atlas_contents.cc

+      auto sample_rect = texture_coords_[i];
+      auto matrix = xform_[i];
+      auto color = (colors_.size() > 0 ? colors_[i] : Color::Black());
+      auto transformed_points = sample_rect.GetTransformedPoints(matrix);


Since transformed_points is used as the position of the draw rect, should this be something like Rect::MakeSize(sample_rect.size).GetTransformedPoints(matrix)? It seems like the transformed rect should have origin 0 rather than the position used for sampling the atlas.

Ahh this was it, thanks @bdero !

bdero · 2022-08-02T18:41:01Z

impeller/entity/contents/atlas_contents.h

+
+  std::shared_ptr<Texture> GetTexture() const;
+
+  void SetXForm(std::vector<Matrix> xform);


Nit: Here and everywhere else: Maybe call this SetTransforms? We've been using "Transform" for transformation matrices instead of Skia's "XForm" around Impeller.

…ncludes, rename xform

jonahwilliams · 2022-08-02T20:28:56Z

I've udpated this with some simple playgrounds. The colors example doesn't do anything yet since blend modes aren't implemented

bdero

LGTM, thanks!

bdero · 2022-08-02T21:51:35Z

impeller/entity/contents/atlas_contents.cc

+}
+
+std::optional<Rect> AtlasContents::GetCoverage(const Entity& entity) const {
+  Rect bounding_box = Rect::MakeLTRB(0, 0, 0, 0);


Nit: This can just be the default constructor.

bdero · 2022-08-02T21:57:17Z

impeller/display_list/display_list_dispatcher.cc

@@ -838,8 +874,10 @@ void DisplayListDispatcher::drawAtlas(const sk_sp<flutter::DlImage> atlas,
                                      flutter::DlImageSampling sampling,
                                      const SkRect* cull_rect,


I think we need to incorporate this cull rect at some point, but we can do this as follow-up work. It can probably be done by either surrounding DrawAtlas with DrawClip/Restore calls or doing clipping tricks while building the draw call + coverage itself.
May be worth inserting a TODO nearby.

I added a todo is aiks canvas.

jonahwilliams added 2 commits July 29, 2022 12:06

[impeller] drawAtlas

320cb32

everything broke lol

a3b31d6

chinmaygarde added the e: impeller label Aug 1, 2022

jonahwilliams added 2 commits August 1, 2022 18:02

Actually use atlas shader

4a69912

no colors

42792c6

jonahwilliams marked this pull request as ready for review August 2, 2022 16:44

jonahwilliams added 2 commits August 2, 2022 10:16

remove coverage computation for now

d29db5f

update licenses

7132185

flutter-dashboard bot added the needs tests label Aug 2, 2022

bdero reviewed Aug 2, 2022

View reviewed changes

implement coverage, fix transform calculation, remove count, remove i…

de84926

…ncludes, rename xform

bdero approved these changes Aug 2, 2022

View reviewed changes

bdero reviewed Aug 2, 2022

View reviewed changes

rename attributes use default ctr

0563130

jonahwilliams added autosubmit Merge PR when tree becomes green via auto submit App and removed needs tests labels Aug 2, 2022

auto-submit bot merged commit e7a37cc into flutter:main Aug 2, 2022

engine-flutter-autoroll mentioned this pull request Aug 3, 2022

Roll Flutter Engine from c07e1acfadda to a1e77ae885a0 (5 revisions) flutter/flutter#108850

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 3, 2022

e7a37cc [impeller] drawAtlas (flutter/engine#35009)

b96fef7

betrevisan pushed a commit to betrevisan/engine that referenced this pull request Aug 5, 2022

[impeller] drawAtlas (flutter#35009)

e291f87

emilyabest pushed a commit to emilyabest/engine that referenced this pull request Aug 12, 2022

[impeller] drawAtlas (flutter#35009)

fe9015a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[impeller] drawAtlas #35009

[impeller] drawAtlas #35009

jonahwilliams commented Jul 29, 2022 •

edited

Loading

jonahwilliams commented Aug 2, 2022

flutter-dashboard bot commented Aug 2, 2022

bdero left a comment

bdero Aug 2, 2022

jonahwilliams Aug 2, 2022

bdero Aug 2, 2022

bdero Aug 2, 2022

jonahwilliams Aug 2, 2022

bdero Aug 2, 2022

jonahwilliams Aug 2, 2022

bdero Aug 2, 2022 •

edited

Loading

jonahwilliams Aug 2, 2022

bdero Aug 2, 2022

jonahwilliams Aug 2, 2022

bdero Aug 2, 2022

jonahwilliams Aug 2, 2022

bdero Aug 2, 2022

jonahwilliams Aug 2, 2022

jonahwilliams commented Aug 2, 2022

bdero left a comment

bdero Aug 2, 2022

bdero Aug 2, 2022 •

edited

Loading

jonahwilliams Aug 2, 2022


		std::shared_ptr<Texture> GetTexture() const;

		void SetXForm(std::vector<Matrix> xform);

		@@ -838,8 +874,10 @@ void DisplayListDispatcher::drawAtlas(const sk_sp<flutter::DlImage> atlas,
		flutter::DlImageSampling sampling,
		const SkRect* cull_rect,

[impeller] drawAtlas #35009

[impeller] drawAtlas #35009

Conversation

jonahwilliams commented Jul 29, 2022 • edited Loading

jonahwilliams commented Aug 2, 2022

flutter-dashboard bot commented Aug 2, 2022

bdero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdero Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Aug 2, 2022

bdero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdero Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Jul 29, 2022 •

edited

Loading

bdero Aug 2, 2022 •

edited

Loading

bdero Aug 2, 2022 •

edited

Loading