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

[web] Fix conic to quad conversion assertion. #18811

Merged
merged 4 commits into from
Jun 20, 2020

Conversation

ferhatb
Copy link
Contributor

@ferhatb ferhatb commented Jun 4, 2020

Description

When computing subdivision count for quad approximation of conic, if initial error rate is below tolerance, debug build throws assertion and fails to construct path.

Related Issues

Related to refactor for flutter/flutter#44572

Tests

Added test case to test/path_test.dart

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [contributor guide] and followed the process outlined there for submitting PRs.
  • I signed the [CLA].
  • I read and followed the [C++, Objective-C, Java style guides] for the engine.
  • I read the [tree hygiene] wiki page, which explains my responsibilities.
  • I updated/added relevant documentation.
  • All existing and new tests are passing.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read [handling breaking changes].

Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

LGTM but the second part of the assert in the toQuads method seems to belong in the _computeSubdivisionCount method?

@@ -33,7 +33,7 @@ class Conic {

// Split conic into quads, writes quad coordinates into [_pointList] and
// returns number of quads.
assert(subdivideCount > 0);
assert(subdivideCount >= 0 && subdivideCount <= _maxSubdivisionCount);
Copy link
Member

Choose a reason for hiding this comment

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

Isn't subdivideCount <= _maxSubdivisionCount guaranteed by the implementation of _computeSubdivisionCount?

The documentation of _computeSubdivisionCount says: The number of subdivisions never exceed _maxSubdivisionCount

Should the second half of this assert be moved to the _countSubdivisionCount method, so the postcondition in the documentation is "guaranteed"?

assert(pow2 <= _maxSubdivisionCount);
return pow2;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added check here to make sure if implementation breaks, this will throw.

@ferhatb ferhatb merged commit b8654ff into flutter:master Jun 20, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants