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

Changing keyframe interpolation type incorrectly changes other keyframe interpolation types #3823

Closed
rexdk opened this issue Nov 5, 2020 · 30 comments
Labels
🐞 bug A bug, error, or breakage of any kind stale This issue has not had any activity in 60 days :(

Comments

@rexdk
Copy link

rexdk commented Nov 5, 2020

I add 4 keyframes for (say) Location-Y to a single clip - we'll call them (in time order) A B C D.

By default the keyframes all interpolate as linear, and the clip moves up or down correctly.

If I change C to bezier interpolation, A and B both change to bezier interpolation. D stays linear. (and the bezier interpolation happens in section A-B and B-C). That's wrong.

There are many other similar unwanted interactions like this - just giving one example.

The user guide does not say (that's why I'm playing with it) - but I think that setting the interpolation type at C applies that type of interpolation to the B-C section.

For example setting 'constant' at C means the value when at B stays unchanged (constant) until C when it jumps the new value.

Workaround: check neighbouring keyframes and reset them as needed, maybe iteratively.

Win 10; OpenShot daily build from 29Oct20 (but I think I've noticed it on the production version but not nailed it as a bug)

@rexdk rexdk added the 🐞 bug A bug, error, or breakage of any kind label Nov 5, 2020
@rexdk
Copy link
Author

rexdk commented Nov 8, 2020

Looks like #2552 is relevant to this.

I think that setting the interpolation type at C applies that type of interpolation to the B-C section.

Goodness - looking at issue 374 in the cpp library I'm now not so sure! Would useful to know what was eventually implemented, so I can add it to the docs - can't quite figure it out.

@ferdnyc
Copy link
Contributor

ferdnyc commented Nov 8, 2020

@rexdk Oh, good, you found those. I was going to link you to those discussions, but you beat me to it. Perfect.

As for how things work...

If I change C to bezier interpolation, A and B both change to bezier interpolation.

That's... correct. And necessary at least for B (for weird implementation reasons) — for A, far less clear, which has always been the crux of the issue.

D stays linear. (and the bezier interpolation happens in section A-B and B-C). That's wrong.

Well... you need to be careful about "wrong", as some of those interactions are as-intended, for the most part, and come down to quirks in the design of the keyframe implementation(s). (Two of them, between libopenshot and OpenShot, which is also part of the issue.)

It's certainly fair to say that a lot of these side-effects are "unexpected", though.

Heck, as you probably have already seen, when I puzzled out how it all works I described it point-blank as:

more than "somewhat" odd. It's extremely odd, to be honest, and not at all what I'd have initially expected — if anything, I'd expect that setting interpolation at a given point would change how the values are computed after that point, not until that point.

But, it is what it is, and fundamental changes to how things work are probably out of reach, at least if the change would result in users' existing projects no longer being interpreted the same way by OpenShot. Though that doesn't necessarily apply to anything that could be altered without affecting project data created prior to the change. (Which in practical terms means changes to how the data is created and modified, only — not how it's interpreted.)

(Any possibility of maintaining two implementations in parallel, one for older data and another for newer, I'm just throwing out as a non-starter before we even go down that road. Maintaining one working implementation of various OpenShot features is enough of a strain on our resources as it is, I don't think anyone's going to come forward and volunteer to shoulder two.)

As far as docs go... I suspect much of this was intentionally unaddressed in the docs, because it's weird and scary and turns most users off. The design goals for OpenShot usually present it as aiming for a higher-level view of the project: Rather than setting keyframes for various properties, you use the interactive Transform tool, or you right-click on a clip and choose a "Fade out" or "Animate" preset, and OpenShot (in theory) takes care of all that for you. No worrying about scary things like "key frames", never mind concepts like "interpolation".

Heck, though they're mentioned at a couple of other points in other sections, the ENTIRE section in the manual that covers keyframes consists of this:

image

Which... technically is an explanation, but most likely one that's already lost the non-techie set, and is far too brief and handwavey for anyone who actually wants to understand anything.

Of course, documenting how something works also forces you to (a) come up with a coherent notion of what, precisely, that actually would be, and (b) figure out how to express it in concrete terms. When that isn't done in text, the danger is that it won't be done in code either, and I think we're suffering from some of that effect in certain areas.

(Because... I have no idea how the Time property implementation came about, or who designed it that way — if it even was actually "designed" at all — but the idea that anyone could have sat down and deliberated over the problem at hand, and then ultimately concluded that the current implementation makes sense and is a good solution to that problem... is a notion I find deeply, deeply troubling.)

@ferdnyc
Copy link
Contributor

ferdnyc commented Nov 8, 2020

if it even was actually "designed" at all

By which I mean, I would have absolutely no trouble believing that it's cursed code someone got off a floppy disk they found inside the drive of an old Apple ][, one that's been buried for 30 years deep in the second sub-basement of a haunted computer museum.

@rexdk
Copy link
Author

rexdk commented Nov 8, 2020

Arf! Now I've understood the Time property, I do rather like it :). But I'm sure it's almost never used - because there is no documentation! (apart from mine)

There is an unresolved problem to address with the documentation, which is: how do you make it easy - and also describe the powerful bits.

I've mocked up a 'simple user guide', with an 'advanced manual' running in parallel, chapter for chapter. Technically it works - you can get hold of the simple or advanced info. But it doesn't read well enough to me - too many choices for the beginner.

Haven't really found a good solution yet. I've been feeding @USATechGuy ideas, but he's not on the case at the moment.

You've got to describe the powerful bits, or no-one will use them and they may as well be removed. You've got to keep them away from the beginner, who would freak out.

@rexdk
Copy link
Author

rexdk commented Nov 8, 2020

Back to topic: I've been trying to make sense of the code - which is not easy!
It looks to me as if a keyframe point has a single interpolation-type value, but needs two - one for the left side and one for the right side. I can't figure out what OpenShot saves, but maybe the existing interpolation-type value could be relabelled as left, and the right side type kept up to date automatically, just a copy of the next keyframe's lefthand interpolation-type.

I have a feeling my current documentation on this reads like a farce, which is not good, but the user needs to know some way to use the feature.

It's certainly misleading to describe the bezier curve as 'quadratic'. The implementation is a cubic bezier, using bernstein polynoms - powers of 3.

We talked about beziers elsewhere, and you said I would have fun... But I do understand them all now, and am likely the only person on the planet who knows how to achieve a "constant power audio crossfade" in OpenShot. I've written it up with graphs, but it is definitely not for the squeamish.

Ah - I've just had a ghastly thought - if the first keyframe is set to linear and the second to bezier - does it implement it as bezier - or does it start off with a linear section - a bezier with P1 being the 0,0 or 1,1? That would be horrible, but it didn't occur to me to check that!

@ferdnyc
Copy link
Contributor

ferdnyc commented Nov 9, 2020

Ah - I've just had a ghastly thought - if the first keyframe is set to linear and the second to bezier - does it implement it as bezier - or does it start off with a linear section - a bezier with P1 being the 0,0 or 1,1? That would be horrible, but it didn't occur to me to check that!

The interpolation type of the end keyframe defines how a segment is interpreted. That's one of the things I was referring to as "extremely odd" in that bit I quoted above, because you'd expect that e.g. a constant keyframe would start constant interpolation that lasts until the next keyframe, not the other way around.

I gather it was done that way so that, if you created a keyframe somewhere in the clip's runtime, you could set an interpolation type and it would automatically apply across the video up to that point, effectively "overriding" the automatic one at the first frame.

I don't personally think that was the right call, but there's a certain logic to it and I get the decision.

But it makes things weird, and it forces you to go chasing keyframes in other, unexpected ways. Like, if that new, middle-of-clip keyframe you set is a Constant one, it defines that value for the entire runtime right? (Parameters always hold constant past the last defined keyframe point, so a single Constant keyframe effectively means constant interpolation on both sides.)

Well, no, because the value that'll be Constant before that keyframe is the initial one from the first frame. So if you really want a Constant value on any segment, you have to set a Constant-interpolation keyframe at the end, and put a keyframe with the value at the beginning. That part... is a design bug, IMHO, if not an implementation bug.

My personal take is that there shouldn't be automatic first keyframes at all. There should instead be default values. All keyframes should act as modifiers from those defaults, and only be present on a property that's been modified. That would allow you to reset a property simply by deleting all of the keyframe points on it, something there's no way to do today once you've modified the value at the automatic first keyframe. Today if you delete all of the points for a property, the property just goes to 0. If that's not a valid or reasonable value, things break.

That's something we actually could implement without breaking old projects. Since they have "unnecessary" initial keyframes on every property, they'll continue to produce the same values as always — they'll just ignore and override the (new, real) defaults.

It would still be quite a bit of work to get libopenshot to the point where it didn't need keyframe points defined for every property, and could just apply defaults unless told otherwise. But it would make the project data a lot cleaner, even if we didn't feel safe leaving the default values out of the file entirely. (And, there's some argument for including them in the project data, so that changes to those defaults down the road don't impact existing projects.) But even if they were included, they could be stored as just a dictionary of values, rather than a set of default keyframe points.

That's potentially the difference between this:

{"clips": [ 
  { "id": "ABCDE12345",
    "scale_x": {
      "Points": [ 
        { "co": { 
           "X": 1.0, 
           "Y": 1.0 },
        "interpolation": 2
        } ] },
    "scale_y": {
      "Points": [ 
        { "co": { 
           "X": 1.0, 
           "Y": 1.0 },
        "interpolation": 2
        } ] }
} ] }

And this:

{"clips": [
  { "id": "ABCDE12345",
    "defaults": {
      "scale_x": 1.0,
      "scale_y": 1.0
    } } 
] }

... Multiplied by all of the properties we define. Most of which are hardly ever changed, if you consider all of the users, all of their projects, all of the clips in those projects, and all of the properties on each clip. Viewed in those terms, every user-defined keyframe point is really a very rare thing.

Oh, but as for what it uses as the curve type coming out of the first keyframe, when it's set to Linear -- well, that's one of those things... It won't be Linear, because OpenShot would adjust the first keyframe to Bezier as well. But what would happen if it didn't? Not sure.

I suspect it would be fine, and the first point would just act like a Bezier point with no right handle adjustment. (So, coming out of that point the value would have the initial value, and then the end keyframe would immediately start to influence it based on its own left handle adjustment.)

@ferdnyc
Copy link
Contributor

ferdnyc commented Nov 10, 2020

I just tested by "forcing" the issue.

  1. Created a project with one clip
  2. Set Location X to -0.46 about halfway through, using a Bezier keyframe with "Ease In/Out (Back)" curves
  3. Went back to the first frame, set the interpolation to Linear

Of course, at that point nothing changed, because the first keyframe still had its handle_right value with the ease-out adjustment applied. So, the value still went positive slightly for the first few frames, before swinging back to negative and decreasing to -0.46. (Actually, before going past it to -0.57, then returning to -0.46 at the second keyframe point.)

So then I saved the project, quit OpenShot, and edited the project file to remove the handle_right data entirely from the first keyframe point on location_x.

Loaded OpenShot back up with the modified project file, and the only difference was that the ease-out adjustment was gone, so now after the first frame the Location X value begins very slowly decreasing from 0.0.

@ferdnyc
Copy link
Contributor

ferdnyc commented Nov 10, 2020

Of course, at that point nothing changed, because the first keyframe still had its handle_right value with the ease-out adjustment applied.

Make that, the ease-in adjustment! #TIL that the "ease-in" curve shape refers to the handle_right of the previous keyframe point, while "ease-out" refers to the shape created by the handle_left value of the current keyframe point!

I guess the "in" and "out" in question mean easing the value into and out of the curve between the previous and current points. Not, as I'd again incorrectly assumed, referring to the shape of the curve on either side of where it crosses (or "enters" and "exits") the keyframe point.

@rexdk
Copy link
Author

rexdk commented Nov 10, 2020

Actually having the interpolation defined at the right hand end doesn't bother me - as long as it is defined and everyone knows. After all, the interp type refers to the curve, and there is always going to be one less curve than points (5 points, 4 curves), so one point left with an un-needed interp type. Having defined the interp type 'constant' I guess it needs to be defined at the end, otherwise it would need to be called 'step' - ie "step change at this point" vs "constant until this point."

I'd managed to find in KeyFrame.cpp that it's the right end that's used.

What still puzzles me is why we need to set the left point to bezier at all. Because if it is set to bezier then the interp to the left of that is implemented as bezier, which is not what the user wanted. I don't understand the python code well enough to see what is happening, but the results I found were very unexpected - not just the left point being set to bezier, but sometimes the point to the left of that as well, and I think in one case a point to the right was set to bezier.

Now, currently my OpenShot installation has problems, possibly because of multiple installs/uninstalls of daily builds, so I'd better sort that out next.

I do like your idea of having a 'default value'. This is edging towards having a "no-animation" option for properties - that would be so useful for the ordinary user. In fact most users. Most of the time users don't want animation, and all the tutorials have to cover the gory details of getting to the start of the clip, and how to recover if you set a keyframe you didn't want. We all do that, me frequently :).

@rexdk
Copy link
Author

rexdk commented Nov 10, 2020

I guess the "in" and "out" in question mean easing the value into and out of the curve between the previous and current points.

Yes, my reading of the bezier implementation and what I found from testing was that easing in delays/slows the start of the curve, and easing out slows the ending.

https://drking.org.uk/openshotguide/manual-animation/
is far from finished, but there are pretty pics at the bottom, generated with a js implementation of what OpenShot does. I hope.

@rexdk
Copy link
Author

rexdk commented Nov 10, 2020

OK I now have a properly working OpenShot. Probably my fault before.

So - re setting the previous point to bezier; I set a linear point at =61, then a bezier 'ease in/out back' point at x=91

Saved project .osp file:

"location_y": {
"Points": [
{
"co": {
"X": 1.0,
"Y": 0.0
},
"interpolation": 2
},
{
"co": {
"X": 61,
"Y": 0.7062937062937062
},
"interpolation": 0,
"handle_right": {
"Y": -0.55,
"X": 0.68
}
},
{
"co": {
"X": 91,
"Y": -0.13286713286713292
},
"interpolation": 0,
"handle_left": {
"Y": 1.55,
"X": 0.265
}
}
]
},

Everything looks like the code has done as expected - eg point x=61 has been set to bezier. But the problem is that the first curve is implemented as bezier,; there is no right point set at x=61 (and no left point at x=1) and the code presumably defaults to a type of bezier that the user didn't ask for). Just can't see a reason for it? Maybe I've missed something?

@rexdk
Copy link
Author

rexdk commented Nov 10, 2020

Yes, in Points.cpp handles are initialised - and presumably stay like that unless changed, so there is an unrequested ease in/out
Initialize_LeftHandle(0.5, 1.0);
Initialize_RightHandle(0.5, 0.0);

When I reported this issue I was getting a lot of strange results, but I think my installation was wonky. But the setting of the previous keyframe interpolation type is definitely there and applies to all interpolation types, not just bezier.

Would simply removing the line
point["interpolation"] = interpolation
for point["co"]["X"] == previous_point_x fix it?

@ferdnyc
Copy link
Contributor

ferdnyc commented Nov 11, 2020

Would simply removing the line
point["interpolation"] = interpolation
for point["co"]["X"] == previous_point_x fix it?

Reading that code over again now, I'm beginning to suspect the answer is yes. In fact, I'm pretty sure that line is only there due to a copy-paste mistake. Because, that entire block is identical to the code below it, except for the swap from handle_left to handle_right. And it shouldn't be identical.

There is at least one case, though, that has to be accounted for. And I'm not sure whether it's accounted for correctly in the current code, but I can tell it's at least trying to.

Say you have a bezier segment from X = 10 to X = 30. Now you create a keyframe at X = 20. What happens to that Bezier segment?

  1. The new keyframe will always be created as Linear. (I'm not sure that's right, even — meaning, I'm not sure it's the best approach, duplicating the existing interpolation on the segment being divided may be more natural. But for the moment, let's assume it's correct.)
  2. So, to preserve the "Bezier-ness" of the second half of the curve, quite possibly the handle_right parameter from the previous keyframe point (the X = 10 one) should be moved to become the new X = 20 point's "handle_right", since X = 10 is now going to be the start point of a Linear interpolation segment (at least temporarily), so it has no use for a "handle_right" anymore.

The whole "inserting points into the middle of existing curves" conundrum is the point where any sort of "interpolation applies to line segments" model really breaks down. Interpolation doesn't apply to individual line segments, it can't — it applies (separately) to each of a curve's endpoints, and the the only way to really implement Bezier curves is to have every node represent a Bezier spline point, complete with handles — not "some are Bezier curves, some are something else". Linear progressions can be implemented with Bezier curves anyway, it's just a special case of Bezier where the angle of attachment at each end is exactly equal to the slope of a straight line between the two values.

But, trying to change that now would affect existing projects, which we've already established we can't do, so really I'm just ranting and I don't know why I brought it up.

But the "incorrectly sets the interpolation mode of the preceding point" thing we can definitely change. We just have to work out how to handle bisected curves.

@rexdk
Copy link
Author

rexdk commented Nov 11, 2020

That's a very good point. Perhaps in an ideal world we'd ask the dude what left hand / right hand curves s/he wanted, every time he entered a point in the middle of a bezier. I don't think it's possible to guess his intentions:
new points
This is ease-in cubic.
For new point P, you could say perhaps he wanted a more vigorous ease in, and recalculate the handles. But you'd never be able to reproduce it, if he set another type and wanted to revert.
For new point Q, what does he want? Does he want a smooth curve, or is it the slow start, or fast finish that's important to him? There's no way to know. Same for new point R.
It might be possible to guess better if we considered the curves on either side. But - it's still a guess.

There is one case where we might take a sensible guess - where it's type 'constant' and the types on both sides are constant. Probably the dude wants 'constant' at the new point.

Thinking about how it could be documented, it might just be better to say a new middle point sets both curves to linear - in all cases. That would at least be clear.

If we try to be smart, what about a point after the start but before the first point. The interpolation type setting at the start is never used (and I've found is often wonky) - any decision is a pure guess. Same at the end, though you might be luckier here if all the previous curves have been ease in/out quart for example.

There is another drawback to being smart - which is that the bezier type is not stored at the point - merely the handles there and at the previous point. It would be possible but pretty messy to discover the bezier type (maths and search through the bezier handles table for the closest match I think).

So my current thinking is just make the two curves linear. But I'm not at all sure.... I could see me being annoyed when my previously working ease-in was destroyed. And what about a simple 'insert keyframe' - what does it do?

I guess the good news is that it doesn't work properly at the moment, so any improvement will be better.

I'll think some more.

@rexdk
Copy link
Author

rexdk commented Nov 11, 2020

Another situation to consider: what should happen if a keyframe B between two keyframes A, C is deleted? What interp type should apply?
Currently it just deletes the point. So if C was bezier, A-C will be bezier, with C's righthandle and A's lefthandle - if that has ever been set. The lefthandle only gets set for on a bezier being added to the right, but that could have happened (several times). In essence the behaviour is unpredictable. Is that too terrible?

Trying to improve it would be messy, and probably not as useful as just telling the dude to look at his interp types if he deletes a point. And maybe the same if he adds a point.

===
Incidentally I suspect the default value here should not be the same as for the lefthandle

point["handle_right"] = point.get("handle_right") or {"Y": 0.0, "X": 0.0}

but should be
{"Y": 1.0, "X": 1.0}

Probably not a worry either way though

@ferdnyc
Copy link
Contributor

ferdnyc commented Nov 12, 2020

@rexdk

It might be possible to guess better if we considered the curves on either side. But - it's still a guess.

Well, it's only a guess if we try to guess the user's intentions. But as you say, we can't really do that, so we shouldn't try. By which I mean, we're better off dictating how things work based on a model that's workable and relatively expected — Principle of Least Astonishment — but we can still expect the user to meet us halfway.

Perhaps in an ideal world we'd ask the dude what left hand / right hand curves s/he wanted, every time he entered a point in the middle of a bezier.

In a sense, we are. Meaning, they adjust a property value to create a point, and then they have the ability to right-click on the curve icon and choose whatever interpolation they want, if they don't want the default linear. Interrupting the user with a prompt every time, though, would be an irritation for anyone who does want the default... and it's an unnecessary irritation, really, because once you've gotten familiar enough with the interface, you know that you need to set the interpolation for the new point if you want something different than the default.

There is one case where we might take a sensible guess - where it's type 'constant' and the types on both sides are constant. Probably the dude wants 'constant' at the new point.

Oh, I'm not really looking to try and make determinations beyond the current segment, because as you said trying to read too much into things is just guesswork, and often as not it'll be poor guesswork. With constant interpolation, specifically, by far the most common case for inserting a keyframe into the middle of a constant segment is that you want to make one side or the other non-constant, so I think the current default of inserting a linear keyframe is fine. (After all, linear degrades to constant if you set the same value for the keyframe points on both ends — which ultimately is just_another_ special case of Bezier. The whole point of Bezier splines is that any open, non-repeating curve can be expressed using them. That includes lines with perfectly linear slopes, even fully horizontal ones, which don't technically require the complexity of a Bezier curve but are fully compatible with them.)

Thinking about how it could be documented, it might just be better to say a new middle point sets both curves to linear - in all cases. That would at least be clear.

It would, but it would also be unnecessarily destructive, I think. IOW, if the user took the time to set Bezier interpolation on a point, I think it is safe to assume that they probably don't want that changed on them just because they later choose to add another control point — after all, isn't that how this whole thing started, just on the other end of the curve? I definitely want to try to preserve as much as possible from any previous explicit settings the user applied.

If we try to be smart, what about a point after the start but before the first point. The interpolation type setting at the start is never used (and I've found is often wonky) - any decision is a pure guess. Same at the end, though you might be luckier here if all the previous curves have been ease in/out quart for example.

There is another drawback to being smart - which is that the bezier type is not stored at the point - merely the handles there and at the previous point.

I think you're overthinking things, there — the Bezier type is the handles, that's literally how the different types are defined, as a pair of handle (X, Y) values, nothing more. We don't have to know or care what curve they chose by name, all we need to be concerned with (potentially) are handle values from the surrounding points.

It would be possible but pretty messy to discover the bezier type (maths and search through the bezier handles table for the closest match I think).

Buys us nothing, there's no additional information to be had beyond what we already have just from the handle values themselves.

So my current thinking is just make the two curves linear. But I'm not at all sure.... I could see me being annoyed when my previously working ease-in was destroyed.
Exactly, that's the thing I want to avoid.

And what about a simple 'insert keyframe' - what does it do?

Currently, inserts a linear keyframe point on the curve at that position (meaning, having the same value as was produced by the interpolation at that point in the curve. Which means, if the curve was a Bezier segment, then potentially it changes the shape quite a bit, potentially on both ends. (Though, how drastically it's changed will depend how far it is from either end, and what those ends look like. Bezier splines nominally tend towards linear near the middle of a segment, especially longer segments, but even still it's also affected by the eccentricity of the attachment points.)

I guess the good news is that it doesn't work properly at the moment, so any improvement will be better.

That's kind of it, yeah. We went through a similar thing with the timeline zoom, I guess over 2 years ago already — at the start it was just a continuous adjustment, you could set the scale anywhere from 1 second to I think 300 seconds, or any (and every) point in between. Which was super flexible, sure, but it also made the "Zoom in" and "Zoom out" buttons/shortcuts effectively useless, because each of the 300 steps had virtually no effect on the actual zoom level, so you'd have to repeat them like 15-20 times. Plenty of people had ideas, similar but all slightly different, for the "perfect" set of zoom levels. But the point wasn't to try and make zoom perfect for anyone, it was simply to make it usable for everyone.

Same basic goals apply here, I think. We want the interface to:

  1. Apply predictable defaults wherever possible
  2. Assist the user as much as reasonably possible
  3. Without making (bad) assumptions about what they want
  4. Make it as convenient as possible for them to override those defaults
  5. Respect their explicit inputs, first and foremost, when applying defaults/automatic adjustments

So, I'm least concerned about the defaults for the current keyframe point, when adding one. It's not an inconvenience to adjust things that only require a couple of mouse clicks, so that covers:

  1. The interpolation type of the previous segment up to that point (the one defined by the current keyframe point)
  2. The "handle_left" of that point
  3. The "handle_right" of the previous point.

Those are all easily adjusted after-the-fact.

Whereas, to set any of:

  1. The interpolation type on any other points in either direction
  2. Any handles in either direction other than the previous point's "handle_right"
  3. The current point's "handle_right"

The user has to move the playhead to make changes. That's much more inconvenient, and therefore IMHO it's a much higher priority that we try to get those right for the majority of cases.

@ferdnyc
Copy link
Contributor

ferdnyc commented Nov 12, 2020

@rexdk

Oh, I meant to respond to this, too:

Incidentally I suspect the default value here should not be the same as for the lefthandle

point["handle_right"] = point.get("handle_right") or {"Y": 0.0, "X": 0.0}

but should be
{"Y": 1.0, "X": 1.0}

Doesn't matter, those values are always going to be immediately overwritten with the applicable interpolation_details numbers on the next two lines. The only purpose of the {"X": 0.0, "Y": 0.0} default is to create a blank set of handle parameters into which the new values can be stored. (Which is needed if the existing handle values can't be retrieved, because there's no existing handle defined for that point.) But the initial values could be literally anything.

@rexdk
Copy link
Author

rexdk commented Nov 12, 2020

Great, thanks. I've thought about it overnight and came to the same conclusion as you.

I think you're overthinking things, there — the Bezier type is the handles, that's literally how the different types are defined

Yes, again you're right - I had thought that the handles were stored scaled, in which case there might be precision loss. But they're still 0-1 so can just be copied, and anyway any precision loss would have been unnoticed. I'd realised that overnight too.

So, the delete issue: with keyframes A, B C, on deleting B do we copy across B's right handle to A (if C is bezier)?

@ferdnyc
Copy link
Contributor

ferdnyc commented Nov 17, 2020

@rexdk

Reading over the value_updated() code end of last week, it suddenly struck me why that code was so overcomplicated and buggy. (Well — cart before the horse. It was buggy exactly because it was so overcomplicated and impossible to follow.) But it was so overcomplicated, I realized, because that one function was responsible for all of the following:

  • Inserting new keyframe points
  • Updating existing keyframe points
  • Updating NON-keyframe property values
  • Changing interpolation types for existing keyframes

So, I kind of... rewrote the whole thing.

I broke everything up into multiple separate functions with focused purpose, making it far easier to follow, and hopefully therefore much easier to maintain and modify as well. #3840 is the in-progress result of that effort, though it will still require a ton of testing before I'm confident all of the weird corner cases that will arise are covered in the same way the old code would have covered them. (Except the cases where the old code was wrong, and the new code will hopefully be able to fix those bugs.)

I also want to start incorporating things we've discussed here. So far I haven't addressed any of it, except that changing the interpolation type for point N no longer also changes point N-1. The new code at most sets only the handle_right of that point, which is as it should be.

The other proposals for how to better handle insertions, deletions, etc. are all still on the table, and my intention with the new code is that it would provide an opportunity to realistically address them. (There was no way I was going to even attempt to make the old code work the way we wanted it to, when I couldn't even follow it without a map and compass.)

I can post official builds made off that branch as needed, either right now in its current state (assuming the builds succeeded, I haven't checked) or once it's farther along and more ready for testing.

@ferdnyc
Copy link
Contributor

ferdnyc commented Nov 17, 2020

it was so overcomplicated, I realized, because that one function was responsible for all of the following:

  • Inserting new keyframe points
  • Updating existing keyframe points
  • Updating NON-keyframe property values
  • Changing interpolation types for existing keyframes

I left some things out, in fact! It was also responsible for:

  • Handling itemChange signals from the properties table (which are emitted when the user edits a property value by double-clicking and typing, as opposed to dragging the sliders or using the context menus)
  • Converting values produced by the properties model to the proper data type / format for insertion into the project data store.

@ferdnyc
Copy link
Contributor

ferdnyc commented Nov 17, 2020

So, the delete issue: with keyframes A, B C, on deleting B do we copy across B's right handle to A (if C is bezier)?

That's the million-dollar question, isn't it?

Well, actually, that can be an easy question depending on the interpolation type of segment A-B.

If B also has Bezier interpolation, then we should do nothing when deleting B other than delete B itself. Because:

  1. Point A already has a user-defined handle_right. (Defined when B was set to Bezier interpolation1)
  2. Point C already has a user-defined handle_left.
  3. So, although dropping B will perturb the A-C curve somewhat (inevitable when removing nodes from a Bezier curve, you're explicitly reducing the precision of the curve function), the curve exiting A and entering C will remain as close as possible to the user's previous settings if we simply don't touch any of the remaining handles at all.

Things become trickier if A-B isn't Bezier, though. Then, if you delete B, suddenly you're taking a point A which doesn't have a handle_right and was never set up to be part of a Bezier curve, and you're making it one of the endpoints of the newly-Bezier A-C, which is why we start talking about moving handles and such. But what if there's a different approach?

Backing up a bit, we already discussed the opposite problem that arises when inserting nodes into an existing Bezier segment. But that problem only arises because the inserted node is forced to convert the first half of the segment to Linear interpolation.

For any given point along a Bezier curve, the line passes through that point with a slope defined by its curve function, and the function can be solved for the slope at that point X. So, with the right math it's possible to insert a node into a Bezier curve at point X without disturbing the shape of the curve, simply by ensuring that the inserted point preserves the slope of the line and results in two curve functions that equal the original one when combined.

In Inkscape, you can insert a node along a Bezier curve by just selecting the "Edit paths by nodes" tool and double-clicking somewhere on the curve:
Screenshot from 2020-11-16 22-49-57
Inkscape not only inserts the node itself, but it automatically pulls out handles for it so that the shape of the curve is maintained even in the presence of the additional control point.

Long story short, theoretically if we were to do the same thing when inserting a keyframe point somewhere along an existing Bezier segment, then both sides could be preserved as Bezier segments, with all of their existing attributes preserved. Neither the handle_left of the first segment nor the handle_right of the second segment would match any of our Bezier presets, but that's fine.

The inserted slope/handle settings certainly may become undesirable the moment the user adjusts the value at the inserted point. (Which is one of the possible reasons for inserting it, the other being that they want to modify/delete some or all of the curve parameters on either side of that point.) But if they do that, then they just have to also adjust the Bezier properties to match — that's neither surprising nor inconvenient, and it's a manageable expectation on their part.

But until then, Least Astonishment is preserved, and the action of inserting a keyframe point is as minimally disruptive as possible, particularly to their previous work.

Swinging back around to deletion, now, the problem becomes opposite when deleting a node that lies between two segments that are not both Bezier, because there we're connecting together two dissimilar and fundamentally incompatible types of curve. A linear first-segment has no handle_right to contribute, in its new role as the starting endpoint of a Bezier curve segment.

It's tempting to solve this by moving the handle_right of the deleted point (the starting point of the second segment) down to its new starting point, creating a longer curve with a shape that's potentially similar to the shape of the previous second segment, though not necessarily that similar. (Because point A may have a very different value from point B, the new A-C curve may not have a shape that's anything close to B-C, even if A is given B's former handle_right.) Even beyond that caveat, sometimes this will be what the user wants, but at least as often it probably won't be anything like what they wanted.

However, just like we can fit an inserted Bezier node into an existing curve while still preserving the curve shape by matching the slope of the line at that point, we can make a point into a Bezier endpoint while still preserving the shape of its original Linear segment attachment. We simply need to give it a handle_right that results in a Bezier endpoint that has the same slope as the previous linear segment. And that slope is child's play to compute, especially positioned right at what was formerly the other end of that line segment. It's simply rise over run, or B.Y - A.Y / B.X - A.X.

So, instead of moving any handles, we should be able to create a new handle_right that turns the former Linear segment into the first half of a Bezier segment with minimal disruption to the shape of the resulting curve. (Especially compared to arbitrarily repositioning handles on the X axis.)

It may still not be what the user wants, but if not then they can easily override it after the fact by applying different parameters. They wouldn't even have to move the playhead, since you can apply new interpolation parameters for the current curve segment from anywhere along that segment, and immediately after deleting point B the playhead would be positioned somewhere along the new A-C. Plus, given that this approach would be minimally disruptive, it's arguably what the majority of users should reasonably expect. It also seems to me like it's by far the easiest/strongest case to make, should anyone need further convincing.

Notes

  1. Ignoring, for the moment, the originating bug. As long as it's a factor, the possibility exists that Bezier interpolation may have been accidentally spread to A-B merely as a consequence of applying it to B-C. But coding/designing around that buggy behavior is the wrong approach. Just plain fixing it is the right one, and then it's no longer an issue.

@ferdnyc
Copy link
Contributor

ferdnyc commented Nov 17, 2020

Long story short,

"Too late!"

@rexdk
Copy link
Author

rexdk commented Nov 17, 2020

Excellent :) - it may that I'll actually be able to understand the code properly now. I've found the existing code pretty opaque, Fun ahead.

One thing to watch out for in inserting/deleting is generating curves that the user cannot himself set, So he deletes a keyframe B and gets a bezier curve that starts in one way (ease-in (circ)) and ends in another (ease-in/out (back)). If he then adjusts either keyframe, he can never get back to what he had (except by undo, and of course repeating the original insert / delete steps.) How much of a worry is that?

One practical area where the user will often want to delete/insert keyframes is simply to move a keyframe ("this sliding-fade-in is happening too quickly - how do I move the keyframes back"). I wonder if it is possible to consider that now, in your new code - ie a new feature to move keyframes? That obviously impinges in other areas - so I'm just suggesting to give it consideration now, so that a future separate enhncement becomes easier.

The idea of inserting a keyframe so the curve remains smooth is I have to say rather appealing/eleagant (except for my point above that it cannot be specifically reset after). The maths is easy enough it seems - De Casteljau will give the handles.

@rexdk
Copy link
Author

rexdk commented Nov 17, 2020

Inserting keyframes on the curve: if we insert by 'insert keyframe', we're exactly on the curve - but what should happen if the user manually sets a keyframe slightly off the curve? We could still make the curve smooth. But even further off the curve? and then nowhere near the curve? I see a minefield.....

I note we're getting nearer to your view about being able to set both handles at each keyframe...

@ferdnyc
Copy link
Contributor

ferdnyc commented Nov 17, 2020

@rexdk

One thing to watch out for in inserting/deleting is generating curves that the user cannot himself set, So he deletes a keyframe B and gets a bezier curve that starts in one way (ease-in (circ)) and ends in another (ease-in/out (back)). If he then adjusts either keyframe, he can never get back to what he had (except by undo, and of course repeating the original insert / delete steps.) How much of a worry is that?

I'd be interested to hear takes on that from @jonoomph and others, but personally I don't think it's a problem. They can still change those values if they're not what they want by applying any of the presets that have always been available, and will still be available. So really, all we're doing is giving them access to additional possibilities that previously weren't accessible through any means.

There are already areas where modifications can't be completely removed, such as deleting the last keyframe point for a property. Unless that bug's been fixed, doing that breaks the property completely, because the initial point is where the property's initial/default value is stored.

(If that hasn't been fixed, then it's something I want to address with these changes, most likely by simply refusing to delete the last point in a list. Keyframe properties have to have at least one point, always. The user still won't be able to reset the property back to its default value except by manually re-applying it, though. The initial value is lost when the point is edited.)

Down the road, if people express a need, I could see us possibly adding more context menu options to apply computed handles, instead of preset, and I'm sure eventually users will have the ability to configure handles however they want manually. It's a feature that a small number of power users desire, and as long as it can be added without forcing complexity on everyone there's no good reason NOT to support it.

But we've gotten this far with only the presets, and as long as users are still ultimately in control of everything -- which they are -- I think enhancements are enhancements. It seems workable to me if the list of things users have access to apply directly is allowed to evolve based on need/interest, rather than burying them under options right from the jump. (Heck, I think he existing Bezier preset list is excessive to the point of being overwhelming.)

But I'm more than willing to be talked out of my position on "autogenerated-only" handle values, if people really see it as a problem.

@ferdnyc
Copy link
Contributor

ferdnyc commented Nov 17, 2020

@rexdk

One practical area where the user will often want to delete/insert keyframes is simply to move a keyframe ("this sliding-fade-in is happening too quickly - how do I move the keyframes back"). I wonder if it is possible to consider that now, in your new code - ie a new feature to move keyframes? That obviously impinges in other areas - so I'm just suggesting to give it consideration now, so that a future separate enhancement becomes easier.

The new code isn't likely to inherently make features like that easier to support, beyond the fact that it should be more modifiable in general simply by virtue of being less complex/convoluted. But it certainly doesn't add any new barriers, either.

Feature-wise it would be great to have, "in some fashion" -- and that's the hurdle, really: coming up with a clean UI. Data-wise, moving a keyframe point is a simple matter of modifying its co.X value. But as is so often the case, the devil is in the details.

  • How is that exposed to the user in the interface?
  • How do we handle moves that cross other points for the same property (change the order of points)?
  • etc, etc.

Practically, I can think of a few ways we could expose UI to better meet the user's needs there. (Their REAL need: Not to move points, but to customize the time range over which a property modification is applied -- moving points is one means of accomplishing that, but users don't necessarily have to be exposed to that level of detail.) So, to give them the ability to adjust where and for how long property actions like "Fade out" are applied to timeline objects...

  1. We could allow drag-and-drop moving of the green keyframe marks on the Timeline.
    • Cons: Imprecise; fiddly; no easy way to handle multiple properties with keyframes at the same point
  2. We could allow copy-paste relocation of individual points, in a manner similar to the existing copy/paste functionality for entire property keyframe lists.
    • Cons: Even more fiddly; AND tedious; could seriously explode the already-cluttered context menus
  3. We could allow the length of preset operations like Fade, Freeze, etc. to be specified on creation, say by enhancing the Timeline's playhead navigation with range selection. (I'll elaborate on this one shortly.)
    • Cons: Pretty minor, from an enduser perspective. Only notable concern is that it's not nearly as flexible as after-the-fact adjustment -- for better and worse. (It doesn't add the same complexity as adjustment, by the same token.) The primary barrier is the implementation complexity. This would be extremely involved, especially compared to the other options. (IOW, basically the only thing lacking is someone to write the hundreds/thousands of lines of both Python and JavaScript code. You know, that little detail. If not for the weeks and weeks of development time this would entail, we could have it tomorrow!)

Here's how that last idea would work. Imagine if, instead of timeline interactions being limited to selection of entire clips/transitions and positioning of the playhead by clicking or dragging, you could also select a time range on the timeline ruler. Picture it in a manner very much analogous to text selection: you drag a highlight from one point on the ruler to another, or shift-click somewhere adjacent to your cursor (the playhead), and the range of time between those two points becomes the “selected time[span]”. (The Playhead would probably always define one end of that range; we wouldn't want selection independent of positioning, I don’t think. But as with (some) text cursor implementations, the playhead could potentially be moved elsewhere after the selection is made, without affecting what’s currently selected.)

When a time selection is active, any clip property presets applied via the context menu would be applied over the selected time range, instead of using the current hardcoded values. So, if you want a 4-second fade out, you just select four seconds of the clip's runtime before applying Fade Out. No need to move points at all, because they're placed exactly where you say.

Moving points is far, FAR easier to implement, but it's much more complex for the user — not to mention, completely inaccessible to any users who haven’t yet climbed the fairly steep learning curve to fully understand keyframing in general. As soon as you start to really consider the UI in concrete terms, it gets surprisingly hard to design (or even envision) something that’s both a good fit for our existing interface, and exposes the functionality in a clear and usable fashion.

Performing an action over part of a dataset (as constrained by the currently selected range/area/region), OTOH, is a readily-understood, familiar UI convention that’s observed in everything from text editors to web browsers to GIMP to Inkscape to Audacity. Bringing that same concept to OpenShot would be a near-freebie in terms of user expectations and learning curve, though unfortunately the same is not even remotely true when it comes to implementation burden.

@ferdnyc
Copy link
Contributor

ferdnyc commented Nov 18, 2020

We could allow drag-and-drop moving of the green keyframe marks on the Timeline.

  • Cons: Imprecise; fiddly; no easy way to handle multiple properties with keyframes at the same point

There is one potential solution to that, and it might help with some other annoyances as well: Active-property selection.

In the same way that the green keyframe marks are only visible for the currently-selected clip/transition (the one for which property values are being displayed in the table), it would potentially be useful to have a notion of the "selected property": the one row out of all of them in the Properties Table that's currently visible / interactive on the Timeline.

Selecting a property might display only that property's keyframe positions (hiding all of the keyframe marks representing points for a different property), or its keyframe marks could be styled differently so they stand out from the others. They might be drawn slightly larger, or with less transparency, or in a different color — or in color at all (as opposed to any others which could be grayed out). Near-limitless possibilities.

Further extending that notion from display (output) to interaction (input), only the markings for the selected property would respond to mouse interactions. That would solve the problem of differentiating multiple properties that have keyframes in the same location, since dragging a tickmark would always imply dragging the selected property's keyframe point.

The tradeoff to that kind of feature is that it would increase the complexity of the Properties Table — both visually, and in terms of program logic. I don't think we'd be able to use the built-in table cell selection for this. (Selection among the table cells has already proved far too finicky and unreliable. The built-in Qt table selection is much too reactive to things like keyboard navigation (arrow keys, Tab) and input focus, even when we'd prefer it not be.)

We'd probably need to add a new column of switches or radio buttons to the properties table, for use in setting or changing the current property selection. Something like Audacity's per-track Solo / Mute switches, perhaps.

@ferdnyc
Copy link
Contributor

ferdnyc commented Nov 18, 2020

There are already areas where modifications can't be completely removed, such as deleting the last keyframe point for a property. Unless that bug's been fixed, doing that breaks the property completely, because the initial point is where the property's initial/default value is stored.

(If that hasn't been fixed,

It hadn't.

then it's something I want to address with these changes,

I now have, in #3840.

most likely by simply refusing to delete the last point in a list.

That's exactly how.

@rexdk
Copy link
Author

rexdk commented Nov 19, 2020

I now have, in #3840.
most likely by simply refusing to delete the last point in a list.
That's exactly how.

Good fix - makes sense

@stale
Copy link

stale bot commented May 21, 2021

Thank you so much for submitting an issue to help improve OpenShot Video Editor. We are sorry about this, but this particular issue has gone unnoticed for quite some time. To help keep the OpenShot GitHub Issue Tracker organized and focused, we must ensure that every issue is correctly labelled and triaged, to get the proper attention.

This issue will be closed, as it meets the following criteria:

  • No activity in the past 180 days
  • No one is assigned to this issue

We'd like to ask you to help us out and determine whether this issue should be reopened.

  • If this issue is reporting a bug, please can you attempt to reproduce on the latest daily build to help us to understand whether the bug still needs our attention.
  • If this issue is proposing a new feature, please can you verify whether the feature proposal is still relevant.

Thanks again for your help!

@stale stale bot added the stale This issue has not had any activity in 60 days :( label May 21, 2021
@stale stale bot closed this as completed Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug A bug, error, or breakage of any kind stale This issue has not had any activity in 60 days :(
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants