[Impeller] geometry changes to support line/point style. #56340
Conversation
impeller/geometry/path_component.h
Outdated
| @@ -71,7 +71,8 @@ class StripVertexWriter : public VertexWriter { | |||
| class GLESVertexWriter : public VertexWriter { | |||
| public: | |||
| explicit GLESVertexWriter(std::vector<Point>& points, | |||
| std::vector<uint16_t>& indices); | |||
| std::vector<uint16_t>& indices, | |||
| bool line_strip = false); | |||
There was a problem hiding this comment.
Can you add a docstring for what line_strip is? Maybe just link to tessellator's documentation.
impeller/tessellator/tessellator.h
Outdated
| /// @return A point vector containing the vertices in triangle strip format. | ||
| /// Matrix::GetMaxBasisLengthXY of the CTM applied to | ||
| /// the path for rendering. | ||
| /// @param[in] line_strip if true, generates line strip geometry instead of a |
There was a problem hiding this comment.
What is a "line strip"? A triangle strip that makes a line?
There was a problem hiding this comment.
A line strip is a GL_LINE_STRIP geometry. How about a link to the primitive type?
There was a problem hiding this comment.
I don't understand why a tessellator would be generating a line strip, tessellation means converting something to packed polygons (triangles in our case). I see, this is doing stuff like converting a bezier path to a line strip?
Yea, mentioning GL_LINE_STRIP would help to distinguish it from a line make with a triangle strip. Especially since a generating a GL_LINE_STRIP isn't technically tessellation.
The brief for this method also explicitly says it creates a triangle fan structure, that should be updated.
Now that I think of it. Don't you think this would be better off as it's own function instead of a bool parameter? Generally it's advised to make procedures do 1 thing. We can share the code in a helper function.
There was a problem hiding this comment.
Yeah I agree with you, a separate method would be better in this case. I suppose since its a line primitive its not exactly tessellation but primitive generation :)
| point_buffer.GetBuffer()->Flush(point_buffer.GetRange()); | ||
| index_buffer.GetBuffer()->Flush(index_buffer.GetRange()); |
There was a problem hiding this comment.
Is there a test for these changes?
There was a problem hiding this comment.
I don't have a test case for this, except for the fact that it may or may not work on swiftshader. I don't want to add a mock test for these, instead I think switching to the point arena like we did for strokes is probably better?
There was a problem hiding this comment.
So, you're saying if this wasn't here the goldens would look wrong? Sounds good to me.
There was a problem hiding this comment.
It would be racy :(
jonahwilliams
added
the
autosubmit
|
auto label is removed for flutter/engine/56340, due to - The status or check suite Mac mac_ios_engine has failed. Please fix the issues identified (or deflake) before re-applying this label. |
auto-submit
bot
removed
the
autosubmit
|
|
||
| std::pair<size_t, size_t> GetVertexCount() const; | ||
|
|
||
| const std::vector<Point>& GetOveriszedBuffer() const; |
Split off from #55230 . Adds getter that tracks if a Path is a single contour.
################################
flutter/flutter#152702