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

[Impeller] Change Path::CreatePolyline from tolerance to scale, and make it required #39917

Merged
merged 2 commits into from
Feb 27, 2023

Conversation

dnfield
Copy link
Contributor

@dnfield dnfield commented Feb 27, 2023

Fixes flutter/flutter#121540
See also dnfield/flutter_svg#830 which has screenshots.

This patch mainly does two things:

  • Makes sure that we pass a non-default tolerance value when creating a polyline for strokes. Previously, we calculated a tolerance but the method we called dropped the value on the floor (see first line of StrokePathGeometry::CreateSolidStrokeVertices). An "unused parameter" check wouldn't have caught this, since we use the parameter later in the method.
  • Removes the stroke width from the calculation of the tolerance. This is error prone because a stroke_width of 0 means hairline for Flutter, but div by zero results in badness. I am not seeing any benefits when looking at this at various scales/high stroke widths.
  • Changes the tolerance parameter on Path::CreatePolyilne to a scale parameter and removes the default. This should prevent this error from happening in the future.

Ideally we'd also have a test that CreateSolidStrokeVertices. That would require either friendship or making it public. I'm open to doing that if reviewers think it would be helpful. We do risk regression here if someone uses an incorrect value for the scale in the future.

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

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.

An "unused parameter" check wouldn't have caught this, since we use the parameter later in the method.

I think this makes sense. If only for TUs handled by the impeller_component GN template. We can fixup existing issues by either dropping the argument name or explicitly annotating it as being unused.

EDIT: Can we file a bug this?

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. I have a questions about what should happen scale=0, but that concern is preexisting/not introduced by this PR.

Polyline polyline;
auto tolerance = kDefaultCurveTolerance / scale;
Copy link
Member

Choose a reason for hiding this comment

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

IIRC, it's acceptable for tolerance to end up being NaN (when the max basis length is 0). Is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, curves ending reverting to lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#38497 (comment) is where you discussed this before

/// transformed.
///
/// It is suitable to use the max basis length of the matrix used to transform
/// the path.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Maybe mention what will happen when scale is 0 here. E.g. will curves revert to lines (seems like the most reasonable behavior)?

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

@dnfield dnfield added e: impeller autosubmit Merge PR when tree becomes green via auto submit App labels Feb 27, 2023
@dnfield dnfield self-assigned this Feb 27, 2023
@dnfield dnfield changed the title Change Path::CreatePolyline from tolerance to scale, and make it required [Impeller] Change Path::CreatePolyline from tolerance to scale, and make it required Feb 27, 2023
@auto-submit auto-submit bot merged commit ae5231b into flutter:main Feb 27, 2023
@dnfield dnfield deleted the poly_tol branch February 27, 2023 21:27
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 27, 2023
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] curve tolerance is wrong for strokes
4 participants