godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

Add a `cyclic` property to Curve2D (already implemented in Curve3D) #6396

Open ettiSurreal opened 1 year ago

ettiSurreal commented 1 year ago

Curve3D done as of https://github.com/godotengine/godot/pull/86195.

Describe the project you are working on

Testing!

Describe the problem or limitation you are having in your project

The current way the engine handles closed/cyclic curves makes them very hard to work with This implementation also seems pretty hack-y, and is not how most other software do it.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Add a cyclic property to Curve2D and Curve3D, that toggles if the curve is closed/cyclic. This is how most game engines and software handle it.

Could also be named closed

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Unlike now, the first and last points in the Curve2D and Curve3D point array would now have both In and Out positions. Maybe hide the unused handle when cyclic is off?

Setting cyclic to on makes the first and last point connected, now without making a new point to connect them. Attempting to add a point at the end of the curve in the path editor sets the spline to cyclic instead of adding a new point. Close Curve (rename to Toggle Cyclic?) in the path editor would now simply toggle the property.

If this enhancement will not be used often, can it be worked around with a few lines of script?

You could probably make a script that clamps the last point to the first, to make it easier to edit, but this proposal is about changing the node behavior to make it work like this out of the box.

Is there a reason why this should be core and not an add-on in the asset library?

Improving the editor usability out of the box.

Calinou commented 1 year ago

I think closed is more intuitive than cyclic (or even a path_mode enum property that can be Open or Closed).

Close Curve (rename to Toggle Cyclic?) in the path editor would now simply toggle the property.

I think the dedicated button in the top bar could be removed if a closed property is added, since you can toggle that property with a single click (and it'd be reversible).

ettiSurreal commented 1 year ago

I think closed is more intuitive than cyclic (or even a path_mode enum property that can be Open or Closed).

I took cyclic mostly since it's the first thing that came to my mind (I'm used to Blender), closed also works and I've seen it used in Unity and Unreal.

I think the dedicated button in the top bar could be removed if a closed property is added, since you can toggle that property with a single click (and it'd be reversible).

Idea was to turn it into a shortcut in case people would find doing it through inspector too cumbersome, but i imagine removing it wouldn't have much impact in the long run anyway.

GreenCrowDev commented 11 months ago

I opened a new proposal you'd want to check @ettiSurreal https://github.com/godotengine/godot-proposals/issues/8650 What do you think?

ettiSurreal commented 11 months ago

I opened a new proposal you'd want to check @ettiSurreal #8650 What do you think?

Recently I've been using looping curves a lot, and it's not a pleasant experience because of lack of proper support, I would prefer a cyclic/closed property though.

GreenCrowDev commented 11 months ago

Here we go, I implemented the _closed curve property in the proper way, following pretty much https://github.com/godotengine/godot/pull/79182 as @Calinou suggested. This is how it looks:

https://github.com/godotengine/godot-proposals/assets/62719360/ab5e1324-2f7d-4461-b0d2-e1281430b9fe

The lag is a video recording issue. Next I will address:

When would be the right time to open a pull request? Should I complete all of the talked about features before opening one? Should I also work on Curve2D or that's for another PR?

Calinou commented 11 months ago

When would be the right time to open a pull request? Should I complete all of the talked about features before opening one?

You can open a draft pull request now, or complete the features you listed before opening your PR. Opening a draft PR can be useful if you're looking for help to complete the feature.

Should I also work on Curve2D or that's for another PR?

I'd leave it to a separate PR so we can iron out any issues in the Curve3D PR first, and then replicate the design for Curve2D.

GreenCrowDev commented 11 months ago

Okay, I'll open a draft when I'm ready. I was also thinking, now that the curve can be closed, probably would be a good idea to distinguish between the Start/End points and the other ones, otherwise it's impossible to tell where the path would start. I was thinking we could simply do another color, or maybe start displaying indices numbers near the handles of the points. I like the indices idea since it would make easier to identify and move points through code or inspector (I'd like to make appear also the indices of the points in the inspector property, right now they are pretty confusing and you can't tell what you're modifying). Eventually I can just implement another color (violet like the display path?) for the Start/End and then add the indices in another PR if you like the idea.

Calinou commented 11 months ago

I was thinking we could simply do another color,

Sounds good. I'd probably go with green for the first point, red for the last point, yellow for the first point if it's a closed curve.

I was thinking we could simply do another color, or maybe start displaying indices numbers near the handles of the points.

This is a good idea! We currently don't have a system in place to draw text near 3D gizmos yet, so this would require a bespoke solution. Note that we can't create additional nodes to draw gizmos, so Label3D can't be used here.

ettiSurreal commented 13 hours ago

Note that that only added it to Curve3D, while this proposal also specifies 2D, so this is not complete yet.