LMMS / lmms

Cross-platform music production software
https://lmms.io
GNU General Public License v2.0
8.2k stars 1.01k forks source link

BUG: The tension parameter does not have the expected effect on the curve #4120

Open raindropsfromsky opened 6 years ago

raindropsfromsky commented 6 years ago

I thought that a higher "Tension" in automation curve should make it look more like linear progression, while a lower tension should allow a smoother transition between control points. Secondly, I expected this change to occur throughout the length of the curve.

But this is not so, as can be seen from the following samples: This is the graph with the lowest tension: image

This is the same graph, with the highest tension: image

But in reality, the following unexpected things happen:

  1. Only the left side of the graph is affected more. The right side is hardly affected.
    This is as if we have put multiple nails on a board (where the control points are),
    and then threaded a string around them.
    As we pull the string from left, only its portion on the left experiences the pull.
  2. Increasing the tension actually relaxes the curve, which becomes more rounded (instead of sharper).
  3. With the highest tension, the graph does not resemble linear progression.
tresf commented 6 years ago

In a quick unit test, it seems to ignore the last two points. Is that what you're observing? The last point I believe is intentional, so I'd guess this is a simple off-by-one coding mistake.

raindropsfromsky commented 6 years ago

No on the whole, when I adjust the tension from 0 to 1, I see a little movement only in the left (starting of the timeline). The rest of the curve remains more or less unchanged.

Secondly, the curve behaves exactly opposite compared to what I expect: More tension actually relaxes the curve.

Can I attach a video here?

tresf commented 6 years ago

Can I attach a video here?

You'd have to link it to a video service however GIFs will work.

Ok, I took a closer look at the tension behavior and I think "tension" is the wrong description here (or the values are reversed). Inkscape uses node handles and the length of them determines the width of the curve.

It's been asked before and I think we could benefit from allowing a true zero length curve to obtain a straight line, but we should also extend to the behavior to be able to control both with option to link (default) and unlink.

image

raindropsfromsky commented 6 years ago

I created a random shape, and then varied the tension. The yellow dotted line shows the original curve at tension=0. Note that there is no difference in 2-3, 3-4, 4-5, 5-6 and 8-9 segments at all. (WHY??) image There is a dramatic difference in segment 1-2 (at left). In this case there is some change at right too (6-7 and 7-8).

The general observation is that sag is removed when tension is increased. But then I don't think the algorithm draws the curve based on gravity! :)

raindropsfromsky commented 6 years ago

Ok, I took a closer look at the tension behavior and I think "tension" is the wrong description here (or the values are reversed). Inkscape uses node handles and the length of them determines the width of the curve.

Bingo! I was thinking of InkScape when I made the observations.

In InkScape, the handles are not only made longer to simulate the increased tension, but the handles are turned toward the next/previous control point also. Thus as the tension is increased, the curve more and more resembles the linear progression curve.

tresf commented 6 years ago

Probably a good to-do for @codythecoder, related #2466. Thanks for reporting @raindropsfromsky.

raindropsfromsky commented 6 years ago

image

I tried to add tangents (handles) with their lengths proportionate to the tension.

They do not seem to follow either of the two rules:

  1. When tension increases, the force increases
  2. When tension increases, the handles turn toward the adjacent control point.
musikBear commented 6 years ago

just a comment Changing these curves influence old projects

tresf commented 6 years ago

Changing these curves influence old projects

Yes, any bug fixes need to carefully consider backwards compatibility, but we're not in the business of preserving bugs, so please save the comments for the PRs and the actual testing.

PhysSong commented 6 years ago

I've also looked into the logic and found that the tension parameter affects slopes of tangent lines. If tension is 0, every tangent line should be a horizontal line. If it's 1, a tangent line from a point will be parallel to the line which connects two adjacent points. For implementation details, see AutomationPattern::generateTangents and the logic below. https://github.com/LMMS/lmms/blob/788c990ae12b137b5551e60d44f0ebf62b43e7ec/src/core/AutomationPattern.cpp#L392-L400 Here m1 and m2 stands for starting tangent and ending tangent.

FYI, the Cubic Hermite spline progression is introduced in 6249b23f1ff2740732bf53be89a2d9d4dfd37db9.

raindropsfromsky commented 6 years ago

If tension is 0, every tangent line should be a horizontal line.

Isn't that one of the problems? If you see my actual graph, the tangents are NOT supposed to be horizontal at any time. This will only work if the peaks and troughs are symmetrical around X-axis (such as point#4 in the graph). That is rarely the case.

PhysSong commented 6 years ago

tangents are NOT supposed to be horizontal at any time.

Yes, it may be bad in some(maybe almost every?) situation. Current implementation for tension=0 doesn't look good for me.

I thought that a higher "Tension" in automation curve should make it look more like linear progression, while a lower tension should allow a smoother transition between control points.

As long as we keep using cubic Hermite spline, the only way to change the curve is changing tangents at automation points. Unless LMMS allows discontinuous slope at points for cubic Hermite progression(which is supposed to be have smooth curve IMO), that's almost impossible.

PhysSong commented 6 years ago

I think we can use control points for cubic Hermite spline, as well as the Bezier curve in #2466.