Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Fixes for asymmetric stencil descriptors #43535

Merged
merged 1 commit into from
Jul 12, 2023
Merged

[Impeller] Fixes for asymmetric stencil descriptors #43535

merged 1 commit into from
Jul 12, 2023

Conversation

johnmccutchan
Copy link
Contributor

@johnmccutchan johnmccutchan commented Jul 10, 2023 •
edited
Loading

Fixes for asymmetric stencil descriptors

  • Fix GLES implementation to properly set back and front face options in the case that they aren't identical.
  • Add write_mask to hash function.

flutter/flutter#129855

@bdero
Copy link
Member

bdero commented Jul 10, 2023

Interface LGTM. 👍

@bdero bdero changed the title Interface changes for asymmetric stencil specification [Impeller] Interface changes for asymmetric stencil specification Jul 10, 2023
@bdero bdero changed the title [Impeller] Interface changes for asymmetric stencil specification [Impeller] Interface changes for asymmetric stencil specification. Jul 10, 2023
@chinmaygarde chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label Jul 11, 2023
@johnmccutchan johnmccutchan changed the title [Impeller] Interface changes for asymmetric stencil specification. [Impeller] Fixes for asymmetric stencil descriptors Jul 11, 2023
@flutter-dashboard
Copy link

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
bdero approved these changes Jul 12, 2023
View reviewed changes
Copy link
Member

@bdero bdero left a comment

Choose a reason for hiding this comment

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

LGTM, just one suggestion

impeller/renderer/renderer_unittests.cc
ImGui::SliderInt("Stencil Write Value", &stencil_reference_write, 0,
0xFF);
ImGui::SliderInt("Stencil Compare Value", &stencil_reference_read, 0,
0xFF);
Copy link
Member

@bdero bdero Jul 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: It would be nice if the front/back face stencil behavior could be checked separately in this tool. One way would be to:

  1. set the stencil_compare to kLessEqual, and then
  2. add an x_mirror bool to the controls that, when multiplies the mvp by Matrix::MakeScale(Vector2(-1, -1))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@bdero bdero Jul 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Whoops, meant to say invert just x with Matrix::MakeScale(Vector2(-1, 1)) and then the image would flip horizontally + the checkerboard pattern should invert.

Copy link
Member

Choose a reason for hiding this comment

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

And keep the front face as kGreater, but just change the back face to kLessEqual.

