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

Conversation

@ferhatb
Copy link
Contributor

@ferhatb ferhatb commented Feb 13, 2020

@auto-assign auto-assign bot requested a review from flar February 13, 2020 23:47
Copy link
Contributor

@harryterkelsen harryterkelsen left a comment

Choose a reason for hiding this comment

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

LGTM with nits. Mostly rubberstamping here

class _SurfacePathMeasure {
_SurfacePathMeasure(this._path, this.forceClosed) {
_currentContourIndex =
-1; // nextContour will increment this to the zero based index.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: put this comment on the previous line to avoid ugly formatting

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.


final List<_PathSegment> _segments = [];
// Allocate buffer large enough for returning cubic curve chop result.
static final Float32List _buffer = Float32List(4 * 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I don't think it's clearer to put 4 * 2 because I can't tell from the context why the buffer would be 2 groups of 4 rather than just 8 entries

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx. added comment above.

// If control point is at start or end point, this yields 0 for t = 0 and
// t = 1. In that case use the quad end points to compute tangent instead
// of derivative.
final ui.Offset tangentVector = ((t == 0 && x0 == x1 && y0 == y1) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on comment above, shouldn't we also be checking for t == 1 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx for spotting incorrect t == 0. Corrected.

buffer[5] = abc2y;
}

double _interpolate(double startValue, double endValue, double t)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'm pretty sure this already exists somewhere (lerp?)

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 comment. lerpDouble uses num does null checks.

@ferhatb
Copy link
Contributor Author

ferhatb commented Feb 14, 2020

Merging on infra failure.

@ferhatb ferhatb merged commit 15e7f51 into flutter:master Feb 14, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 15, 2020
jason-simmons pushed a commit to flutter/flutter that referenced this pull request Feb 18, 2020
* c0549fb Roll buildroot. (flutter/engine#16613)

* 8b0b649 improve surface state assert error messages (flutter/engine#16595)

* cd77e78 Fix drawRRect failure when shader is specified (flutter/engine#16601)

* fe63094 [web] Handle alignment correctly in Paragraph.getPositionForOffset (flutter/engine#16569)

* 65d1126 [web] Fixing launching Safari. This should solve the LUCI issue (flutter/engine#16590)

* f88f7df [web] Unskip tests that are already passing in Safari (flutter/engine#16567)

* 594f660 [shell tests] Integrate Vulkan with Shell Tests

* 400ed7c Revert "[shell tests] Integrate Vulkan with Shell Tests"

* 15e7f51 Implement Path extractPath, tangent APIs (flutter/engine#16599)

* 4941ff7 Remove usage of Dart_AllocateWithNativeFields from tonic (flutter/engine#16588)

* bb01cb7 Roll fuchsia/sdk/core/linux-amd64 from Bmq1m... to J-_s6... (flutter/engine#16592)
NoamDev pushed a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
* Add pathMeasure
* Implement tangetAt
* add extractPath segment iterator
* Implement extractPath/chopCubicAt/chopQuadAt
* Wire up extractPath and add tests for cubic/quadratic
* Switch to interpolate on chopCubic
* addressed review comments
NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
@ferhatb ferhatb deleted the path_measure branch August 10, 2020 20:45
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