Add closed property to Curve3D

[GreenCrowDev]

Should close godotengine/godot-proposals#6396 and close godotengine/godot-proposals#8650

2023-12-16-21-37-14.mp4

TODO:

• Change color of start and end points (especially important when the curve is closed and you can't tell where's the start);
• In and Out gizmo handles display, respectively for Start and End node, when _closed = true;
• In and Out properties don't show up in inspector right now, but should be displayed when _closed = true;
• Close Curve button could become a Closed Curve toggle, so that user can easily access the value through the already nice icon on the top bar of the viewport; Path3DEditorPlugin Close Curve button is now a "Toggle Open/Closed Curve" button that just toggles the curve closed property;
• Documentation;
• Tests (can you suggest some? I'm not used to them);

TODO (optional):

• Draw indices text near the respective point to easily identify them (needs a bespoke solution as at the moment there's not a system in place to draw text near 3D gizmos);

If you have suggestions about what behaviour I should try to check if stuff is broken by this change let me know (for example something like PathFollow3D...).

[GreenCrowDev]

Should I commit the changes to the branch as they are ready or wait to complete all of them? I'm not sure if running CI/CD checks every time is a problem...
Right now I'm adding features to a _dev branch on top of this one.

[GreenCrowDev]

Any idea when to merge this and set a milestone? 4.3 or 4.4?

[AThousandShips]

It hasn't been reviewed or decided on so it's in 4.x until some decision or review has been made 🙂

[Chubercik]

Should I commit the changes to the branch as they are ready or wait to complete all of them? I'm not sure if running CI/CD checks every time is a problem...
Right now I'm adding features to a _dev branch on top of this one.

No one ever reprimanded me for committing to a live PR branch, so I think it's fine - just be sure to squash your commits afterwards (see: Pull request workflow).

[Calinou]

Tested locally (rebased on top of master 29b3d9e), I noticed some issues:

• The start/end color for gizmos only appears at certain camera angles. Perhaps two sprite gizmos are overlapping and only one of them is showing up?

path3d_editor.mp4

This also causes the colors to stop appearing depending on the number of points drawn:

path3d_editor_2.mp4

They show up reliably in a top orthogonal view, which is what leads me to think it's an overlapping gizmo sorting issue:

Also, as an improvement, I suggest drawing the last line that goes from the last point to the first in a different color:

Mockup made in GIMP:

This way, you can tell which line is automatically being inferred by the engine as opposed as being designed by the user.

[GreenCrowDev]

Thanks for the review @Calinou!

Yes, this worked well before #87901, and there's this other PR #89847 that refines Path3D editor, ready to be merged.

Since they work on the same lines of code, I'd like to wait for #89847 to be merged, that defines stuff the lack of which makes difficult to finalize these last tweaks you mentioned.

Is that okay? 😊

[Chubercik]

Since they work on the same lines of code, I'd like to wait for #89847 to be merged, that defines stuff the lack of which makes difficult to finalize these last tweaks you mentioned.

[GreenCrowDev]

I fixed the handles issue @Calinou.

Also fixed handles color in the curve edit mode, after the rebase.

I didn't implement the last segment color change though, as I realized that it would require some decent work, since we don't have access to control points but the baked ones. Eventually to add in a later PR?

[GreenCrowDev]

Changes implemented.

Also fixed a couple of bugs.

Not sure why it fails the check on Linux 🤔

gpg: keyserver receive failed: Connection reset by peer
Error: Process completed with exit code 2.

[AyyZerrAsa]

I fixed the handles issue @Calinou.

Also fixed handles color in the curve edit mode, after the rebase.

I didn't implement the last segment color change though, as I realized that it would require some decent work, since we don't have access to control points but the baked ones. Eventually to add in a later PR?

What do you mean in regards to not having access to control points? I made some changes in #90357 that caches baked distance at each original control point. Not sure whether that's relevant here. Great commit btw I was needing this, super suprised closed curves weren't a thing already.

[Calinou]

Tested locally (rebased on top of master 79de2ea), it works as expected. I think this is in a good state to be merged early in the 4.4 cycle.

path3d_closed.mp4

[GreenCrowDev]

Thanks for the review!
I applied the suggestions and rebased 😊

[GreenCrowDev]

Rebased :)

[akien-mga]

This seems fine overall to me, but I question the usefulness of the toggle_closed method, which is equivalent to closed = !closed. Is this used often enough to use a dedicated method? I note that Line2D has a closed property but no toggle_closed method.

[GreenCrowDev]

This seems fine overall to me, but I question the usefulness of the toggle_closed method, which is equivalent to closed = !closed. Is this used often enough to use a dedicated method? I note that Line2D has a closed property but no toggle_closed method.

I honestly don't recall why toggle_closed is there, I guess I'll remove it and force push again.

[Repiteo]

Thanks!