@johnmccutchan
Fix asymmetric stencil setting in GLES backend
3de072c 
- Small fix for GLES backend.
- Add unit test for stencil masking.
@johnmccutchan johnmccutchan merged commit e5e78b8 into flutter:main Jul 12, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 12, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 12, 2023
@engine-flutter-autoroll
e5e78b897 [Impeller] Fixes for asymmetric stencil descriptors (flutte…
cc9ea50 
…r/engine#43535)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 12, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 12, 2023
@engine-flutter-autoroll
e5e78b897 [Impeller] Fixes for asymmetric stencil descriptors (flutte…
cb06653 
…r/engine#43535)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 13, 2023
@engine-flutter-autoroll
Roll Flutter Engine from 16e2ab7e986c to 1b1ccdd1f527 (13 revisions) (#…
47ba59c 
…130458)

flutter/engine@16e2ab7...1b1ccdd

2023-07-12 [email protected] Roll Skia from 4e989b1564ee to bedc92598644 (1 revision) (flutter/engine#43617)
2023-07-12 [email protected] move rtree and canvas_spy sources to Fuchsia sub-directory (flutter/engine#43615)
2023-07-12 [email protected] Roll Skia from 68e3c0b3eea7 to 4e989b1564ee (6 revisions) (flutter/engine#43614)
2023-07-12 [email protected] Document (and assert) that channel names can't contains nulls (flutter/engine#43593)
2023-07-12 [email protected] Reland "Refactor JSNumber.toDart and Object.toJS" (flutter/engine#43363)
2023-07-12 [email protected] Add a flag to `ParagraphBuilder` for rounding hack migration (flutter/engine#43118)
2023-07-12 [email protected] [Impeller] Fixes for asymmetric stencil descriptors (flutter/engine#43535)
2023-07-12 [email protected] Roll Skia from adeeb8d50f7c to 68e3c0b3eea7 (1 revision) (flutter/engine#43609)
2023-07-12 [email protected] Add comment to use of 3x3 mapRect in TransformLayer (flutter/engine#43608)
2023-07-12 [email protected] Roll Skia from a251a36ea519 to adeeb8d50f7c (1 revision) (flutter/engine#43606)
2023-07-12 [email protected] Roll Dart SDK from b95f6531c726 to 8f8f281ccdc6 (2 revisions) (flutter/engine#43607)
2023-07-12 [email protected] Roll Skia from ebc149cff431 to a251a36ea519 (2 revisions) (flutter/engine#43604)
2023-07-12 [email protected] [Impeller] Use new SkParagraph APIs for stroked text. (flutter/engine#41735)

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] 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 13, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 13, 2023
@engine-flutter-autoroll
e5e78b897 [Impeller] Fixes for asymmetric stencil descriptors (flutte…
8ce1f8a 
…r/engine#43535)
fluttermirroringbot pushed a commit to flutter/flutter that referenced this pull request Jul 13, 2023
@engine-flutter-autoroll
Roll Flutter Engine from 16e2ab7e986c to b4080361f2f9 (28 revisions) (#…
ee4208f 
…130496)

Roll Flutter Engine from 16e2ab7e986c to b4080361f2f9 (28 revisions)

flutter/engine@16e2ab7...b408036

2023-07-13 [email protected] Roll Skia from e5ec341bc3ca to 56b68ce6196c (1 revision) (flutter/engine#43633)
2023-07-13 [email protected] Roll Dart SDK from f499e91e8cb2 to ade4dae923f3 (1 revision) (flutter/engine#43632)
2023-07-13 [email protected] Roll Skia from 811b046c673b to e5ec341bc3ca (1 revision) (flutter/engine#43631)
2023-07-13 [email protected] Make GOMA state automatic by default (flutter/engine#43584)
2023-07-13 [email protected] Roll Skia from c8da0c657c4e to 811b046c673b (3 revisions) (flutter/engine#43630)
2023-07-13 [email protected] [Impeller] Remove unactionable error logs and use structure chains for instance creation. (flutter/engine#43629)
2023-07-13 [email protected] Roll Skia from 7f391ea9164e to c8da0c657c4e (1 revision) (flutter/engine#43628)
2023-07-13 [email protected] [Impeller] Add RAII wrappers for VMA objects. (flutter/engine#43626)
2023-07-13 [email protected] Roll Fuchsia Linux SDK from 1STsUj0X5YgpiSNEb... to xBJq6PsO5ebblODMe... (flutter/engine#43627)
2023-07-13 [email protected] Roll Skia from 6ed93436d57c to 7f391ea9164e (1 revision) (flutter/engine#43625)
2023-07-13 [email protected] Roll Dart SDK from 8f8f281ccdc6 to f499e91e8cb2 (3 revisions) (flutter/engine#43623)
2023-07-13 [email protected] Roll Fuchsia Mac SDK from 0fvk838jTDNQ_l43k... to 3C7P0w8ySmtqpyi3S... (flutter/engine#43622)
2023-07-13 [email protected] Roll Skia from bedc92598644 to 6ed93436d57c (3 revisions) (flutter/engine#43621)
2023-07-13 [email protected] [Impeller] Add support to embedder for Impeller on GL (via Angle on Windows). (flutter/engine#43388)
2023-07-12 [email protected] [Impeller] Allocate buffers out of a pool on the raster thread. (flutter/engine#43564)
2023-07-12 [email protected] Roll Skia from 4e989b1564ee to bedc92598644 (1 revision) (flutter/engine#43617)
2023-07-12 [email protected] move rtree and canvas_spy sources to Fuchsia sub-directory (flutter/engine#43615)
2023-07-12 [email protected] Roll Skia from 68e3c0b3eea7 to 4e989b1564ee (6 revisions) (flutter/engine#43614)
2023-07-12 [email protected] Document (and assert) that channel names can't contains nulls (flutter/engine#43593)
2023-07-12 [email protected] Reland "Refactor JSNumber.toDart and Object.toJS" (flutter/engine#43363)
2023-07-12 [email protected] Add a flag to `ParagraphBuilder` for rounding hack migration (flutter/engine#43118)
2023-07-12 [email protected] [Impeller] Fixes for asymmetric stencil descriptors (flutter/engine#43535)
2023-07-12 [email protected] Roll Skia from adeeb8d50f7c to 68e3c0b3eea7 (1 revision) (flutter/engine#43609)
2023-07-12 [email protected] Add comment to use of 3x3 mapRect in TransformLayer (flutter/engine#43608)
2023-07-12 [email protected] Roll Skia from a251a36ea519 to adeeb8d50f7c (1 revision) (flutter/engine#43606)
2023-07-12 [email protected] Roll Dart SDK from b95f6531c726 to 8f8f281ccdc6 (2 revisions) (flutter/engine#43607)
2023-07-12 [email protected] Roll Skia from ebc149cff431 to a251a36ea519 (2 revisions) (flutter/engine#43604)
2023-07-12 [email protected] [Impeller] Use new SkParagraph APIs for stroked text. (flutter/engine#41735)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from 1STsUj0X5Ygp to xBJq6PsO5ebb
  fuchsia/sdk/core/mac-amd64 from 0fvk838jTDNQ to 3C7P0w8ySmtq

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] 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
...
LouiseHsu pushed a commit to LouiseHsu/flutter that referenced this pull request Jul 13, 2023
@engine-flutter-autoroll @LouiseHsu
Roll Flutter Engine from 16e2ab7e986c to 1b1ccdd1f527 (13 revisions) (f…
b575927 
…lutter#130458)

flutter/engine@16e2ab7...1b1ccdd

2023-07-12 [email protected] Roll Skia from 4e989b1564ee to bedc92598644 (1 revision) (flutter/engine#43617)
2023-07-12 [email protected] move rtree and canvas_spy sources to Fuchsia sub-directory (flutter/engine#43615)
2023-07-12 [email protected] Roll Skia from 68e3c0b3eea7 to 4e989b1564ee (6 revisions) (flutter/engine#43614)
2023-07-12 [email protected] Document (and assert) that channel names can't contains nulls (flutter/engine#43593)
2023-07-12 [email protected] Reland "Refactor JSNumber.toDart and Object.toJS" (flutter/engine#43363)
2023-07-12 [email protected] Add a flag to `ParagraphBuilder` for rounding hack migration (flutter/engine#43118)
2023-07-12 [email protected] [Impeller] Fixes for asymmetric stencil descriptors (flutter/engine#43535)
2023-07-12 [email protected] Roll Skia from adeeb8d50f7c to 68e3c0b3eea7 (1 revision) (flutter/engine#43609)
2023-07-12 [email protected] Add comment to use of 3x3 mapRect in TransformLayer (flutter/engine#43608)
2023-07-12 [email protected] Roll Skia from a251a36ea519 to adeeb8d50f7c (1 revision) (flutter/engine#43606)
2023-07-12 [email protected] Roll Dart SDK from b95f6531c726 to 8f8f281ccdc6 (2 revisions) (flutter/engine#43607)
2023-07-12 [email protected] Roll Skia from ebc149cff431 to a251a36ea519 (2 revisions) (flutter/engine#43604)
2023-07-12 [email protected] [Impeller] Use new SkParagraph APIs for stroked text. (flutter/engine#41735)

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] 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
LouiseHsu pushed a commit to LouiseHsu/flutter that referenced this pull request Jul 13, 2023
@engine-flutter-autoroll @LouiseHsu
Roll Flutter Engine from 16e2ab7e986c to b4080361f2f9 (28 revisions) (f…
d6f8d61 
…lutter#130496)

Roll Flutter Engine from 16e2ab7e986c to b4080361f2f9 (28 revisions)

flutter/engine@16e2ab7...b408036

2023-07-13 [email protected] Roll Skia from e5ec341bc3ca to 56b68ce6196c (1 revision) (flutter/engine#43633)
2023-07-13 [email protected] Roll Dart SDK from f499e91e8cb2 to ade4dae923f3 (1 revision) (flutter/engine#43632)
2023-07-13 [email protected] Roll Skia from 811b046c673b to e5ec341bc3ca (1 revision) (flutter/engine#43631)
2023-07-13 [email protected] Make GOMA state automatic by default (flutter/engine#43584)
2023-07-13 [email protected] Roll Skia from c8da0c657c4e to 811b046c673b (3 revisions) (flutter/engine#43630)
2023-07-13 [email protected] [Impeller] Remove unactionable error logs and use structure chains for instance creation. (flutter/engine#43629)
2023-07-13 [email protected] Roll Skia from 7f391ea9164e to c8da0c657c4e (1 revision) (flutter/engine#43628)
2023-07-13 [email protected] [Impeller] Add RAII wrappers for VMA objects. (flutter/engine#43626)
2023-07-13 [email protected] Roll Fuchsia Linux SDK from 1STsUj0X5YgpiSNEb... to xBJq6PsO5ebblODMe... (flutter/engine#43627)
2023-07-13 [email protected] Roll Skia from 6ed93436d57c to 7f391ea9164e (1 revision) (flutter/engine#43625)
2023-07-13 [email protected] Roll Dart SDK from 8f8f281ccdc6 to f499e91e8cb2 (3 revisions) (flutter/engine#43623)
2023-07-13 [email protected] Roll Fuchsia Mac SDK from 0fvk838jTDNQ_l43k... to 3C7P0w8ySmtqpyi3S... (flutter/engine#43622)
2023-07-13 [email protected] Roll Skia from bedc92598644 to 6ed93436d57c (3 revisions) (flutter/engine#43621)
2023-07-13 [email protected] [Impeller] Add support to embedder for Impeller on GL (via Angle on Windows). (flutter/engine#43388)
2023-07-12 [email protected] [Impeller] Allocate buffers out of a pool on the raster thread. (flutter/engine#43564)
2023-07-12 [email protected] Roll Skia from 4e989b1564ee to bedc92598644 (1 revision) (flutter/engine#43617)
2023-07-12 [email protected] move rtree and canvas_spy sources to Fuchsia sub-directory (flutter/engine#43615)
2023-07-12 [email protected] Roll Skia from 68e3c0b3eea7 to 4e989b1564ee (6 revisions) (flutter/engine#43614)
2023-07-12 [email protected] Document (and assert) that channel names can't contains nulls (flutter/engine#43593)
2023-07-12 [email protected] Reland "Refactor JSNumber.toDart and Object.toJS" (flutter/engine#43363)
2023-07-12 [email protected] Add a flag to `ParagraphBuilder` for rounding hack migration (flutter/engine#43118)
2023-07-12 [email protected] [Impeller] Fixes for asymmetric stencil descriptors (flutter/engine#43535)
2023-07-12 [email protected] Roll Skia from adeeb8d50f7c to 68e3c0b3eea7 (1 revision) (flutter/engine#43609)
2023-07-12 [email protected] Add comment to use of 3x3 mapRect in TransformLayer (flutter/engine#43608)
2023-07-12 [email protected] Roll Skia from a251a36ea519 to adeeb8d50f7c (1 revision) (flutter/engine#43606)
2023-07-12 [email protected] Roll Dart SDK from b95f6531c726 to 8f8f281ccdc6 (2 revisions) (flutter/engine#43607)
2023-07-12 [email protected] Roll Skia from ebc149cff431 to a251a36ea519 (2 revisions) (flutter/engine#43604)
2023-07-12 [email protected] [Impeller] Use new SkParagraph APIs for stroked text. (flutter/engine#41735)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from 1STsUj0X5Ygp to xBJq6PsO5ebb
  fuchsia/sdk/core/mac-amd64 from 0fvk838jTDNQ to 3C7P0w8ySmtq

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] 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
...
kjlubick pushed a commit to kjlubick/engine that referenced this pull request Jul 14, 2023
@johnmccutchan @kjlubick
[Impeller] Fixes for asymmetric stencil descriptors (flutter#43535)
e004719 
Fixes for asymmetric stencil descriptors

- Fix GLES implementation to properly set back and front face options in
the case that they aren't identical.
- Add write_mask to hash function.

flutter/flutter#129855
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@bdero bdero bdero approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@johnmccutchan johnmccutchan

Labels

e: impeller Work in progress (WIP) Not ready (yet) for review!

Projects

No open projects
Impeller
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@johnmccutchan @bdero @chinmaygarde