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

[Impeller] Metal:Reset Encoder viewport and scissor rect in case the command specifies no opinion #34252

Merged
merged 2 commits into from
Jun 24, 2022

Conversation

ColdPaleLight
Copy link
Member

@ColdPaleLight ColdPaleLight commented Jun 23, 2022

fix flutter/flutter#105936

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

@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.

Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

A minor nit about drying up the code a bit. But otherwise LGTM. Thanks!

@@ -440,6 +440,16 @@ static bool Bind(PassBindingsCache& pass,
.zfar = v.depth_range.z_far,
};
[encoder setViewport:viewport];
} else {
MTLViewport viewport = {
Copy link
Member

Choose a reason for hiding this comment

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

Minor nit: You can dry this up a bit. Instead of checking if the viewport has value, then getting its value and then converting to MTLViewport in two places, you could use value_or when getting the viewport. Then you won't need branch here and the code will be more concise.

Something like:

auto v = command.viewport.value_or({GetRenderTargetSize...});
// The old code remains as is

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@ColdPaleLight ColdPaleLight force-pushed the reset_viewport_and_scissor branch from 86e1f67 to 3f9a428 Compare June 24, 2022 02:57
@ColdPaleLight ColdPaleLight added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 24, 2022
@auto-submit auto-submit bot merged commit 1daf7ba into flutter:main Jun 24, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 24, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 24, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 24, 2022
bdero added a commit to flutter/flutter that referenced this pull request Jun 25, 2022
* 9508a36 Roll Dart SDK from 692562354d6d to d3b8091c30f0 (1 revision) (flutter/engine#34273)

* a2985c0 Roll Fuchsia Linux SDK from F1U6IH2Nf... to aRT7s0Yct... (flutter/engine#34251)

* 54867f3 Roll Skia from bdd0205ae470 to 4345a2ea731a (1 revision) (flutter/engine#34268)

* 98221a2 Clean up text input configuration in clearTextInputClient (flutter/engine#34209)

* b9e02cc Adds a license check shard to CI (flutter/engine#34274)

* 1daf7ba [Impeller] Metal:Reset Encoder viewport and scissor rect in case the command specifies no opinion (flutter/engine#34252)

* 83b9a59 [Linux] remove duplicate clone_string() in favor of g_strdup() (flutter/engine#34031)

* Don't use .packages

* Another attempt

Co-authored-by: engine-flutter-autoroll <[email protected]>
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
* 9508a36 Roll Dart SDK from 692562354d6d to d3b8091c30f0 (1 revision) (flutter/engine#34273)

* a2985c0 Roll Fuchsia Linux SDK from F1U6IH2Nf... to aRT7s0Yct... (flutter/engine#34251)

* 54867f3 Roll Skia from bdd0205ae470 to 4345a2ea731a (1 revision) (flutter/engine#34268)

* 98221a2 Clean up text input configuration in clearTextInputClient (flutter/engine#34209)

* b9e02cc Adds a license check shard to CI (flutter/engine#34274)

* 1daf7ba [Impeller] Metal:Reset Encoder viewport and scissor rect in case the command specifies no opinion (flutter/engine#34252)

* 83b9a59 [Linux] remove duplicate clone_string() in favor of g_strdup() (flutter/engine#34031)

* Don't use .packages

* Another attempt

Co-authored-by: engine-flutter-autoroll <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects: engine autosubmit Merge PR when tree becomes green via auto submit App e: impeller needs tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Impeller] Metal: Encoder viewport and stencil is not reset in case the command specifies no opinion.
2 participants