Use newer Skia API for PathMeasure#7809
Conversation
|
If the api surface of the dart code didn't change, then no |
|
It didn't, although an internal class used by dart:ui (_PathMeasure in painting.dart) is changing. |
| class _PathMeasure extends NativeFieldWrapperClass2 { | ||
| _PathMeasure(Path path, bool forceClosed) { | ||
| currentContourIndex = -1; // PathMetricIterator will increment this the first time. | ||
| currentContourIndex = -1; // nextContour will incremenet this to the zero based index. |
There was a problem hiding this comment.
Argh. I thought I pushed up the fix for this before merging. Will fix...
I missed this in #7809 - thought I had pushed up the change and realized I hadn't saved in my local editor.
flutter/engine@d48de7a...3757390 git log d48de7a..3757390 --no-merges --oneline 3757390 Roll src/third_party/dart ecd7a88606..0a7dcf17eb (4 commits) 61d3080 Add FFI to libraries.yaml. (flutter/engine#7811) f5259b8 Roll src/third_party/skia b6f53783337e..186669c4128b (8 commits) (flutter/engine#7816) 816921b Use newer Skia API for PathMeasure (flutter/engine#7809) da56ff9 Roll src/third_party/skia 94a5328e0e4b..b6f53783337e (8 commits) (flutter/engine#7812) 04fbc25 Roll src/third_party/skia 66f09a72995a..94a5328e0e4b (1 commits) (flutter/engine#7810) 347d690 Add support for new Scenic clip planes. (flutter/engine#7804) 309b90c Roll src/third_party/skia 14d64dd4c47c..66f09a72995a (8 commits) (flutter/engine#7805) 1dead52 Roll src/third_party/skia 0b6ae6386d34..14d64dd4c47c (10 commits) (flutter/engine#7802) 74d94e5 Revert "Use all font managers to discover fonts for strut. (#7734)" (flutter/engine#7801) 769016c Roll src/third_party/skia 63d477cd99b0..0b6ae6386d34 (9 commits) (flutter/engine#7798) f7eb3cb Roll src/third_party/skia 217acf58d0d8..63d477cd99b0 (1 commits) (flutter/engine#7797) c4a5555 Use all font managers to discover fonts for strut. (flutter/engine#7734) 17b7d1e Roll src/third_party/dart 754e5f404c..ecd7a88606 (8 commits) b4ed8cf Roll src/third_party/skia 233fc0b03c02..217acf58d0d8 (1 commits) (flutter/engine#7795) 27ab9f7 Roll src/third_party/skia 454e5fb7457d..233fc0b03c02 (4 commits) (flutter/engine#7794) c58f5fe Roll src/third_party/dart fdfe40ea95..754e5f404c (13 commits) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.
| /// iterator is at the contour for which they were created. It will also only be | ||
| /// valid for the path as it was specifed when [Path.computeMetrics] was called. | ||
| /// If additional contours are added or any contours are updated, the metrics | ||
| /// need to be recomputed. |
There was a problem hiding this comment.
what will happen if they're not?
There was a problem hiding this comment.
I think what's really going on here is that the API takes a snapshot of the path when you call it. In which case, we should just say that ("[Path.computeMetrics] measures the [Path] as it stands when the method is called; subsequently changing the [Path] does not affect the objects returned from the method." or something).
There was a problem hiding this comment.
Yes - I can update that to clarify.
| // calling `_moveNext` - `_moveNext` should be called after the first | ||
| // iteration is done instead of before. | ||
| // Before Skia introduced an SkPathContourMeasureIter, this didn't work like | ||
| // a normal iterator. Now it does. |
There was a problem hiding this comment.
nit: avoid talking about the history of the code in the comments. it doesn't really matter to the person reading tho code.
| expect(metrics[1].length, 10); | ||
| expect(metrics[1].isClosed, false); | ||
| expect(() => metrics[1].getTangentForOffset(4.0), isNotNull); | ||
| expect(() => metrics[1].extractPath(4.0, 10.0), isNotNull); |
There was a problem hiding this comment.
isNotNull is really really vague. We should test that it's actually returning the right thing.
Also we should check what happens when the path is mutated, since apparently that changed.
There was a problem hiding this comment.
It hasn't changed, it just wouldn't have worked at all before and now it's very likely undefined behavior (but perhaps still gets you the old values from before the path changed). @reed-at-google - which is correct?
There was a problem hiding this comment.
I can update the test for getTangentForOffset to something more meaningful, but the getPath one gets a little tougher - I guess I could compute the metrics on it and make sure they come out to the expected length.
There was a problem hiding this comment.
The path is copied as soon as you create/reset the pathmeasure object, so there is no worry about edits to the path after that (including deleting it).
BTW: This is pretty cheap, as we just "ref" the path's internals, which perform a copy-on-write if someone actually edits the path after that. Thus in the common case, no actual copy has had to occur.
6 participants
Fixes flutter/flutter#27139
Skia refactored the API for this as of https://skia-review.googlesource.com/c/skia/+/187922 - we can now support the methods exposed here even if the iterator has advanced beyond the current contour.
This allows us to avoid throwing if someone tries to access some members of the
PathMetricclass when the iterator isn't on the expected contour.@jonahwilliams - do I have to update anything in the ui_stub folder for this?