Keyframes for transformation properties.
ClosedPublic

Authored by stefanzzz on Jul 4 2017, 4:02 PM.

Details

Summary

Added the possibility to add/remove keyframes on transformation
properties + visual keyframe curve.

When adding or moving a keyframe, we don't use the matplotlib position
anymore. Instead, we compute the position the same way we do for the seek
logic, to make sure the playhead seeks precisely on the added/moved keyframe.
This change led to some other changes in the unit test which tested the
keyframe curve. More precisely, we needed to compute the position in pixels
for the click events, as that is how the seek logic received it.

Diff Detail

Repository
rPTV Pitivi
There are a very large number of changes, so older changes are hidden. Show Older Changes
stefanzzz added inline comments.Jul 4 2017, 6:39 PM
pitivi/clipproperties.py
761

Will do.

809

No. They're not always equal. self.__keyframes_active is True if the source uses keyframes and self.__activate_keyframes_btn.props.active is True if the source uses keyframes and the keyframe curve is visible. Maybe I should change the naming of __activate_keyframes_btn. The workflow is like this:

  • Pressing the activate keyframes button will activate the keyframes and show the keyframe curve
  • Any further press of the activate keyframes button will toggle the visibility of the keyframe curve
  • Pressing reset to default button will deactivate the keyframe.
stefanzzz added inline comments.Jul 4 2017, 6:58 PM
pitivi/timeline/elements.py
491

Ok.

498

That's how it was done in the method I was overriding. But you're right. It's not needed in either of the classes.

613

For MultipleKeyframeCurve, the 2 edge keyframes are always already created before calling this function. So that condition would never be True.

thiblahute requested changes to this revision.Jul 4 2017, 7:01 PM
This revision now requires changes to proceed.Jul 4 2017, 7:01 PM
thiblahute added inline comments.Jul 5 2017, 6:47 PM
pitivi/clipproperties.py
653

Another issue, another task then :-)

809

Ah, this sounds weird to me, clicking on an a toggled activate keyframe button should deactivate keyframes I think, unexpanding the 'Clip Properties' expander should hide them, I think it would feel more natural to users (maybe I decided to do that back then, but then I do not agree with myself anymore xD)

pitivi/timeline/elements.py
613

OK then

stefanzzz added inline comments.Jul 6 2017, 7:26 AM
pitivi/clipproperties.py
809

Actually, in your branch, the activate keyframes button would activate/deactivate the keyframes. I decided to change the meaning of it because I wanted to be the same as for the effects. Now, it behaves exactly like the effects keyframes, so I guess the users should be used to that.

aleb added inline comments.Jul 7 2017, 5:32 AM
pitivi/clipproperties.py
605

If position is past the clip, use the end of the clip instead of the start?

809

It should work as much as possible like the buttons for effect properties, indeed.

pitivi/timeline/elements.py
384

It would be best if callback methods are not called directly by us. Why is this needed?

446

Please add a one line pydoc to say what it is

491

pass is how you have an empty method in Python. We have it in other places.

499

You can remove two parenthesis

tests/test_timeline_elements.py
65

Use Gst.SECOND maybe

stefanzzz added inline comments.Jul 7 2017, 8:45 AM
pitivi/clipproperties.py
605

Ok.

pitivi/timeline/elements.py
384

_mpl_motion_event_cb is responsible for moving a keyframe or keyframe line when dragging them. The thing is the final position of any of those objects should be the one we get from the BUTTON_RELEASE event (that's where the dragging basically stops), so that's why I call the motion callback from the button release callback. If I didn't do that, the playhead would not seek to the exact position of the keyframe when dragging it, for example.

446

Ok.

499

Ok.

tests/test_timeline_elements.py
65

Yep. That's better.

aleb added inline comments.Jul 7 2017, 9:42 AM
pitivi/timeline/elements.py
384

Can you make it such that we don't call directly a callback method?

stefanzzz added inline comments.Jul 7 2017, 9:46 AM
pitivi/timeline/elements.py
384

Ok, but it would lead to some duplicate code.

stefanzzz updated this revision to Diff 6028.Jul 11 2017, 2:41 PM
stefanzzz edited the summary of this revision. (Show Details)

Made all required changes except for undo/redo on set/remove control
bindings, which looks a bit trickier and should be handled in a different
commit, I think.

stefanzzz updated this revision to Diff 6031.EditedJul 11 2017, 2:46 PM

No changes here.

stefanzzz marked 32 inline comments as done.Jul 11 2017, 3:14 PM
stefanzzz added inline comments.
pitivi/clipproperties.py
584

I'll do this in another commit. It look like it requires some more changes to the undo system.

653

I'll do this in another commit. It look like it requires some more changes to the undo system.

stefanzzz updated this revision to Diff 6054.Jul 17 2017, 4:01 PM

Extended undo/redo system so it can handle control binding set/remove
operations.

stefanzzz marked 8 inline comments as done.Jul 17 2017, 4:05 PM
aleb added inline comments.Jul 17 2017, 5:09 PM
pitivi/clipproperties.py
552

prefer quotes for new strings
Why connect to 'move' instead of connecting in __set_source to the objects we want to monitor, and update the UI when those change? Seems more likely to break if we connect to the action log.

721

Why ignore it?

pitivi/undo/timeline.py
589–590

Doesn't make a difference, but it should be UndoableAction

617

UndoableAction

stefanzzz added inline comments.Jul 18 2017, 4:17 PM
pitivi/clipproperties.py
552

You're right. Monitoring the source properties makes more sense.

721

I'm using get_child_property if I get a pipeline error. I noticed that get_child_property doesn't always give the correct result (I think it's a bit delayed) when using control bindings, so that's why I prefer getting the value from the control binding. If that results in a pipeline error though, it seems better to have the result of get_child_property rather than nothing.

pitivi/undo/timeline.py
589–590

Right! :)

stefanzzz updated this revision to Diff 6063.Jul 18 2017, 5:01 PM

Made required changes.

stefanzzz marked 5 inline comments as done.Jul 18 2017, 5:32 PM
thiblahute requested changes to this revision.Jul 18 2017, 7:49 PM
thiblahute added inline comments.
pitivi/clipproperties.py
668

This should not be needed anymore.

693

undo/redo subsystem is responsible for setting the modification state.

721

Same idea as in a previous commit, we should have a way to tell the pipeline to give us a position, even if it is the last known position (and never raise an exception), the handle weirdness as out of bound values in the calling function.

791

We should make sure that never happens.

pitivi/timeline/elements.py
467

Well, that should never happens, we should make sure that the keyframes are added at the right time (ie. before their representation.).

614

contol_source?

This revision now requires changes to proceed.Jul 18 2017, 7:49 PM
stefanzzz added inline comments.Jul 19 2017, 12:43 PM
pitivi/timeline/elements.py
467

I used it so I won't get any errors when removing the keyframes at reset to default operation. Basically, for reset to default I first clear all the bindings and than update the keyframe curve from the MultipleKeyframeCurve to the alpha Keyframe Curve. So, there's a small period when there are less than 2 keyframes and the plot won't update.

thiblahute added inline comments.Jul 19 2017, 2:05 PM
pitivi/timeline/elements.py
467

OK then.

stefanzzz updated this revision to Diff 6069.Jul 19 2017, 2:19 PM
stefanzzz marked 8 inline comments as done.

Made required changes.

stefanzzz added inline comments.Jul 19 2017, 2:21 PM
pitivi/clipproperties.py
791

It only happens if get_source_property returns False (which shouldn't normally happen).

thiblahute accepted this revision.Jul 19 2017, 3:59 PM

Looks good to me.

pitivi/clipproperties.py
791

Hrmk then.

This revision is now accepted and ready to land.Jul 19 2017, 3:59 PM
aleb added inline comments.Jul 19 2017, 9:13 PM
pitivi/timeline/elements.py
232

indentation

304

indentation

314

indentation

448

newline above, for consistency

tests/test_timeline_elements.py
65

why not do this when we set the duration above?

184

If you mock it like this here, it will remain mocked for all the tests.
Please use a "with ..." construct whenever you need to mock library methods.

stefanzzz updated this revision to Diff 6077.Jul 20 2017, 12:07 PM
stefanzzz marked 9 inline comments as done.

Some coding style improvements + "with..." mock construct

stefanzzz added inline comments.Jul 20 2017, 12:10 PM
tests/test_timeline_elements.py
65

Because I didn't want to change the unit tests that we already had. It doesn't really matter, though.

aleb added inline comments.Jul 20 2017, 12:21 PM
tests/test_timeline_elements.py
65

We should not do extra unneeded operations (change clip length) - better to set it once at the beginning.

158

use with ...

stefanzzz updated this revision to Diff 6080.Jul 20 2017, 12:47 PM
stefanzzz marked 4 inline comments as done.

Made required changes.

stefanzzz updated this revision to Diff 6155.Jul 27 2017, 7:49 AM
stefanzzz edited the summary of this revision. (Show Details)

Commit the timeline at the end of the reset to default operation
(otherwise, the viewer doesn't update).

This revision was automatically updated to reflect the committed changes.