WIP: Properties: Rewrite keyframe processing #3840
Conversation
| PropertiesModel.set_interpolation( | ||
| points, | ||
| prop_data.get("previous_point_x"), | ||
| prop_data.get("closest_point_x"), | ||
| interpolation, | ||
| curves, | ||
| ) for points in points_to_update]) |
There was a problem hiding this comment.
Among the untested and/or unproven assumptions in the new code that really need to be tested before we can even think about merging this: The assumption here is that the current keyframe curve segment's endpoints are always located at previous_point_x and closest_point_x.
- Is that always correct? Is it ever really correct, even?
- Is it (still?) correct when the playhead is positioned at a keyframe point?
- Is it (still?) correct when the playhead is positioned beyond the last defined keyframe point?
- Is it (still?) correct when the playhead is positioned beyond the end of the selected timeline item entirely?
- (What will
closest_point_xbe, in the previous three cases? The same asself.frame_number? Which for the last case, would be equal to the clip/transition's end frame.) - Are there ever cases where we can't rely on
closest_point_x, and have to look atself.frame_numberinstead, when setting interpolation parameters?
There was a problem hiding this comment.
- Does
set_interpolation()ever need to also insert a value atclosest_point_x? (Meaning, doesclosest_point_xever represent a position that doesn't already hold an existing keyframe point, but one that needs to be created? Perhaps when the playhead is positioned out beyond the farthest point in the existing list?)
There was a problem hiding this comment.
Hmm. Tangentially, I'm now wondering if I should re-rename curves (which I renamed from the supremely unwieldy interpolation_details), and instead call it handles?
There was a problem hiding this comment.
This appears to all be OK, except for a special case when the playhead is positioned at frame 1 of the clip. In those cases, closest_point_x == previous_point_x == 1, which confuses the set_interpolation() points-loop because looks for previous_point_x, finds it and sets handle_right, then does a continue and starts looking for closest_point_x — which it's already passed. So, I just have to special-case the two endpoints being the same.
(In which case, trying to create a curve at all is nonsensical, so I think set_interpolation can just bail immediately.)
There was a problem hiding this comment.
prev_x == closest_x fixed with warning log and early return.
And I did rename curves to handles, which is more descriptive.
There was a problem hiding this comment.
- (What will
closest_point_xbe, in the previous three cases? The same asself.frame_number? Which for the last case, would be equal to the clip/transition's end frame.)- Are there ever cases where we can't rely on
closest_point_x, and have to look atself.frame_numberinstead, when setting interpolation parameters?
To answer my own questions, when out past the last keyframe point, closest_point_x is always equal to the position of that point, and previous_point_x is always the previous point. Which is why you need at least 2 keyframe points before interpolation modes become visible/active in the UI.
The good
closest_point_x will never point to a non-keyframe point. No worries about that.
The ugly-ish
It's possible to choose an interpolation mode with the playhead positioned beyond the last keyframe, and that will (somewhat-surprisingly) change the mode of the previous segment, the one between previous_point_x and closest_point_x, even though the playhead is not positioned between those two points.
(In any other situation, setting interpolation mode changes the interpolation of the curve segment the playhead is currently positioned in — that is, the interpolation mode between previous_point_x < self.frame_number <= closest_point_x. But after the last keyframe, when self.frame_number becomes greater than closest_point_x, that value and the value of previous_point_x do not change, so any keyframe/interpolation operations will still affect the previous keyframe curve segment, not the unbounded one in which the playhead is positioned.)
There was a problem hiding this comment.
I wonder if we should disable/hide interpolation mode once
self.frame_number > ...let's call it last_point_x? (Which we can easily check because it's the only condition in which self.frame_number > closest_point_x.)
The user can still use Insert keyframe, or they can still adjust values to implicitly create a new keyframe, but they can't view or change the interpolation mode of the current curve segment because it doesn't have one.
|
It strikes me that @musteresel's input would be especially helpful here, since ultimately this code is deeply intertwined with the libopenshot keyframe implementation. |
ferdnyc
added
needs testing
|
As I've mentioned in the past, most recently in #3823 (comment), currently if "Remove keyframe" is used on a keyframe list which contains only one point, that point is removed and the property promptly breaks because it no longer has any value at all. (Initial values are set via a keyframe at Worse, once a keyframe list is broken (emptied) the interface can no longer set values for that property anymore, Which means the property is not just broken, it's unfixable (at least from within OpenShot itself). This PR's code now fixes that long-standing bug, by simply refusing to delete the last point in a keyframe list. Doesn't matter where it's located, doesn't matter what its value is, every list of points is prohibited from having a length < 1. This implies that it's still permissible to:
The property will now have the value That has the potential to be confusing, so it's probably a bad idea and best avoided unless there's a pretty good reason to do it. But there's really no reason to prohibit this sort of setup, since everything will still function just the way it's supposed to. |
|
My goodness, I'm struggling to keep up here... :) The case where the x=0 keyframe is deleted: just thinking about the prospect of having a no-animation setting (which we've touched on, and I reckon would help many users). I'd vaguely thought that with 'no-animation' the x=0 keyframe would store the value set. But it can already be deleted in the current build, so I guess nothing has changed in that respect. There is some arcane logic behind that x=0 keyframe, which is a mystery to me; sometimes it is visible to the user and sometimes not - but why? Possibly because there is no no-animation facility? There is another small issue that I've not brought up yet, but it might help to mention it while this is hot: the property label helpfully goes blue to tell the dude that interpolation is happening. But after the final keyframe, if no end keyframe is set, the property label is still blue - even though the value is static and not interpolating. This was more of an issue before we could navigate between keyframes, but it's still confusing and difficult to document. I guess it's part of the arcane logic, mysterious. It may well be better as a separate issue, but possibly helpful to mention now as the relevant code is presumably nearby. I'm finding it hard to get to grips with the code - I'm sure understanding will come slowly, but there are lots of hurdles - like why is there a closest_point_x - rather than actual_point_x ? Drilling down through the many layers is a slow process. Not asking for help - there are better uses for your time, just explaining why I'm not more incisive |
Well, there is no
That's primarily a rendering bug. At some point the keyframe marks slipped a bit left of where they should be. So right now if a clip is positioned at the first frame of the Timeline, the green tick for all of its
I added some followups to my review comment above which hopefully shed some light on that. But in short, So,
That's kind of a definitional / semantic question, I suppose — in OpenShot's current logic, that value is still interpolated, because it's computed based on keyframe points and will be non-static across the entire clip. A value that isn't changing, but previously did change, can still be interpolated — the same way, when you have a Constant segment between points A and B (where Effectively, you could say that for every property with a keyframe list, the value is interpolated for every frame If the value is always the same for the entire clip, the property isn't colored. If it changes at any point, it's colored green at keyframe points, blue otherwise — including the frames beyond the last keyframe point. |
|
P.S> " |
I wondered if there was anything else that would set the last point when the playhead is past it. I can't find it, but managed to generate a few more keyframes for different properties at the end. So now changing the interpolation type near the end affects a keyframe several back. That's not great, and I'm inclined to agree we should hide interpolation setting here as you say. There's still the situation where we adjust location-x interp type between 2 location-x keyframes say, but there are other location-y say keyframes in between. But that's logical enough I think. While on the topic, a keyframe at the very start shows an interpolation type, but that's not used of course. It can actually be set. Same thing for your case where a keyframe B is set in the middle somewhere, and the keyframe at the start is deleted. Should we |
I'll have to look into that, TBH I'm not sure if we can. It depends if that part of the code has access to both the playhead position and the X location of the last keyframe point for the property. If so, we can check for that condition no problem. But if it isn't easily detected, then we may have to live with how things are currently.
Well, the thing to keep in mind there, though it's admittedly a behind-the-scenes sort of detail, is that keyframes for any other property aren't really "in between", since each property's keyframes are kept on a separate list. In terms of how they're displayed on the Timeline, all of the lists are merged together, but each property only has access to its own keyframes. Any others, it doesn't even know are there. That's an area where the active-property stuff I mentioned in #3823 (comment) might help, since it would differentiate between keyframe points that belong to one property and any other points that belong to other properties. ...It might help, or OTOH maybe it would just make things even more confusing for the users, because they'd have to keep changing their property selection when viewing / seeking to / modifying keyframe points.
Mmm, in both of those cases, the interpolation type shouldn't show unless there are more points for that property. Like I said, we special-case for properties that only have one point, and already hide the interpolation menu choices and icon. That's true even if the single point is somewhere other than When there are two or more points, it's true that the first frame shows interpolation options, but as of the new code in this PR you won't be able to set the interpolation mode — if you try, you'll trigger the new check I added that will abort I suppose we could see about checking when the playhead is at the first frame, and special-case the view so it doesn't display the icon or the interpolation menu options. I'm just a little worried that might be confusing. Of course, this is only a problem because of the bugfix. In the old code, the interpolation mode at I kind of wish that's what it did show for the first frame, since the interpolation mode of the first curve is really the only one that matters from that point — it's how all of the values to the right of that frame will be computed, which is all you care about in terms of interpolation. The interpolation mode set or not set on the point |
Ah, but it's not exactly clear at the moment :) . I actually think it's OK if it doesn't show at x=1, but then I'm comfortable with having the curve set at the second keyframe. Another option would be to partially reverse what you've just been correcting, and copy the interp type only if previous_point is at x=1. Not sure I like it much, but you might :)
There's certainly scope for confusion - they might want to focus on both locaton x and location y for example if they're moving a clip, so they probably want "active properties" (plural). How on earth do you activate that without too many options? Well possibly by allowing them to tick mutliple properties I guess. But it's getting complex - and most users wil only have a minimum number of keyframes, if any. Anyone with a lot of keyframes is probably a power user, and can cope.
Arf - yes indeed. Thanks for all the explanations - very helpful |
|
Merge conflicts have been detected on this PR, please resolve. |
|
Merge conflicts have been detected on this PR, please resolve. |
|
Merge conflicts have been detected on this PR, please resolve. |
- PropertiesModel.value_updated() was an overloaded method that
performed all of the following functions:
* Inserting new keyframe points
* Updating existing keyframe points
* Updating NON-keyframe property values
* Changing interpolation types for existing keyframes
As a result, it was a confusing ramble of conditional logic.
- Yet, color properties (specifically, and only) were handled by a
separate (but largely cut-and-paste identical) color_updated()
method.
- Broke logic apart into separate, purpose-focused methods:
* update_value() to set/modify properties
* set_point() to create/modify single keyframe points
* update_interpolation() to apply keyframe interpolation types
* set_interpolation() to adjust interpolation values for single
keyframe segments
* item_changed(), the slot that processes the itemChanged signal
from the properties view
* interpret_as_type(), to convert model data into project data
- All of the above handles both color and non-color properties, of
all supported data types
- A rewritten remove_keyframe() supplements and complements the
new methods listed above
- Rename 'curves' list argument to 'handles' - Bail (early return) for special case where prev_x == closest_x
- Don't use mutable types (lists) as default arguments - Don't place keyword args before *args
|
Merge conflicts have been detected on this PR, please resolve. |
PropertiesModel.value_updated()was an overworked method that performed all of the following functions:itemChangesignals 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)As a result, it was a confusing ramble of conditional logic.
Yet, color properties (specifically, and only) were handled by a separate (but largely cut-and-paste identical)
color_updated()method.So, with this PR, I have:
value_updated()logic apart into separate, purpose-focused methods:update_value()to set/modify propertiesset_point(), a staticmethod that creates/modifies points in a single keyframe point list (called byupdate_value()one or multiple times to adjust individual property curves)update_interpolation()to apply keyframe interpolation parametersset_interpolation(), a staticmethod that adjusts interpolation values for a single keyframe point list (called byupdate_interpolation()one or multiple times to adjust individual property curves)item_changed(), the slot that processes the itemChanged signal from the properties view (by callingupdate_value()with the necessary parameters)interpret_as_type(), a staticmethod used to convert model data values into project data values before applying them to the project dict.TODOabout addingQFont/QFontInfosupport to the oldvalue_updated()method has been redirected tointerpret_as_type(), where that support would now be implemented... but has not yet been.)remove_keyframe()to supplement and complement the new methods listed aboveWIP because I have about 100 corner cases to test, to ensure that this is fully compatible with the old code (except where it fixes bugs in the old code). And also because there's a lot of ongoing conversation in #3823 that's worth incorporating into these changes. But I wanted to get it out there early.
Ultimately, hopefully: Fixes #3823