Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a cyclic property to Curve2D and Curve3D to make them closed automatically #6396

Open
ettiSurreal opened this issue Mar 2, 2023 · 8 comments · May be fixed by godotengine/godot#86195
Open

Comments

@ettiSurreal
Copy link

ettiSurreal commented Mar 2, 2023

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 Calinou changed the title Add a cyclic property to Curve2D and Curve3D Add a cyclic property to Curve2D and Curve3D to make them closed automatically Mar 2, 2023
@Calinou
Copy link
Member

Calinou commented Mar 2, 2023

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
Copy link
Author

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
Copy link

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

@ettiSurreal
Copy link
Author

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
Copy link

GreenCrowDev commented Dec 14, 2023

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

2023-12-14-21-42-38.mp4

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

  • In and Out handles display, respectively for Start and End node, when _closed = true;
  • In and Out properties in the inspector (same as above);
  • 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;
  • Documentation;
  • Tests (can you suggest some? I'm not used to them);

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
Copy link
Member

Calinou commented Dec 14, 2023

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
Copy link

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
Copy link
Member

Calinou commented Dec 15, 2023

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.

@GreenCrowDev GreenCrowDev linked a pull request Dec 15, 2023 that will close this issue
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants