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

Extend Curve and CurveEditor to allow for a domain beyond [0, 1] #5607

Open
anvilfolk opened this issue Oct 17, 2022 · 15 comments · May be fixed by godotengine/godot#67857
Open

Extend Curve and CurveEditor to allow for a domain beyond [0, 1] #5607

anvilfolk opened this issue Oct 17, 2022 · 15 comments · May be fixed by godotengine/godot#67857

Comments

@anvilfolk
Copy link

anvilfolk commented Oct 17, 2022

Describe the project you are working on

2D semi-realistic flight simulator using coefficient of lift curves to define wing lift.

Describe the problem or limitation you are having in your project

Coefficients of lift are a function of angles of attack (AOA), which range over [-180, 180]. In order to use the CurveEditor to define these coefficient of lift curves for different airfoils/airplanes, I would have to manually guesstimate where in [0, 1] a specific AOA like -10 or 6 would be before adding a point in the CurveEditor.

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

This is a proposal to extend the hardcoded domain of Curve to user-defined limits. This is already possible for its range, but not its domain.

This allows anyone to define curves over the domain that makes sense for their own application. Coefficients of lift based off AOA, acceleration based off current speed, a spring's pushback depending on current compression, jump height over time, etc etc.

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

We could modify and extend the Curve class to generalize it to arbitrary real_t domains. We would expose those min_x and max_x properties to users through the inspector, like min_y and max_y already are.

Most of what needs to happen everywhere for this to work is already implemented in the Y coordinates. We just need to implement it for the X coordinates.

I have noticed that Curve is used in a few other places (e.g. animations, particles), so there is an alternative that leaves Curve alone so as not to break compatibility or cause unwanted issues/confusion. We could e.g. rename Curve to UnitCurve and subclass it with a new Curve class containing the extra methods; or subclass Curve into ArbitraryCurve or some other suitable name.

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

It is possible to write scripts that convert values from an arbitrary domain into the unit domain [0,1].

However, this wouldn't allow someone to directly modify a Curve (its points and tangents), through the CurveEditor, within the domain that is relevant to their application.

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

We would be extending the functionality of something already existent in core, so it doesn't make sense to reimplement the same (slightly extended) feature in another language and have it be an option in alternative to something already in core.

@anvilfolk
Copy link
Author

It is going well so far! Currently able to change domain at will and change curve points within that domain. Lots of QoL stuff to do though :)

curve-extended.mp4

@Calinou
Copy link
Member

Calinou commented Oct 19, 2022

This makes me wonder if Gradient should also be able to have its domain changed, so you don't have to remap() when reading its value from a script. Gradient is effectively a 1D color cuve, when you think about it.

@anvilfolk
Copy link
Author

anvilfolk commented Oct 19, 2022

I like that too! I think anything that allows people to define gameplay without having to dig into scripts can be very useful. This way programmers can set up the system, and gameplay or graphics designers can use the editor to visually & interactively test out what works best, using only the inspector.

My main worry right now is making it easy to break other parts of Godot. For instance, what happens if we extend Curve to allow arbitrary domains, then someone uses arbitrary domains in curves inside the animation modules, or the particle system?

I was hoping we could avoid a subclass, because that's more "stuff" out there for people to choose from and that's annoying. We could set up a toggle called "force unit curve", but people would still be capable of disabling that. We could just reflect the unit curve assumptions in the documentation.

Or, we could modify the editor/inspector plugins of those other systems to only accept unit curves, and/or having a "force unit curve" option available upon creating a curve that disables any features around changing the domain.

Would love people's thoughts! :)

@Calinou
Copy link
Member

Calinou commented Oct 19, 2022

Or, we could modify the editor/inspector plugins of those other systems to only accept unit curves, and/or having a "force unit curve" option available upon creating a curve that disables any features around changing the domain.

This can probably be done, but it'll require bespoke work on the editor (and adding a new property hint such as PROPERTY_HINT_CURVE_UNIT_DOMAIN).

@anvilfolk
Copy link
Author

This can probably be done, but it'll require bespoke work on the editor (and adding a new property hint such as PROPERTY_HINT_CURVE_UNIT_DOMAIN).

It sounds like as a first implementation, the best option is to keep Curve's default behavior as a unit curve for the sake of backwards compatibility to avoid introducing any odd behaviors. The unitcurve toggle would still be there, but we can add a hint to remind folks to check docs for whether unit curves are necessary where they are using them.

In the future, it might be possible to implement changes to all editor parts currently using curves to force unit curve behavior without allowing the curve to be toggled off unit curve.

@NotAFile
Copy link

I've been forced to make my own curve Resource which provides min and max values for remapping, but obviously in terms of UI this is suboptimal. Having it natively would be great.

@anvilfolk
Copy link
Author

Thanks for voicing that! It's my understanding that this will be punted to 4.1 since right now the priority is bugfixes and not new features. I'm hoping to also do a UI/UX pass on the Curve Editor, so please let me know of any features you'd like to see!

@QbieShay
Copy link

Hey @anvilfolk !

I stumbled on this today and I was wondering if you think it could be related to another "issue" I have.
When I do particles, it's not very clear to understand at which point of the particle lifetime the curve and the points of the curve are. I was hoping to have a seconds value on the X axis instead of the 0-1 range, and that the points would move and resize depending on the lifetime (so that if my particle should reach max growth at 0.1s, it will do it always regardless of how i change the lifetime (unless i go below 0.1s, ofc)

Do you think that this usecase is compatible with your work?

@anvilfolk
Copy link
Author

anvilfolk commented Nov 17, 2022

That's an interesting one, @QbieShay! Right now, the particle system internally assumes the curves always exist in the [0, 1] domain. So we either:

  1. Overhaul particle systems so the curves' actual domains match the respective property, e.g. if particle lifetime is 4 seconds, the curve domain is [0, 4], or
  2. Allow the editor to have a customizable displayed domain, keeping the underlying actual domain as [0, 1].

If I'm not mistaken, changing particle systems to allow for extended curves would break backwards compatibility, and we are past the ability to do that for Godot 4. I believe option 2 might be possible though! This would be done entirely in the UI/UX pass for the curve editor, not directly with extended curves! But I can add it to the to-do list :)

@sergeypdev
Copy link

sergeypdev commented Nov 24, 2023

@anvilfolk another idea for option 2: add another set of functions to sample the curve in domain space, this way:

  • backwards compatibility of resource data is preserved
  • editor can show scaled values improving UX
  • old code relying on existing sample functions will get normalized values like before
  • new code can take advantage of scaled domain

@QbieShay
Copy link

QbieShay commented Nov 24, 2023

Yes. I think we can solve this with an editor plugin or a special type of curve. I think I'd like to prototype this first before jumping into CPP Godot code. I am realizing now it's a bit more complex than I imagined, Ang it's better to take some time to design it properly

@anvilfolk
Copy link
Author

I should have a bit of time to work on this again!

The current implementation in #67857 does make every single curve into a curve with arbitrary domain, and I've always been a little worried about the effect this might have on parts of the engine that use curves, like animation editor, particles etc, especially if people don't know they're supposed to be using unit curves.

While it's not hard to have a toggle for showing scaled values in the curve editor, I think the main issue with @sergeypdev's suggestion is that you wouldn't be able to edit points in domain space in the inspector. At least not unless there's another toggle button that I think might make the UX confusing, unintuitive and prone to user errors.

To make sure to fully preserve all current functionality with curves and not introduce regressions, I am starting to think that we can refactor Curve slightly and then provide this functionality in a subclass called DomainCurve or something like that?

@QbieShay would that address your concerns?

@QbieShay
Copy link

Hey @anvilfolk !

I am not sure anymore that curves per-se should offer the functionality that particles need, in fact i believe it should be an editor plugin specific for particles that manages that. If there's a special curve type with wider domain it could help, but in the end curves are sampled in the 0-1 domain when they're converted to curve textures anyway, so I have to find a different approach.

I think the best for now is to remove the particle from the equation and have a class that serves the primary usecase instead.

Regardless, thanks a lot for considering it and keeping me in the loop <3

@anvilfolk
Copy link
Author

I see! Before changing too many things, I'll try to also check with animation folks on whether this change would be an issue for them! If it's not, perhaps I'll just fix the merge conflicts :)

@henryherkula
Copy link

I want to present another example/problem: This functionality would help me with a time based trading game, in which every natural number on the x axis represents the beginning of a new day. In this scenario the curve defines the price of a good changing over time. Without this functionality it is a little bit awkward to define the curve, when the beginning of the day is a fraction.

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.

6 participants