Overhaul the Curve Editor

[MewPurPur]

Currently has issues. I made this PR as though the problem outlined here - #74927 (comment) - has been solved. As it stands, the PR can still be merged, but it would have a few irritating issues:

• After using Add Point / Remove Point operations, the selected point would get deselected.
• Adding points would corrupt the Undo.

For now, I added a hacky workaround to fix the above, now it's only pronounced in points getting deselected when you release the left mouse button after adding a point. For this to be fixed, #76985 must be resolved.

I disabled the "property_list_changed" notifications of curve to make the recordings below and show how the PR would ideally behave.

Anyway, here's what the PR does:

Refactoring

Splits CurveEditor in two: CurveEditorRect CurveEdit (the area where you edit a curve) and CurveEditor (includes the toolbar).

Context menu removed

The context menu really slows down curve editing. I went for an intuitive control scheme:

• Left click to add a point or drag the point/tangent you're hovering
• Right/Middle click to remove a point or make a tangent linear
• Shift + Left click to drag a tangent individually, like before.

As shown, the presets are moved as a button above the curve editor.

(The recordings below don't show the context menus, probably because the engine treats them as different windows. Use your imagination.)

Old:

Before.mp4

New:

After.mp4

Utilities

Added a snap button with an adjustable number of divisions. Ctrl + Drag tricks were removed.

Shift + Drag on points locks them on one axis.

Utilities.mp4

And as you saw in the previews, a bit of text at the top shows the position/angle of the currently selected point or tangent.

Enhancements + Bugfixes

• Visual tweaks
• Adds hover and selection indicators to tangents.
• Makes it harder for tangents to slip outside of the editable area.
  
• No longer forces new points inside the [0, 1] range (Fixes When adding points in curve editor, they don't appear in the specified y range #41083).
• Fixes bug where pressing Delete also attempts to delete nodes.
• "Hold Shift to edit tangents individually" help text is only shown when relevant.
• Presets now use the full range.
• Hover areas are now rectangular and stretch out a little further than their indicators to reduce the number of misclicks.
• When clicking in a place with multiple elements, selects the closest one. Tangents get priority.
• Improves the logic for keeping handles selected during operations.
• Stops operations from being committed if they don't change anything.
• Prevents points from sharing an X position (important for snapping).
• Fixes the full-size preview missing some pixels.
• Fixes "Remove Curve Point" message when adding a point.
• Fixes situation where points may remain hovered if the mouse cursor leaves the editor.
• Tells which tangent was modified when committing actions (left / right / both).
• Fixes cases where the snapping grid from Ctrl was torn apart at the top and the bottom for certain configurations of min and max values.

Misc

• Adds Curve.get_range() (seems generally very useful, used in multiple places throughout the code)
• Code style improvements (i.e. "p_" before parameter names, sentence-like comments)

[Rindbee]

Until the mechanism for updating the property tree is modified, a temporary solution is to pass the necessary state data by calling the set_meta()/get_meta() methods on the curve object.

[MewPurPur]

I'll look into it, thanks. I haven't really tinkered with meta before.

I'm also trying to somehow make an indicator for the snapping positions, but I can't figure anything out as there already is a grid and having a second one would be too busy.

[Rindbee]

I provided a method in #75030 to predict the index of a point after it has been added.

[MewPurPur]

Sorry for force-pushing so much, I just keep noticing little oversights! I think I've ironed out most things by now, though.

[SlugFiller]

I looked at the animations, and noticed something. When adding a new point, tangents should be placed at a third of the distance to the nearest points, not at constant distances. Otherwise, too often, the tangents will go past the next point, which creates a curve that is non-continuous in x. It was actually a common complaint I heard about the animation bezier curve editor from an animator with experience in Blender and Maya.

[MewPurPur]

The situation you're describing should be rarer now because tangents are a bit smaller. I don't think I can implement this because I don't understand it, for example, what if two points are very close to each other? Would the tangent be shown on top of the selected point? Wouldn't it be confusing to make all tangents differently-sized when their effect is the same?

I think I'm mostly done with features and enhancements in this PR, but maybe we can discuss this on RC so I can add it later?

[fire]

GDquest’s team has an pr with those stated features. We can unify them.

[MewPurPur]

The worst occurrences of the "inspector rebuilding on inopportune moments" bug have been fixed with a hack inside Curve and a FIXME linking to the above issue.

I think it's ready.

[anvilfolk]

Wooooooooooooo, done with this review!

First of all: amazing work! The editor itself looks way better, and the UX of it is wonderful. In most parts, I also find the code easier to read and generally better!

I left loooots of little comments here and there. Some of them are small optimizations or refactorings, others are requests for clarification or extra comments. Many of them don't really need addressing if you don't want to/don't have the time, but there are a couple of more serious points :)

Without going into a lot of detail, since that convo can happen in the comments themselves:

1. get_offset_without_collision() has a loop that I believe is either potentially unbounded, or at the very least, has a possibly very large number of iterations, by moving a potential new point placement location by a 0.0001 magic number, then resetting the loop and doing it all over again. I'm not sure that this function is entirely needed (but happy to be shown wrong!), and if it is, I'd rather a more bounded, if less accurate algorithm for this :)
2. Some of the work that used to be done by the world_trans, view_trans, and _world_to_view transforms is seemingly now done manually by a fairly opaque scaling parameter, which I think makes the code less elegant and harder to maintain. I believe this is to ensure that draw_line can use quad lines, which support anti-aliasing, but I believe there is another way around this that doesn't require eliminating the use of the transforms.
3. If you make some changes to a curve, then select a different curve and undo, it'll undo the changes to the previous curve without them being visible because a new curve is selected. I don't know that UndoRedo provides any features to get around this though :/

For both of these, I'd love to see the alternatives implemented, or to be convinced that this really is the only way to do it :)

Then there's just a couple of UX annoyances:

1. The hover rect doesn't appear centered:
   
2. If you place a new point and keep it selected, pressing will do an axis-snap (not grid-snap) to (0,0), when I believe ideally it would axis-snap to the original point-add position!
   

But damn, all in all I am so very excited to see this get in once these things get addressed! It feels so smooth, and the UX is, as promised, a lot more accessible! I didn't entirely test the undo system, but the few things I did worked. As with any larger PR, I'm sure there's some bugs here that we'll all have missed, but those can be fixed in due time!

Great work, @MewPurPur !!!

[anvilfolk]

Ooooooooooooooooh, very cool! Loving it ❤️‍🔥

[MewPurPur]

Thanks to the review! Here's my review on the main points of your review:

get_offset_without_collision()

Not sure what to do with this function, while I like the utility it gives to add points right next to each other, I agree its implementation is very sus.

Scaling parameter instead of transforms

This is only done for one of the transforms, the one used for drawing the curve, and I document why this transform is so weird. In a nutshell, the curve itself must be transformed, because set_draw_transform() breaks quad lines when the scaling is non-uniform.

This is fundamental, not a bug. Picture a transform that scales something by making it 2 times wider. Such a transform would make horizontal lines 2 times longer, and vertical lines two times fatter. So I don't think there's a better solution if we want antialiased curves.

The hover rect doesn't appear centered

No idea how to address this.

Shift after adding a point is weird

Fixed.

[anvilfolk]

Like I said elsewhere, I think some of these things aren't perfect, but honestly at this point I'm also not sure that they should be merge blockers either ☺️

I didn't do a lot of intense testing, but I tried out what I think are a few worst-case scenarios, and it still performed perfectly well. Once this does get merged, I do have a couple of ideas for how some of these things can potentially be improved - though the proof is in the pudding, and, to push this analogy well past its limits, the oven is only heating once the PR is merged anyway 😜

But in seriousness, the PR has been around for a bit, there aren't any enormous issues, what issues there are have potential fixes that can be done in due time, and if @MewPurPur doesn't have a ton of capacity to keep working on this particular PR right now, I do think it makes sense to merge and continue work later rather than wait until @MewPurPur has time to polish this further.

[QbieShay]

I'd also prefer we merge this since it's definitely an improvement overall, refinement can be done later.

[MewPurPur]

Just a little note about Geometror's request for editing tangents easily: I wasn't sure the idea would be approved, so I didn't implement it here, but I wanted to make a popup for tangent editing that shows up when double-clicking a point. This would be similar to the gradient editor, which allows to edit a color when double-clicking a handle. Either way that would be a separate PR and I can't guarantee I'll manage to implement it in a good enough way to be approved.

[Calinou]

I'm testing this PR locally (rebased against latest master 06ccbfe) and can notice the points are offset from the drawn line:

This only occurs if Min Value and Max Value are different from 0.0. Creating curves in a CPUParticles2D node will automatically set the default Min Value and MAx Value to non-zero values for many properties, such as Radial Accel or Curve.

[anvilfolk]

Thank you for catching that! I don't believe I tested changing the range a lot. Was there anything specific you did (other than min != 0 && max != 0), or does it always happen? Does the same thing happen when editing the curve directly vs editing an "inline" curve on some other resource?

Rebasing everything and will try to do some testing also!

[anvilfolk]

Nope, you're right, can confirm it also happens without the curve being inline:

This might be because of those scaling parameters used to place points or draw lines - two different methods are used to place one vs the other. I think this speaks a little to the need to make changes in the readability/maintainability of rendering. Though as @MewPurPur suggested, due to availability/capacity concerns, the best way to address this might be to fix the bug in this PR and then do a further PR refactoring the refactor :)

[MewPurPur]

Oh man, how did I manage to miss this... TY for catching it, I'm on it.

[MewPurPur]

Phew it's a super simple fix.

[MewPurPur]

Oh, and while testing negative values, I encountered a bug that seems to also be present in the old CTRL snapping, which I hadn't fixed... but now have! So that's cool! This PR fixed like 10 bugs at this point lmao, anyway, documented this stuff in the PR description.

[Calinou]

Tested locally, it works:

PS: In the long term, I wonder if we should preview Bake Resolution in the curve editor by sampling data according to the specified resolution. This way, you can see how much degradation occurs at low resolutions.

[MewPurPur]

After this is merged, the toolbar at the top can easily have its Presets button changed into a dropdown menu with common functionalities.

[akien-mga]

Thanks!

[golddotasksquestions]

This is exactly the kind of UI improvement the Godot Editor needs! Thank You!