Fix polyline not supporting closed polygons and not having a uniform width

[MinusKube]

trafficstars

The polyline draw method didn't support drawing closed polygons correctly, thus causing graphical glitch, notably for the collision shapes, as seen in #56320 (but this PR doesn't totally fix the issue, as the "snap to pixel" options still cause some glitches).

Also, the generated line didn't have a uniform width, depending on the edge angles:

[rburing]

Could you please explain why this solution is better than https://github.com/godotengine/godot/pull/63425? :)

[MinusKube]

Actually, I think #63425's code is way more readable than what I wrote, but I believe mine cover more edge cases.

In the other PR, closed shapes, like collision ones, still don't close correctly (this is something that I fixed in my PR):

In the other PR, when you get a very acute angle on a Path2D, there's no limit to how far the angle can get (also there's an artifact at the bottom left):

Here is how it render in my version (still not perfect, but at least there's a limit to the angle distance):

I think a good solution would be a combination of both PR :p

[clayjohn]

@MinusKube Thanks for taking a look and comparing. Are you able to update your PR with the relevant bits from the other?

[lmurray]

My main concern with this PR vs the other is that this one allocates temporary data on the heap for every call while the other operates entirely on the stack. Making this one only use the stack may be ideal.

[MinusKube]

Okay I'm working on improving the code of this PR, but I just learned about the LineBuilder class in the engine, and I'm wondering: why would someone use the polyline instead of this LineBuilder? I feel like it serves the same purpose, but the LineBuilder seems more versatile.

[lmurray]

I do think LineBuilder should be promoted to core and reused in a lot more locations as it has the base algorithm for generating a mesh from an arbitrary line. One issue though is that Line2D currently exposes some features of LineBuilder that make little sense elsewhere, e.g. texture UVs, width curve and color gradient, and will just get in the way when making the builder more useful as a generic algorithm. It also is partially implemented as it doesn't support antialiasing feathers. I'm actually using LineBuilder in my own project and it does require some effort to get working outside of Line2D.

There is also the Clipper third-party dependency that is partially exposed by Geometry2D. Clipper has a far more robust line algorithm (it handles self-intersections and the like) but it is also far more expensive to compute which is probably not worth it for generating visuals for physics nodes for example.

[MinusKube]

Okay, I'll do that asap!

[MinusKube]

Here you go!

[MinusKube]

Visually an obvious improvement. Code-wise it looks ok. Fine to merge to me.

I mainly dislike some variable naming like segment directions being called segment, or tangent referring to a vector perpendicular to a segment (tangent to the middle parts of the segment would be rather oriented the same as the segment, and there are infinitely many tangents to the segment endpoints; so used tangent is at least unclear). These unclear namings result e.g. in normalizing already normalized vectors which is redundant. But not gonna bikeshed about these in here because looking at that code I think I'd create a follow-up PR anyway as currently antialiased polylines are always drawn with 9 triangle strips and this can be reduced (I think to 3/5 triangle strips for looping/non-looping polylines). So I could fix the small inconsistencies in there.

Thanks for your feedback, what do you think of segment_dir instead of segment, and maybe bisector instead of tangent (even thought it's not exactly the bisector, but I don't really see what better name I could use) ?

[kleonc]

Thanks for your feedback, what do you think of segment_dir instead of segment, and maybe bisector instead of tangent (even thought it's not exactly the bisector, but I don't really see what better name I could use) ?

@MinusKube Yeah, segment_dir should be clear enough. Regarding tangent: you could have that helper function changed to something like compute_polyline_miter_joint_left_offset_clamped(p_prev_segment_dir, p_segment_dir) to make it super clear. This function is not used in many places so being clear like that should actually be fine. But within the loop I'd keep using simple names. Seeing there's border used for the border offset (and I find it intuitive), then tangent could maybe be replaced by edge (should be clear that's an offset to the edge seeing something like pos + edge + border). Or just left_edge_offset (just edge is probably also quite vague). And base_tangent could become base_left_offset.

What do you think about these? It's not like I'm fully convices to them, feel free to disagree etc. Naming things properly is definitely the hardest part. :laughing: And thanks for willing to change them!

[MinusKube]

@MinusKube Yeah, segment_dir should be clear enough. Regarding tangent: you could have that helper function changed to something like compute_polyline_miter_joint_left_offset_clamped(p_prev_segment_dir, p_segment_dir) to make it super clear. This function is not used in many places so being clear like that should actually be fine. But within the loop I'd keep using simple names. Seeing there's border used for the border offset (and I find it intuitive), then tangent could maybe be replaced by edge (should be clear that's an offset to the edge seeing something like pos + edge + border). Or just left_edge_offset (just edge is probably also quite vague). And base_tangent could become base_left_offset.

What do you think about these? It's not like I'm fully convices to them, feel free to disagree etc. Naming things properly is definitely the hardest part. 😆 And thanks for willing to change them!

I like the edge_offset idea, but I think compute_polyline_miter_joint_left_offset_clamped feels a little overwhelming and hard to understand, but maybe we could apply the edge offset here too, to have something like compute_polyline_edge_offset_clamped?

[kleonc]

I like the edge_offset idea, but I think compute_polyline_miter_joint_left_offset_clamped feels a little overwhelming and hard to understand, but maybe we could apply the edge offset here too, to have something like compute_polyline_edge_offset_clamped?

edge_offset fine; regarding compute_polyline_edge_offset_clamped to me it's not clear that the offset is at the joint between the segments. But mine name is unclear to you. Well, I'll accept what you've proposed too. Better than what we got now anyway. (I'll just remind you I've already approved this PR:slightly_smiling_face:, so I've accepted these even if unchanged. In the end it's not super hard to deduce the meanings from the code. And if I'd be arguing about namings I don't like in the codebase then I'd be arguing all the time:smile:). So please change it to your liking (I think you got the general idea) and hopefully let's get it merged.

[MinusKube]

Here you go, everything is renamed 😊

[akien-mga]

Thanks!