Change uber SDF to get pixel size (for AA) using euclidean distance rather than manhattan distance.#184984
Conversation
…ather than manhattan distance.
There was a problem hiding this comment.
Code Review
This pull request updates the anti-aliasing logic in the uber_sdf.frag shader by replacing the fwidth function with a manual calculation of the SDF gradient magnitude using dFdx and dFdy. It also adds descriptive comments explaining the geometric interpretation of the gradient and its role in determining the pixel-relative fade size. I have no feedback to provide.
|
Lots of golden changes, as expected. https://flutter-gold.skia.org/search?issue=184984&crs=github&patchsets=1&corpus=flutter I looked through all of them and they all show the same categories of differences. A selected set of the simplest examples I found which demonstrate the expected changes:
|
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
walley892
left a comment
There was a problem hiding this comment.
Really nice. A few nits on the comments but overall good
| // fwidth(dist) gives the change in SDF per pixel. | ||
| float fade_size = fwidth(dist) * frag_info.aa_pixels * 0.5; | ||
|
|
||
| // Gradient vector of the SDF at point p. Points in the direction of steepest |
There was a problem hiding this comment.
Nit-ish: could you work in somehow that this is the gradient w.r.t. the pixels?
| // The length of the gradient vector is how fast the SDF changes per unit | ||
| // distance. This is equal to the width of a pixel when measuring in the | ||
| // direction of the gradient vector. |
There was a problem hiding this comment.
This one took me a minute to parse. Could you clarify that this is scaling pixel dimensions to SDF units?
There was a problem hiding this comment.
I updated these comments to try to explain things in more detail. I don't know if it makes sense, or if I'm just being too wordy and talking in circles. Let me know what you think and if you have any suggestions.
|
Ping @walley892 @flar |
flar
left a comment
There was a problem hiding this comment.
Sorry, never hit submit on these comments.
| // The gradient is based on dFdx and dFdy, which are computed in terms of the | ||
| // screen-space pixel grid. As a result, this gradient vector is in terms of | ||
| // physical pixels, rather than local-space coordinates. | ||
| vec2 gradient = vec2(dFdx(dist), dFdy(dist)); |
There was a problem hiding this comment.
The other way around - the vector is in local space rather than physical pixels. It's a rate "per physical pixel", but the rate is measuring the change in the distance function, which is in local space.
It's dF(the function: dist)/d[xy] or how much the local-space distance function changes in X and Y.
There was a problem hiding this comment.
By "in terms of physical pixels", I did mean that it is "per physical pixel". But I can see how the wording can be kind of ambiguous. I reworded this to hopefully be more clear.
| // In local space, the SDF always increases by 1 in the gradient's direction | ||
| // per unit distance. That's the definition of an SDF - it is the distance to | ||
| // the closest point of the shape. But in terms of screen-space, the SDF may | ||
| // increase a non-1 number of units per physical pixel due to |
There was a problem hiding this comment.
Suggestion: Another word for non-1 is "non-unit"
There was a problem hiding this comment.
Unit can mean 1, but it also has other meanings. If I use it here the sentence becomes "the SDF may increase in a non-unit number of units per physical pixel", which I think is confusing. I think non-1 is clearer here.
But non-1 is also a weird term. So I reworded this in a way that I hope is less confusing.
There was a problem hiding this comment.
Right, not a drop-in replacement per se.
| // scales/skews/rotations. | ||
| // | ||
| // As an example, if the SDF is a plain unscaled/unskewed circle, the gradient | ||
| // is vec2(1.0, 0.0) for points along the x axis: For every one pixel we move |
There was a problem hiding this comment.
"for points aligned horizontally with the center"? It's along the X axis if you've transformed to be center-relative, which we do, but your example doesn't really say that. Also, isn't it negative left of the center and positive right of it? The distance is reducing as you approach the center from the left and increasing after you pass it. Also, if you are right on the axis, then the distance for the pixels below the sample would be larger than the pixel on the axis and so you'd have a positive value for the Y part of the vector.
If we end up with a circle centered horizontally between 2 pixels, the vector would be (0, 1). If the center was midway in the middle of 4 pixels, the vector would be (0, 0) because all 4 of those pixels would evaluate to the same value.
There was a problem hiding this comment.
This example had some implicit assumptions that the circle is at the origin, and that the points along the axis meant along the positive x axis. I made these assumptions explicit.
As you point out, there can be points where the gradient vector is 0. This would only occur at the center of shapes, where the distance to the shape's edge is equidistant horizontally and equidistant vertically. So this would not affect our antialiasing pixel width logic, antialiasing is only applicable at the edges of shapes, not in their center points.
|
autosubmit label was removed for flutter/flutter/184984, because This PR has not met approval requirements for merging. Changes were requested by {flar}, please make the needed changes and resubmit this PR.
|
That is definitely a bug in autosubmit. It's picking up a stale review from jim. |
Yeah, that's really odd. Jim did request reviews in an earlier revision, but has since approved the PR multiple times, including in his most recent action on the PR. The icons on the reviewers list in the github UI also don't show the "requested changes" icon for him. |
|
Yea, I filed an issue. This needs to get fixed: #185224 I think it's become more of an issue now that approvals get wiped out after new commits. |
flar
left a comment
There was a problem hiding this comment.
Fixing my stale "request changes" review.
…distance rather than manhattan distance. (flutter/flutter#184984)
…distance rather than manhattan distance. (flutter/flutter#184984)
…distance rather than manhattan distance. (flutter/flutter#184984)
…distance rather than manhattan distance. (flutter/flutter#184984)
flutter/flutter@8e8a194...2844af6 2026-04-19 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from bWoigpGIb60B6C7hD... to aDbXQm6WA0wFCAUp-... (flutter/flutter#185253) 2026-04-18 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from di3JdYrdE9OFu8Iyl... to bWoigpGIb60B6C7hD... (flutter/flutter#185231) 2026-04-18 rmolivares@renzo-olivares.dev Fix: text selection context menu should reappear after a non-fling scroll on Android and iOS (flutter/flutter#185054) 2026-04-18 flar@google.com Rect constructors for circle bounds (MakeCircle/EllipseBounds) (flutter/flutter#185110) 2026-04-17 97480502+b-luk@users.noreply.github.com Change uber SDF to get pixel size (for AA) using euclidean distance rather than manhattan distance. (flutter/flutter#184984) 2026-04-17 engine-flutter-autoroll@skia.org Roll Dart SDK from 7c2564c18770 to 9648f446f131 (7 revisions) (flutter/flutter#185223) 2026-04-17 47866232+chunhtai@users.noreply.github.com Rewrites no-response workflow to work with pr as well (flutter/flutter#185163) 2026-04-17 15619084+vashworth@users.noreply.github.com Only use LLDB breakpoint in debug mode (flutter/flutter#185158) 2026-04-17 flar@google.com add playground to test SDF primitive rendering under transforms (flutter/flutter#185010) 2026-04-17 30870216+gaaclarke@users.noreply.github.com Adds sdf rrects (and fixes opacity + color source) (flutter/flutter#184999) 2026-04-17 matt.kosarek@canonical.com Implementation of popup windows for Win32 (flutter/flutter#184516) 2026-04-17 engine-flutter-autoroll@skia.org Roll Fuchsia Test Scripts from R2EllDf4DgBXVNuiN... to dQ4PjIJB5kZFU8Y32... (flutter/flutter#185207) 2026-04-17 chris@bracken.jp [iOS] Update previousCompositionOrder to return Obj-C type (flutter/flutter#185136) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC boetger@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…r#11531) flutter/flutter@8e8a194...2844af6 2026-04-19 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from bWoigpGIb60B6C7hD... to aDbXQm6WA0wFCAUp-... (flutter/flutter#185253) 2026-04-18 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from di3JdYrdE9OFu8Iyl... to bWoigpGIb60B6C7hD... (flutter/flutter#185231) 2026-04-18 rmolivares@renzo-olivares.dev Fix: text selection context menu should reappear after a non-fling scroll on Android and iOS (flutter/flutter#185054) 2026-04-18 flar@google.com Rect constructors for circle bounds (MakeCircle/EllipseBounds) (flutter/flutter#185110) 2026-04-17 97480502+b-luk@users.noreply.github.com Change uber SDF to get pixel size (for AA) using euclidean distance rather than manhattan distance. (flutter/flutter#184984) 2026-04-17 engine-flutter-autoroll@skia.org Roll Dart SDK from 7c2564c18770 to 9648f446f131 (7 revisions) (flutter/flutter#185223) 2026-04-17 47866232+chunhtai@users.noreply.github.com Rewrites no-response workflow to work with pr as well (flutter/flutter#185163) 2026-04-17 15619084+vashworth@users.noreply.github.com Only use LLDB breakpoint in debug mode (flutter/flutter#185158) 2026-04-17 flar@google.com add playground to test SDF primitive rendering under transforms (flutter/flutter#185010) 2026-04-17 30870216+gaaclarke@users.noreply.github.com Adds sdf rrects (and fixes opacity + color source) (flutter/flutter#184999) 2026-04-17 matt.kosarek@canonical.com Implementation of popup windows for Win32 (flutter/flutter#184516) 2026-04-17 engine-flutter-autoroll@skia.org Roll Fuchsia Test Scripts from R2EllDf4DgBXVNuiN... to dQ4PjIJB5kZFU8Y32... (flutter/flutter#185207) 2026-04-17 chris@bracken.jp [iOS] Update previousCompositionOrder to return Obj-C type (flutter/flutter#185136) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC boetger@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Our golden tests showed that we were getting slightly non-transparent pixels just beyond the corners of filled rects. This was pointed out In #184395 (comment).
This was due to incorrectly using manhattan distance rather than euclidean distance to determine pixel size for antialiasing. At non-axis-aligned angles (most egregious at 45 degrees), using manhattan distance overestimates the pixel size. This results in AA fading shape edges over a longer distance at the 45 degree edges of a shape compared to the axis-aligned edges of the shape.
This caused:
Changing from manhattan distance to euclidean distance fixes this.
Part of #184352
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.