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

Conversation

@jason-simmons
Copy link
Member

This reverts commit b053d74.

@jason-simmons jason-simmons requested review from flar and zanderso March 7, 2023 02:03
@jason-simmons
Copy link
Member Author

This was causing errors in the DanTup_tiler customer test (sorting/isometric golden image test)

See https://ci.chromium.org/ui/p/flutter/builders/try/Mac%20customer_testing/47476/overview

Copy link
Contributor

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

flar commented Mar 7, 2023

The original PR was built from about 6 different commits so I can start with bisecting those to narrow down the cause...

@jason-simmons jason-simmons added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 7, 2023
@auto-submit auto-submit bot merged commit b3e314e into flutter:main Mar 7, 2023
@jason-simmons
Copy link
Member Author

The sorting/isometric test diff reproduces with the first commit in the PR (288dfdb)

I can get the test to pass by reverting the changes to lib/ui/painting/canvas.cc and lib/ui/painting/paint.cc within that commit. But I haven't figured out which change within those files is causing the difference.

I did notice one change in behavior introduced in that commit. Before PR #40083 Paint::sync_to was checking flags.is_stroked(builder->getStyle()) to decide whether to set stroke properties. It was doing this even if flags.applies_style() was not set. So the state of the builder prior to calling Paint::sync_to could affect what sync_to does.

PR #40083 changes this so that the builder's current state is not available to Paint::paint. Paint::paint sets stroke properties based only on the paint data passed from Dart. This seems correct.

But in any case, it does not look like this issue is affecting the sorting/isometric test.

@jason-simmons
Copy link
Member Author

I found the issue - in Canvas::drawImageRect, the call to builder()->DrawImageRect should pass false as the last argument.

(It's currently passing the enum SkCanvas::kFast_SrcRectConstraint, which is intended for the old drawImageRect method and is being implicitly cast to enforce_src_edges = true)

@flar
Copy link
Contributor

flar commented Mar 7, 2023

And a quick grep shows a lot more occurences of that flag in the DL code. I'll follow up and see if any of the others are wrong. It's annoying that the compiler allows implicit conversion between a bool and an enum. Grrr...

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants