Generally I'm fine with this node, but I think we shouldn't prioritize merging it for 3.0. Maybe afterwards.
The input socket name "Time" seems a bit odd.

In D12035#337318, @Jacques Lucke (JacquesLucke) wrote:

Generally I'm fine with this node, but I think we shouldn't prioritize merging it for 3.0. Maybe afterwards.
The input socket name "Time" seems a bit odd.

Yes, the variable names may need some generalisation.

Time = Value
Begin = Start
Change = End
Duration, Amplitude, Period and Overshoot remain as they are.

I think what Jacques meant in the inline comment is that we should avoid adding these large templates to FN_multi_function_builder.hh

I think either templating the operation function or using a larger function with the direction and operations as inputs would be the way to go.

In D12035#349541, @Hans Goudey (HooglyBoogly) wrote:

I think what Jacques meant in the inline comment is that we should avoid adding these large templates to FN_multi_function_builder.hh

I think either templating the operation function or using a larger function with the direction and operations as inputs would be the way to go.

Lol, I read it as a response to your comment and he was fine with it as it is and look at this in the future. I'll take another look.

If only the creator of these functions had written a book, and within that book had written a chapter that covered said functions, and made that chapter freely available on his website. That sure could clear up what the input variables represent.

Oh wait, he did!

http://robertpenner.com/easing/penner_chapter7_tweening.pdf

@Evan Wilson (EAW) The variable name changes are suggested and implemented as part of the code review. Easing is also a type of interpolation so can be represented using non-time based names. I'm not strongly opinionated on this.

I haven't posted this as a revision but this uses a function to get the easing function used in the loop to reduce boiler plate code. P2602

We discussed this in the latest geometry nodes sub-module meeting. Here are the notes from that:

• The inputs and functionality are very similar to the map range node.
• Conceptually the node feels different though, and there are also in and out options, which probably makes it worth adding.
• The clamp option could be named differently, since it clamps the input rather than the output.
• Or the clamp option can be removed, because there doesn't seem to be a strong use case for it and the behavior seems expected.
• There could be more settings for the sinusoidal modes:
  • Animation Nodes has a useful set of interpolations: https://docs.animation-nodes.com/documentation/nodes/interpolation/
  • Number of bounces, and an attenuation for how quickly the bounces fall-off for example.

The idea with the last comment is that we should take advantage of the more specific behavior of this node and expose more settings where it makes sense.

For the code, I think rather than using a function pointer variable, it might make more sense to add a function that applies the operation with the function as a template parameter. I don't think the compiler will optimize this code using function pointers as well.

This is a big node, so I have a couple of remarks:

• I'm not sure how useful the Mirror input is. It kind of undermines the behaviour of easing from a start value to an end value. The same behaviour can be achieved by mirroring the input. I can't really think of a good application other than the pulse curves you mentioned and to me that alone is not a strong enough argument to include it.

• When testing this node, it becomes really apparent to me how crucial the visualization is using the example file. Considering that this is something the user will not have available in practice, it is worth considering how this kind of thing could be visualized. As an example there is the interpolation viewer in animation nodes. Just throwing it out there.

• The behaviour of the power interpolation looks a bit wrong to me. It looks to me like what is Out with exponent=x should be In with exponent=1/x.

• For Slope, it would feel more natural to me if the behaviour of In and Out would be swapped (Offset inverted).

• For Bounce being able to adjust the number of bounces seems quite useful to me, so it would be nice if you could get that to work before the merge.

• In the same sense, I think an Oscillations input would control more intuitively than a period for the Elastic case. I had some trouble what exactly the amplitude is doing, but I think in general it's fine. It just just have a lower soft limit of 1, as it doesn't change anything below that. The dampening shape is something that could also be controlled additionally (Same for Bounce), but I think the natural falloff that it has now is fine.

• The naming of the inputs gives me some issues. In my opinion Start, End and Range aren't very clear at all. Any one of them could be either regarding the input or the output. However I don't have a perfect answer to how they should be called. I would even bring into question what set of input parameters should be defined here. I do think that if we already expose all this functionality, rather than leaving the mapping to external map range node (which I'm fine with), then we should give the same functionality as the map range node has. That would mean an additional parameter to define the From Min/Max.
  
  We can also say that we don't expose all the parameters, but if we define the From Min to always be 0 there is no good reason to expose the From Max as a range imo. The node can both ease in and out, so in and out parameters can be viewed on the same importance level as being symmetrical. So either we expose both or none. I could see the option to only expose the To Min/Max values as inputs and defining From Min/Max as 0 and 1.
  
  As we already use In/Out for the Easing Types, I think using the same language to describe the mapping can make sense. So To Min/Max (Start/End currently) could be called Value In/Out. As for the From Min/Max inputs I think that the language of Start/End seems much more appropriate here, as in many cases an interpolation with easing is associated with time. Again, for these I think we should have either both or none, but that could go either way for me.

@Simon Thommes (simonthommes) thanks for the feedback

What do you think about these changes....

• Remove Range and End... this node then operates solely in the [0-1] range with just a Value input. This reduces the ambiguity and simplifies the node.
• Keep Mirror and rename it to Loop. This name fits better and is great for motion design.
• IN, OUT is accepted terminology for Easing but when you have adjustable curves it doesn't fit so well. Perhaps remove IN,OUT from some of the adjustable curves. I think this should keep the naming as it is used in IPO curves and is commonly used elsewhere for animation.

This would fit in well with the existing toolset.

• Map Range for mapping values
• Curves for fine visual control
• Mix Node (hopefully) for basic interpolation
• RGB Node for blending values

I will look into the suggestions for Bounces and Elastic again.

In D12035#366474, @Charlie Jolly (charlie) wrote:

• Remove Range and End... this node then operates solely in the [0-1] range with just a Value input. This reduces the ambiguity and simplifies the node.

I assume then you mean Range, End and Start?
Leaving out the mapping for both the input and the output of this node would mean a lot of overhead for the mapping (2 additional map range nodes).
I do see a benefit and strong connection to the functionality of the node in exposing from and to what value to ease. In what input range that happens can be [0,1] imo.
Here the naming of start and end implies a time connection imo. I would propose in connection with the easing types something along Value In/Out or Level In/Out.

• Keep Mirror and rename it to Loop. This name fits better and is great for motion design.

For me a loop implies periodicity. With the current behaviour, I think the name mirror is fine. I don't have a strong opinion on whether or not to include this input.

• IN, OUT is accepted terminology for Easing but when you have adjustable curves it doesn't fit so well. Perhaps remove IN,OUT from some of the adjustable curves. I think this should keep the naming as it is used in IPO curves and is commonly used elsewhere for animation.

I wasn't proposing to change the In/Out terms. You are right, on some curves the changing between these doesn't make a lot of sense. But for consistency I wouldn't be oppposed to keep the options nonetheless.

In D12035#366578, @Simon Thommes (simonthommes) wrote:

In D12035#366474, @Charlie Jolly (charlie) wrote:

• Remove Range and End... this node then operates solely in the [0-1] range with just a Value input. This reduces the ambiguity and simplifies the node.

I assume then you mean Range, End and Start?
Leaving out the mapping for both the input and the output of this node would mean a lot of overhead for the mapping (2 additional map range nodes).
I do see a benefit and strong connection to the functionality of the node in exposing from and to what value to ease. In what input range that happens can be [0,1] imo.
Here the naming of start and end implies a time connection imo. I would propose in connection with the easing types something along Value In/Out or Level In/Out.

For Map Range behaviour, I'm thinking this would just plug into the factor/value input. This node would then become a factor control rather than another map range node. In the same way, this could also drive the Factor of a Mix node.

E.g.

Easing > Map Range value input
Easing > Mix Node factor input

I will look at the visualisation. Agree this is important for the dynamic effects.

I feel bad for having an inconsistent opinion here, but I'm finding myself less convinced this is worth adding as a native C++ implementation at this point.
When it used existing functions there wasn't much of a cost to it, but at this point the patch adds a lot of new code that will need to be maintained.

Implementing this as a node group and shipping it in an asset bundle (T95445, T91843) seems like a preferable option to me at this point, for a few reasons:

• Less code to maintain
• Less code to review (just weighing priorities, I'm not sure I'll have to time to review this code for a bit)
• Much better performance, because we can focus on making the smaller builtin nodes faster with SIMD, LLVM compilation, and GPU calculation. (Doing all that manually in this node would be much harder).
• The design can be more easily changed, and is generally less binding.

The biggest blocker is the lack of enum sockets now. But that should be worked on anyway, since it will help in other areas.

In D12035#372326, @Hans Goudey (HooglyBoogly) wrote:

I feel bad for having an inconsistent opinion here, but I'm finding myself less convinced this is worth adding as a native C++ implementation at this point.
When it used existing functions there wasn't much of a cost to it, but at this point the patch adds a lot of new code that will need to be maintained.

This is an interesting case because the node was pretty close to being accepted and after getting Simon to review it, it kind of muddied the water a bit. As a result I did add a simplified version of the patch here: D13884: Geometry Nodes: Add Easing Function Node (WIP)

Implementing this as a node group and shipping it in an asset bundle (T95445, T91843) seems like a preferable option to me at this point, for a few reasons:

• Less code to maintain
• Less code to review (just weighing priorities, I'm not sure I'll have to time to review this code for a bit)
• Much better performance, because we can focus on making the smaller builtin nodes faster with SIMD, LLVM compilation, and GPU calculation. (Doing all that manually in this node would be much harder).
• The design can be more easily changed, and is generally less binding.

I'd consider easing functions a core node. If they are implemented as an asset bundle, I'm not sure that is less work for maintainability and review.

The biggest blocker is the lack of enum sockets now. But that should be worked on anyway, since it will help in other areas.