Repository navigation
Avoid sqrt in gesture distance checks - #187895
bernaferrari wants to merge 5 commits into
Conversation
github-actions
Bot
added
framework
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces distance calculations with squared distance comparisons across several gesture recognizers and the InteractiveViewer widget to avoid expensive square root operations, and adds a microbenchmark to evaluate this optimization. The review feedback suggests adding assertions to verify that tolerances and hit slops are non-negative before squaring them, and adding documentation comments to the public functions in the new benchmark file.
130bb20 to
79d7840
Compare
79d7840 to
67e4c49
Compare
67e4c49 to
1ac0463
Compare
1ac0463 to
dc7efef
Compare
dc7efef to
55e9df2
Compare
Renzo-Olivares
left a comment
Renzo-Olivares
left a comment
There was a problem hiding this comment.
@bernaferrari Thanks for the contribution and your patience on this one! I left a comment that applies to all the changes here. My main concern is readability which you pointed out in the original issue, but maybe we can improve on it.
| if (pendingDelta!.distance > computeHitSlop(kind, gestureSettings)) { | ||
| final double hitSlop = computeHitSlop(kind, gestureSettings); | ||
| assert(hitSlop >= 0.0); | ||
| if (pendingDelta!.distanceSquared > hitSlop * hitSlop) { |
There was a problem hiding this comment.
This is tricky because on one hand it is mathematically correct, but while reading this code it took me an extra step or two to figure out what was happening here. I think it would be more difficult to reason about without the git diff. I wonder if we can somehow still get the gains of avoiding sqrt but keep the code readable. Possible example below. A helper for each of these cases doesn't seem ideal though. Let me know what you think.
@override
void checkForResolutionAfterMove() {
assert(pendingDelta != null);
final double hitSlop = computeHitSlop(kind, gestureSettings);
assert(hitSlop >= 0.0);
if (pendingDelta!.distanceSquared > hitSlop * hitSlop) {
resolve(GestureDisposition.accepted);
}
}bool deltaExceedsThreshold(Offset delta, double threshold) {
assert(threshold >= 0.0);
return distanceSquared > (threshold * threshold);
}
@override
void checkForResolutionAfterMove() {
assert(pendingDelta != null);
final double hitSlop = computeHitSlop(kind, gestureSettings);
if (deltaExceedsThreshold(pendingDelta!, hitSlop)) {
resolve(GestureDisposition.accepted);
}
}
|
We could have an extension for Offset (or even modify Offset in Dart(?)) but this optimization happens in like ~6 times (maybe more, I'm not accounting for material since it is frozen) and the BoxDecoration optimization would be a different case (but maybe I can just comment). Any thoughts? Do you want me to draft it? I'll also try your idea. |
55e9df2 to
9d76848
Compare
|
Check if you like the idea of the extension to offset (we could also upstream the change to Dart if it makes sense). I just pushed. But I'm open to different ideas. |
|
Apologies for the delay on this one @bernaferrari, will take a look this week. |
Renzo-Olivares
left a comment
Renzo-Olivares
left a comment
There was a problem hiding this comment.
Thank you for your patience @bernaferrari, I left a review!
| /// Methods for comparing the magnitude of an [Offset], such as a pointer | ||
| /// delta or velocity, against a threshold, such as a touch slop or a minimum | ||
| /// fling velocity. | ||
| extension OffsetDistanceComparison on Offset { |
There was a problem hiding this comment.
I'm not the biggest fan of using an extension for this since it gets exposed to all IDE users that import the gestures library. I think since we already use this pattern in the framework as pointed out in #187891, i'm okay with this PR in its original state prior to adding this extension method.
…nce checks Replaces the raw 'distanceSquared > threshold * threshold' comparisons with a small named extension on Offset in events.dart, so call sites read like the original code while still avoiding the square root.
ForcePressGestureRecognizer compared event.delta.distanceSquared against an unsquared hit slop, so the effective move tolerance was sqrt(18) = ~4.2 logical pixels instead of the intended 18. Present since the original implementation in flutter#24554.
Tree_analyze failed because the PUBSPEC CHECKSUM was still the old value after declaring a direct vector_math dependency.
f3bb30f to
79dcae8
Compare
|
Check if you like now, naming is a bit tricky ( |
Fix #187894
This started from my curiosity on #187891
Avoids computing square roots for several gesture and geometry distance threshold checks by comparing squared distances against squared thresholds instead. This keeps the same behavior while avoiding unnecessary
sqrtwork in hot paths.sqrt(dx² + dy²) <= ris equivalent to:dx² + dy² <= r²This updates distance checks in:
InteractiveViewernearest-point and fling threshold logicI added a micro-benchmark that shows code 9.1% faster (but depending on the bench and how much you isolate x*x vs sqrt it could be up to 73%).
Let me know what you think.