-
Notifications
You must be signed in to change notification settings - Fork 6k
[Impeller] Implement drawPoints in Impeller #35204
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| } | ||
| for (uint32_t i = 0; i < count; i++) { | ||
| SkPoint p0 = points[i]; | ||
| SkPoint p1 = points[i] + SkPoint{0.0001, 0.0}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Maybe use kEhCloseEnough as the magic number for this hack (and perhaps a comment mentioning it's needed because stroke caps on 0 length line segments don't really work right)? 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. See flutter/flutter#109077 and the new comment about kEhCloseEnough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there supposed to be overdraw happening for a single drawPoints call? I suspect the answer is yes but just in case: I used a stencil to prevent overdraw from happening when drawing strokes internally, so if overdraw isn't supposed to happen, building/drawing a single path with multiple contours can work here (as opposed to building/drawing several single contour paths).
I said exactly this in the description. This is what drawPoints does. Creating a single path will produce the wrong result, though there might be more optimal implementations that would put all of the lines into a single op and inform the stroking code NOT to do its over-draw optimizations, but that would involve a lot of digging around in the internals. Meanwhile, this dozen or so lines of code does exactly what the op is supposed to do. |
|
Sounds good and I agree: If it matches Skia's behavior we should land it. |

It may seem like this is a broken implementation calling drawLine on individual paths, but that is actually the semantics of the call. We could do these in parallel for performance, but they must behave as if they are individual calls to drawLine - complete with Polygon mode not connecting with joins and all of the individual lines/points/polygon segments over-drawing at their overlap points. The following test case helps show the behavior and shows the same behavior with and without Impeller.
DrawPoints interactive app