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

Conversation

@chinmaygarde
Copy link
Member

Viewports, scissors, and stencil reference values are part of the dynamic state and must be initialized in the case where the caller doesn't set any value via the render pass.

Fixes flutter/flutter#142943

@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 "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use 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
Contributor

@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, thanks for fixing this!

Does the test that was failing run on CI in a way that will catch this? If not, is there a way to make it do so?

@chinmaygarde
Copy link
Member Author

chinmaygarde commented Feb 5, 2024

I think we'll get a lot of value for free by just running the existing playgrounds harness with --enable_playground --enable_vulkan_validation --use_swiftshader --playground_timeout_ms=0. We can migrate the existing stuff to gold piecemeal for actual pixel checks.

This should have been easily caught on CI.

@jonahwilliams
Copy link
Contributor

Right, we don't need to golden check every playground - but it sounds like if we have validation enabled and this test wasn't failing when I landed my PR, this test must not be running?

@chinmaygarde
Copy link
Member Author

I think the test was running with validations without a single playground frame being renderered. --playground_timeout_ms=0 ensures that one and only one frame will be rendered.

@jonahwilliams
Copy link
Contributor

Can we set that flag on CI?

@chinmaygarde
Copy link
Member Author

TBH, I thought we already were. Filed flutter/flutter#142995 to followup.

Viewports, scissors, and stencil reference values are part of the
dynamic state and must be initialized in the case where the caller
doesn't set any value via the render pass.

Fixes flutter/flutter#142943
@chinmaygarde chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 6, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Feb 6, 2024
@auto-submit
Copy link
Contributor

auto-submit bot commented Feb 6, 2024

auto label is removed for flutter/engine/50373, due to - The status or check suite Linux mac_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

@chinmaygarde chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 6, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Feb 6, 2024
@auto-submit
Copy link
Contributor

auto-submit bot commented Feb 6, 2024

auto label is removed for flutter/engine/50373, due to - The status or check suite Linux linux_web_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

@chinmaygarde chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 7, 2024
@auto-submit auto-submit bot merged commit 792030d into flutter:main Feb 7, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 7, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 7, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 7, 2024
…143112)

flutter/engine@19ae46a...e4a5acc

2024-02-07 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Dart SDK from 452dd17120b7 to 322c81160fa9 (1 revision)" (flutter/engine#50450)
2024-02-07 [email protected] Roll Skia from a2fe78a31a14 to eacaa2d3600c (3 revisions) (flutter/engine#50445)
2024-02-07 [email protected] [Impeller] Initialize the stencil reference value. (flutter/engine#50373)
2024-02-07 [email protected] Roll Skia from 9b103a4148e2 to a2fe78a31a14 (2 revisions) (flutter/engine#50441)
2024-02-07 [email protected] Fixed CONTRIBUTING.md markdown lints (flutter/engine#50439)
2024-02-07 [email protected] Roll Skia from 7777976cd518 to 9b103a4148e2 (1 revision) (flutter/engine#50438)
2024-02-07 [email protected] Roll Dart SDK from 452dd17120b7 to 322c81160fa9 (1 revision) (flutter/engine#50437)

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
@chinmaygarde chinmaygarde deleted the stencil_reference branch August 22, 2024 19:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App e: impeller

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Impeller] Direct encode on Vulkan doesn't account for dynamic state.

2 participants