Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Impeller] Add Rect::GetNormalizingTransform to handle UV coordinate conversion #47775

Merged

Conversation

flar
Copy link
Contributor

@flar flar commented Nov 8, 2023

Three places in the code were manually computing the UV coordinates relative to a texture coverage rectangle while also transforming the points. This change will make it easier both to compute the UV conversion matrix and also to consolidate it with the transform that was already being applied to streamline the total computations.

@flar
Copy link
Contributor Author

flar commented Nov 8, 2023

Unit tests are included for the new method which should prevent the test-bot from complaining, but reviewers should also assess whether the 4 places that use the new method are already adequately covered by existing rendering tests.

@bdero @jonahwilliams

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@flar
Copy link
Contributor Author

flar commented Nov 8, 2023

Unit tests are included for the new method which should prevent the test-bot from complaining, but reviewers should also assess whether the 4 places that use the new method are already adequately covered by existing rendering tests.

To answer my own question, I instrumented the 4 cases that use the new code and all but the first case in fill_path_geometry.cc is being executed in the golden tests:

% git grep "Using case"
impeller/entity/geometry/fill_path_geometry.cc:    FML_LOG(ERROR) << "Using case 1";
impeller/entity/geometry/fill_path_geometry.cc:        FML_LOG(ERROR) << "Using case 2";
impeller/entity/geometry/geometry.cc:  FML_LOG(ERROR) << "Using case 3";
impeller/entity/geometry/vertices_geometry.cc:    FML_LOG(ERROR) << "Using case 4";
% out/host_debug_unopt_arm64/impeller_golden_tests --working_dir=./imp_golden 2>&1 | grep "Using case" | sort | uniq -c
  10 [ERROR:flutter/impeller/entity/geometry/fill_path_geometry.cc(124)] Using case 2
  14 [ERROR:flutter/impeller/entity/geometry/geometry.cc(82)] Using case 3
   2 [ERROR:flutter/impeller/entity/geometry/vertices_geometry.cc(235)] Using case 4

@jonahwilliams double-checking your review still stands?

@flutter-dashboard
Copy link

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 #47775 at sha 23e60b7

@flar
Copy link
Contributor Author

flar commented Nov 8, 2023

The golden failures were due to getting the order of the matrix multiplies backwards which only caused a problem when the effect_transform was not Identity. In diagnosing that I discovered that we had no tests that covered cases 1 and 4 with an effect transform so I updated the aiks playground tests to make sure those paths were tested with an effect transform. The coverage when I last ran the tests looks complete in terms of the test cases hit:

% out/host_debug_unopt_arm64/impeller_golden_tests --working_dir=./imp_golden 2>&1 | grep "Using case" | sort | uniq -c
  10 [ERROR:flutter/impeller/entity/geometry/fill_path_geometry.cc(124)] Using case 2
  10 [ERROR:flutter/impeller/entity/geometry/fill_path_geometry.cc(96)] Using case 1
  14 [ERROR:flutter/impeller/entity/geometry/geometry.cc(82)] Using case 3
   4 [ERROR:flutter/impeller/entity/geometry/vertices_geometry.cc(235)] Using case 4

@flutter-dashboard
Copy link

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

Changes reported for pull request #47775 at sha 4e1604f

@jonahwilliams
Copy link
Member

Still LGTM

@flar flar added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 8, 2023
@auto-submit auto-submit bot merged commit b5a151f into flutter:main Nov 8, 2023
27 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 8, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 8, 2023
…138091)

flutter/engine@3e3be5e...117d47a

2023-11-08 skia-flutter-autoroll@skia.org Roll Skia from fce71a80b0a2 to a4cce5236dcf (1 revision) (flutter/engine#47807)
2023-11-08 chris@bracken.jp [macOS] Clean up resources in ViewController tests (flutter/engine#47792)
2023-11-08 skia-flutter-autoroll@skia.org Roll Skia from f3d250126ba9 to fce71a80b0a2 (1 revision) (flutter/engine#47796)
2023-11-08 skia-flutter-autoroll@skia.org Roll Skia from b4fa927468e6 to f3d250126ba9 (1 revision) (flutter/engine#47793)
2023-11-08 flar@google.com [Impeller] Add Rect::GetNormalizingTransform to handle UV coordinate conversion (flutter/engine#47775)
2023-11-08 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Remove Fuchsia Mac SDK from DEPS" (flutter/engine#47791)
2023-11-08 yjbanov@google.com [web] fix clicks on merged semantic nodes (attempt #2) (flutter/engine#47360)
2023-11-08 skia-flutter-autoroll@skia.org Roll Skia from 0f78e5f765d3 to b4fa927468e6 (1 revision) (flutter/engine#47788)
2023-11-08 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from VcFEJiUUTYwkhEAlJ... to sD8HRA4JmXczujkqO... (flutter/engine#47785)
2023-11-08 jiahaog@users.noreply.github.com Fix narrowing conversion lint (flutter/engine#47740)
2023-11-08 chris@bracken.jp [macOS] Bail out of tests if engine not running (flutter/engine#47771)
2023-11-08 skia-flutter-autoroll@skia.org Roll Skia from f91d39395e85 to 0f78e5f765d3 (1 revision) (flutter/engine#47776)
2023-11-08 chris@bracken.jp [testing] Extract StreamCapture test utility (flutter/engine#47774)
2023-11-08 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Promote fuchsia build v2 to prod." (flutter/engine#47779)
2023-11-08 aam@google.com Include updated locations for dart third_party components into license ignore-list. (flutter/engine#47770)
2023-11-07 chillers@google.com Remove Fuchsia Mac SDK from DEPS (flutter/engine#47700)
2023-11-07 skia-flutter-autoroll@skia.org Roll Skia from 030e21befbc9 to f91d39395e85 (6 revisions) (flutter/engine#47769)
2023-11-07 chinmaygarde@google.com [Impeller] Support static thread safety analysis with condition variables. (flutter/engine#47763)
2023-11-07 godofredoc@google.com Promote fuchsia build v2 to prod. (flutter/engine#47729)
2023-11-07 737941+loic-sharma@users.noreply.github.com [Windows] Reduce warnings produced by unit tests (flutter/engine#47724)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from VcFEJiUUTYwk to sD8HRA4JmXcz

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 bdero@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller will affect goldens
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants