Extend Curve to allow for domains outside of [0, 1].

[anvilfolk]

This PR allows users to make curves in whatever app/game-specific domain they wish 📈 Previously, curves were restricted to a domain of [0,1]. Implements proposal, closes #5607.

Depends on #78931 and #77622, whose commits have been cherry-picked in this PR. Can rebase/remove those commits when/if they get merged. I can also squash them into this PR if preferred. Let me know 🥰

Recording.2023-07-06.141220.mp4

⚠️ This PR also enforces previously unenforced value ranges for points in the curve ⚠️ I tested this and it should not alter existing curve's points when they are loaded ⚠️ Instead, this happens, then is corrected when limits are changed:

Things accounted for

• Curve editor supports extended curves
• Preview generates correctly
• Baking works correctly
• Presets are set correctly in new domains
• Duplicated unit curve test cases adapted to a domain of [-100, 100]
• Documented that other uses of Curve in the engine require unit curves

[anvilfolk]

Compilation is failing due to compiler not knowing whether to use lerp(floats) or lerp(doubles) when real_t is a double. I don't see that warning or error locally. I'm compiling on Visual Studio 2022 with -dev_mode=true, but that's about it. Is there any flag I ought to be using?

I think it also speaks as to whether we should make a decision to use real_t, float or double for these values. Or maybe just not use lerp at all?

[anvilfolk]

Fixed all the issues that were showing up in the checks! I'd love to work on some UI/UX work on the editor itself next , but that only makes sense once this PR is mostly settled :) Looking at godotengine/godot-proposals#5579 and godotengine/godot-proposals#3267, for example!

So LMK if there's any issues or feedback <3

[anvilfolk]

Converted to draft since I'm fairly certain this isn't the best way to do it. Will work on it once 4.0 is released!

[akien-mga]

Is this superseded by #74959?

[anvilfolk]

It is not! :) It still very much is something I want to do, but I believe I have a better approach and I will work on reimplementing it as time allows. Might be a while still.

[anvilfolk]

Finally fixed and reopened this PR! I updated the original post with information.

Couple of changes:

• Opted out of having a unit_curve parameter. This means it's less clutter in the inspector.
• It's been updated to @MewPurPur 's revamp of the curve editor in Overhaul the Curve Editor #74959
• Depends on a couple of other PRs that facilitate this one, but can be squashed into this one if you want, though they are all relatively different issues.

Most of the lines of code are documentation and tests. The changes themselves are relatively small, especially once you remove the other two cherry-picked commits.

I have ⚠️ not tested anything other than the curve editor in the inspector ⚠️ though apparently curves are used in other places. The documentation changes do reflect the idea that most curves used in the engine need to be unit curves so 🤷 😅 ?

I believe CI is passing, I just seem to have a perpetual single build type that fails?

[MewPurPur]

The code that exists LGTM, however, additional changes are needed to make domains play nicely with existing functionality.

The hidden Alt functionality still considers that 0 and 1 are the edges, and so if you use it on a leftmost or rightmost point, it will be forced into that part of the domain.

Snapping doesn't work properly, because it still snaps on the x-axis based on 1.0/snap_divisions.x. When you fix this, remember that you need to do it in two places, once for the dragging logic, and once for the adding a point logic.

Shift for snapping to an axis will also draw the horizontal line only from 0 to 1.

[ttencate]

Does this PR actually need to depend on #78931? Or is that just to avoid merge conflicts down the line?

[anvilfolk]

@ttencate it does depend on it because while the buggy loading of min/max values isn't a big deal when they're not actually enforced, they are and need to be enforced for curve domains. So if the domain numbers load wrongly, the curve will unusable :(

[ttencate]

One way to unblock this is to leave the bug be, and print a runtime error if a curve with an invalid domain is used (e.g. sampled or rendered in the curve editor). The erroneous state only happens when the user sets the domain right after creating a new curve. At that moment, they'll probably enter both min and max right away, and if they don't, they'll notice soon enough that the curve cannot be edited. I don't think that's a big deal. We could even have the curve visualization show something like "Invalid domain".

The advantage is that this leaves the path free for a cleaner solution later on, unlike #78931 which isn't backwards compatible.

[ttencate]

@anvilfolk What do you think of the above idea? I'm willing to implement it and create a PR against your branch if you don't have time for it. I really want this to land in 4.3 🙂

[anvilfolk]

I do think the maintainers will want this to be added cleanly the first time around. Having Godot load your perfectly valid curve with invalid domains that you need to fix manually every time you open it feels, to me, like a no-go.

I did like your suggestion in #78931, so I'd think that's the first step in making this work. I'll see if I can't ping some folks from the core team to take a look!

[ttencate]

Having Godot load your perfectly valid curve with invalid domains that you need to fix manually every time you open it feels, to me, like a no-go.

That's not what I was suggesting. An invalid domain is saved and loaded as-is. You would only get a curve with an invalid domain upon loading if it was saved like that.

[anvilfolk]

My apologies on misunderstanding your point! I'm super tired all the time and that has been happening a lot :(

Am I reading right that the idea then would be to replicate the solution currently used for the range, which has a slight bug, and use it for the domain as well? This way, as you say, loading works OK, but does allow the user to - just once - set a wrong domain?

I think the main issue with this would be remembering to check domain sanity everywhere that it might be used, which would be slightly annoying to do, but definitely possible.

We just had a big discussion in RocketChat about this, in the core channel, and folks had some suggestions, in case you wanted to catch up and maybe implement them :) I'm still finding myself without capacity :(

[Mickeon]

Nothing against the feature at all, but the fact that this note has to be specified into every property that requires this "unit" Curve peeves me a lot.
Calling it "unit" Curve may not be incorrect, but it is a bit "meaningless" as it won't matter much to most readers. It won't matter nearly as much as something like a warning or anything more noticeable and self-explanatory.
Repetition is not just harder to translate but harder to maintain as well.

Worth noting, a similar issue occurred in #85207 . There it was concluded to generate the note automatically, but that may not be as easy here.

[anvilfolk]

Yeah, it is definitely annoying :( Do you have any specific suggestions? The entire engine kind of expects a unit curve because that's what was available so far, and behavior is undefined when non-unit curves are passed to these things.

I don't think we can automate the comments easily, since both curves and unit curves can now be used in different parts of Godot.

I see two possibilities: 1) Add a is_unit_curve boolean to Curve that enforces the domain or, 2) Create a UnitCurve subclass of Curve that enforces the domain. However, both would still require us to change code across the entire codebase to ensure the components that expect unit curves are actually getting unit curves.

[Mickeon]

Some things before getting to the nitty-gritty that are the docs right now because that's my biggest peeve right now.

I think most of what's necessary is specifying "unit [Curve]", without ruining the flow of the sentence.
I feel like changing the "domain" for all of those properties is fairly unlikely and potentially worth noting in the Curve docs themselves. Plus, actually doing it should warrant a warning of some kind (WAY outside the scope of this PR).

[anvilfolk]

Rebased and implemented @Mickeon 's suggestions :)

[fire]

I support having this feature, but I cannot do a complete technical review now.

[anvilfolk]

Thanks for the reviews! Excited to see where extended curves may be used elsewhere in the engine :)

Let me know if you want me to squash the 3 commits (which include the two other ancillary PRs).

[fire]

We prefer squashed commits. For future prs, if you want to separate them, use different pull requests.

godotengine/godot-proposals#11030

[KoBeWi]

They are different pull requests already though.

[anvilfolk]

It was mostly to separate discussion & ease of reviewing. It sounds like reviewers are OK with them, so I'll go ahead and squash, especially since some of the latest changes are in the latest commit but really should've applied to other PRs.

[anvilfolk]

Looks like I messed up the commits, as usual. Will try fixing soon 🤦

-- edit --

Done!