Add closed property to Line2D

[MewPurPur]

Closes godotengine/godot-proposals#4330

Also helps some of the use cases in godotengine/godot-proposals#1126

Simplified Line2D's codebase a bit, as it reinvents a bunch of wheels for no reason (or maybe Line2D is very old and the wheels were invented later). This could help in a potential future rework of these files, which have lots of TODOs comments.

(the fact it's not representing the gradient properly is unrelated to my PR)

Test project I'm using

PuttingLine2DThroughIntenseTrials.zip

[arkology]

Looks like you referenced wrong issue👀

[MewPurPur]

Ah, sorry, I put in an issue instead of a proposal. Bugsquad fixed it for me.

[kleonc]

Some previous behavior is broken, e.g.:

Before	After
	Last segment broken uvs. Broken end cap box.

[MewPurPur]

Added a test project I made.

[MewPurPur]

Addressed the concerns, but this feature is a tougher nut to track than I thought. Sorry, I think I misjudged and put it out way too undercooked. Putting as draft for now until I fix some bugs with the new feature.

p.s. Can I please combine the line_builder and line_2d files? I don't see a reason to not have LineBuilder as a member of line, it's created on every redraw. Or even just as private methods in the Line2D.

[kleonc]

p.s. Can I please combine the line_builder and line_2d files? I don't see a reason to not have LineBuilder as a member of line, it's created on every redraw. Or even just as private methods in the Line2D.

Potentially a good idea but I'd suggest not complicating this PR and making it only add the new closed feature. Code refactoring could be done separately in another PR. Simpler for reviewing, tracking potential regressions, etc.

[MewPurPur]

Well, aside from transparent closed curves being slightly messed up in Bevel and Round joint mode, it works. I changed the gradient to not flip around automatically. If the user wants a smooth transition of shape and color, they should set up the width curve and gradient in a way that achieves this.

Trying to fix those joint modes with transparent lines currently.

[MewPurPur]

Huh, I don't think the check fails are on me. I guess I'll rebase then?

[kleonc]

Note that for a closed line with bevel/round joint modes the joint at the first point is still being drawn twice (would be fine if only halves would be drawn). Visible when not fully opaque:

Outdated.

[MewPurPur]

Unfortunately this behavior is the most reasonable possible way to do this :v

[kleonc]

So the geometry of the line now indeed seems to be fine.

However, there are still issues with the bevel/round joint modes (for closed line).

• For bevel/round joint modes the start/end seam goes through the joint's beginning instead of through the line's first point (like it does for the sharp joint mode).

sharp	bevel	round

• The UVs at the start/end are also incorrect (as already mentioned in Add closed property to Line2D #79182 (comment)). Specifically, I think V = 0.0 line should also go through the first point (just as the seam mentioned in the previous point). Currently for bevel/round modes the whole joint at the first point has V = 0.0.

sharp	bevel	round

---

If these are too hard to fix then I guess:

• This PR could be merged, leaving the fixes for subsequent PRs.
• Edit: it's already like that. 🤦‍♂️ Optionally (if simple enough), the "whole first joint with v = 0.0" issue could be "fixed" in this PR by making the v = 0.0 be at the first joint's start (so it will match e.g. the gradient seam):
  

[MewPurPur]

Sharp joint mode is the absense of a joint; I think the first issue you've brought up surrounds something I basically threw a coin on how to implement: Whether the joint that connects the last line to the first should be the first thing drawn or the last. I made it the first, although I'm now thinking maybe it should be the last.

What you point out to be a broken UV, if I understand you correctly, are actually the leftmost pixels of Godot.svg, since it's the first joint and the line has not yet started (?)

[kleonc]

Sharp joint mode is the absense of a joint; I think the first issue you've brought up surrounds something I basically threw a coin on how to implement: Whether the joint that connects the last line to the first should be the first thing drawn or the last. I made it the first, although I'm now thinking maybe it should be the last.

What I'm suggesting is that the joint at the first point should be handled as half-joints at the start/end, not as the whole joint either at the start or end. So the start/end seam would go thought the first point / middle of such joint:

What you point out to be a broken UV, if I understand you correctly, are actually the leftmost pixels of Godot.svg, since it's the first joint and the line has not yet started (?)

Oh, my bad! It's already like what I've suggested as a potential meh-fix (:man_facepalming::smile:):

But what I'm suggesting with 2 half-joints is that the texture seem would go through the middle:

[MewPurPur]

The half-joints idea looks quite interesting and should be possible. I'll need some extra logic, but at least no fake pass to only draw a single joint, instead it will be half a joint before the segments drawing logic, half a joint after. I'll also need to modify arc-drawing to be splittable in two.

[MewPurPur]

Well, I added the following to the docs of the new property:

Note: The joint between the closing segment and the first segment is drawn first and it samples the [member gradient] and the [member width_curve] at the beginning. This is an implementation detail that might change in a future version.

I'll do some benchmarking now. I implemented a few safe little optimizations so I don't expect the performance with closed = false to be worse than before.

[MewPurPur]

Performance is good, maybe even better, and there are no regressions.

[kleonc]

I've skimmed through the code. Most things looks fine AFAICT. I'm gonna do some more in-action testing later.

---

Well, I added the following to the docs of the new property:

Note: The joint between the closing segment and the first segment is drawn first and it samples the [member gradient] and the [member width_curve] at the beginning. This is an implementation detail that might change in a future version.

@MewPurPur Looks like a solution to me! 😄 I'm absolutely fine with just documenting that quirk. 👍

[MewPurPur]

Nice catch, I haven't actually tested if my changes fixed the above but I can't see why they wouldn't

[AThousandShips]

Style and docs looks good!

[kleonc]

So today I've finally done some procedural testing against potential regressions, by brute force comparing every rendered pixel for some specific configurations of line parameters (8748 images per tested build = 3 * 3 * 3 begin/end/joint types * 2 widths * 9 different width curves (including no curve) * 6 different line types (UV stretched/tiled, solid color + translucent, gradient + translucent) * 3 different point densities (default points + subdivided twice)). Stuff like:

The results are that there are slight differences between this PR and other versions (which all produce the exact same results between themselves) like: v4.1.1.stable.official [bd6af8e], v4.2.dev1.official [0c2144d], v4.2.dev4.official [549fcce], or some random artifact downloaded from a recent GH action for master (v4.2.dev.custom_build [a79955c]).

However, the mentioned differences are AFAICT negligible. These are either:

• a single pixel "missing" (non-background vs background),
• or some pixels mismatched by approximately no more than 1.0 / 255.0.

Here's an example of a test case with the highest difference count, specifically 34 pixels mismatched where:

• 1 pixel "missing" (pointed by the arrows),
• 33 pixels off by approx no more than 1.0 / 255.0 (can you spot them?:smile::mag:).

I've rechecked the code and the only change I suspect could have resulted in such small discrepancies is that segment intersection check is now done with Geometry2D::segment_intersects_segment instead of a custom method. But haven't verified it anyhow.

Anyway, I doubt anyone will notice anything has changed. 🙃

---

Overall LGTM and seems to be working fine. 👍
(@MewPurPur Thanks for all your patience! 🙂)

[MewPurPur]

Thanks, this PR was lots of fun! Except for the part where width curves bullied me

[akien-mga]

Thanks!