Geometry Wars: Documentation Edition #49910

matanlurey · 2024-01-19T23:42:01Z

Document some confusing bits of geometry for the next bub.

Originally this PR was to add a test and fix a bug, but that's being handled here: #49938.

matanlurey · 2024-01-20T00:47:31Z

~~This is not really ready for review, but I need to mark it ready for review for Flutter gold to run.~~

Ready for review!

flutter-dashboard · 2024-01-20T01:20:21Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #49910 at sha 9941c87

flutter-dashboard · 2024-01-22T17:48:56Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #49910 at sha 064ceba

…ity-transform

matanlurey · 2024-01-22T23:40:10Z

There are no longer golden changes or tests in this PR, it's purely documentation. Thanks!

…ity-transform

flar

Looks fine, but with some fixes as mentioned in the comments.

flar · 2024-01-23T00:59:51Z

impeller/entity/geometry/geometry.cc

-      texture_coverage.GetNormalizingTransform() * effect_transform;
+  // Calculate UV-specific transform based on texture coverage and effect.
+  // For example, if the texture is 100x100 and the effect transform is
+  // scaling by 2, then the UV transform should be scaling by 0.5.


Shouldn't that be .02?

Can you clarify? I'm not sure how the reuslt would be 0.02.

In the example given of a 100x100 texture, texture_bounds.GetNormalizingTransform() will result in a Matrix that scales by 0.01. Then, if the effect_transform is Matrix::MakeScale(Vector2{2, 2}), the resulting uv_transform will have x and y basis vectors with scale 0.02.

uv_transform is used to squish local positions (source_rect) into a 0-1 space for texture sampling.

@bdero beat me to it

Thanks both!

flar · 2024-01-23T01:02:43Z

impeller/entity/geometry/geometry.h


+/// @brief Computes geometry and UV coordinates for a rectangle to be rendered.
+///
+/// UV is the horizontal and vertical axes within the texture.


I think you mean the horizontal and vertical coordinates within the texture.

An axis is a direction, not a position.

bdero

Couple of suggestions, but otherwise LGTM :)

bdero · 2024-01-23T02:52:40Z

impeller/entity/geometry/geometry.h

+/// @param entity           The entity to use for the transform.
+/// @param pass             The render pass to use for the transform.
 GeometryResult ComputeUVGeometryForRect(Rect source_rect,
                                        Rect texture_coverage,


Consider renaming to texture_bounds or texture_local_coverage. "Coverage" as a standalone word has a screen/pass space connotation around other parts of the Entities layer.

bdero · 2024-01-23T02:54:22Z

impeller/entity/geometry/geometry.h

+/// UV is the horizontal and vertical axes within the texture.
+///
+/// @param source_rect      The rectangle to be rendered.
+/// @param texture_coverage The size of the texture to be rendered.


A more accurate description for this would be something like: "The local space bounding box of the geometry."

jonahwilliams · 2024-01-23T16:37:49Z

impeller/entity/geometry/geometry.cc

+  // 2: position
+  // 3: UV
+  // etc.
  std::vector<Point> data(8);


bonus points: change this to an arrary so we don't do an extra heap allocation.

matanlurey

PTAL

matanlurey · 2024-01-23T18:24:12Z

impeller/entity/geometry/geometry.cc

-      texture_coverage.GetNormalizingTransform() * effect_transform;
+  // Calculate UV-specific transform based on texture coverage and effect.
+  // For example, if the texture is 100x100 and the effect transform is
+  // scaling by 2, then the UV transform should be scaling by 0.5.


Can you clarify? I'm not sure how the reuslt would be 0.02.

matanlurey · 2024-01-23T18:26:40Z

impeller/entity/geometry/geometry.cc

+  // 2: position
+  // 3: UV
+  // etc.
  std::vector<Point> data(8);


matanlurey · 2024-01-23T18:26:52Z

impeller/entity/geometry/geometry.h


+/// @brief Computes geometry and UV coordinates for a rectangle to be rendered.
+///
+/// UV is the horizontal and vertical axes within the texture.


matanlurey · 2024-01-23T18:27:03Z

impeller/entity/geometry/geometry.h

+/// UV is the horizontal and vertical axes within the texture.
+///
+/// @param source_rect      The rectangle to be rendered.
+/// @param texture_coverage The size of the texture to be rendered.


matanlurey · 2024-01-23T18:27:37Z

impeller/entity/geometry/geometry.h

+/// @param entity           The entity to use for the transform.
+/// @param pass             The render pass to use for the transform.
 GeometryResult ComputeUVGeometryForRect(Rect source_rect,
                                        Rect texture_coverage,


…ity-transform

ditman · 2024-01-23T22:05:55Z

Just wanted to compliment the reference in this PR's title.

KUDOS! 🚗💨

flar · 2024-01-23T22:37:59Z

impeller/entity/geometry/geometry.cc

-  std::vector<Point> data(8);
+  // Calculate UV-specific transform based on texture coverage and effect.
+  // For example, if the texture is 100x100 and the effect transform is
+  // scaling by 0.2, texture_bounds.GetNormalizingTransform() will result in a


This should be 2.0?

…142079) flutter/engine@0b9b538...220416c 2024-01-23 [email protected] Geometry Wars: Documentation Edition (flutter/engine#49910) 2024-01-23 [email protected] Exclude the Dart SDK sdk/lib/svg/dart2js directory from the license crawl (flutter/engine#49977) 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

Follow up to #49910 (review).

Add a unit test showing a bug in entity transform handling.

9b58929

github-actions bot added the e: impeller label Jan 19, 2024

matanlurey added 2 commits January 19, 2024 16:37

Add some comments for myself.

64fccce

Add hack to run on CI.

9941c87

matanlurey marked this pull request as ready for review January 20, 2024 00:47

flutter-dashboard bot added the will affect goldens label Jan 20, 2024

matanlurey added 3 commits January 22, 2024 09:54

Merge remote-tracking branch 'upstream/main' into engine-impeller-ent…

f222a49

…ity-transform

Merge remote-tracking branch 'upstream/main' into engine-impeller-ent…

5ef8e3f

…ity-transform

Revert changes and tests, comments only.

b4503be

matanlurey force-pushed the engine-impeller-entity-transform branch from 064ceba to b4503be Compare January 22, 2024 23:37

matanlurey changed the title ~~WIP: Entity transform bug in Impeller~~ Geometry Wars: Documentation Edition Jan 22, 2024

matanlurey requested review from bdero and flar January 22, 2024 23:39

matanlurey added 4 commits January 22, 2024 16:50

Clobbered.

34ae228

++

9b8384b

Merge remote-tracking branch 'upstream/main' into engine-impeller-ent…

926c597

…ity-transform

Merge better.

b159cd5

flar approved these changes Jan 23, 2024

View reviewed changes

bdero approved these changes Jan 23, 2024

View reviewed changes

jonahwilliams reviewed Jan 23, 2024

View reviewed changes

++

28db5c9

matanlurey requested a review from flar January 23, 2024 18:28

matanlurey commented Jan 23, 2024

View reviewed changes

matanlurey added 2 commits January 23, 2024 13:17

Merge remote-tracking branch 'upstream/main' into engine-impeller-ent…

8da559d

…ity-transform

++

3c85ddc

matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 23, 2024

auto-submit bot merged commit 220416c into flutter:main Jan 23, 2024

flar reviewed Jan 23, 2024

View reviewed changes

matanlurey deleted the engine-impeller-entity-transform branch January 23, 2024 22:47

engine-flutter-autoroll mentioned this pull request Jan 23, 2024

Roll Flutter Engine from 0b9b538bd458 to 220416cfa6ca (2 revisions) flutter/flutter#142079

Merged

matanlurey mentioned this pull request Jan 23, 2024

Fix a typo in geometry.cc#ComputeUVGeometryForRect. #49986

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 23, 2024

220416cfa Geometry Wars: Documentation Edition (flutter/engine#49910)

59d7d82

auto-submit bot pushed a commit that referenced this pull request Jan 24, 2024

Fix a typo in geometry.cc#ComputeUVGeometryForRect. (#49986)

7c592db

Follow up to #49910 (review).

Geometry Wars: Documentation Edition #49910

Geometry Wars: Documentation Edition #49910

Uh oh!

Conversation

matanlurey commented Jan 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matanlurey commented Jan 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Jan 20, 2024

Uh oh!

flutter-dashboard bot commented Jan 22, 2024

Uh oh!

matanlurey commented Jan 22, 2024

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdero Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdero Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matanlurey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ditman commented Jan 23, 2024

KUDOS! 🚗💨

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

matanlurey commented Jan 19, 2024 •

edited

Loading

matanlurey commented Jan 20, 2024 •

edited

Loading

bdero Jan 23, 2024 •

edited

Loading

bdero Jan 23, 2024 •

edited

Loading