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
773

Will do.

821

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
492

Ok.

499

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.

631

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
660

Another issue, another task then :-)

821

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
631

OK then

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

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
612

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

821

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

pitivi/timeline/elements.py
385

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

447

Please add a one line pydoc to say what it is

492

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

500

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
612

Ok.

pitivi/timeline/elements.py
385

_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.

447

Ok.

500

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
385

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
385

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
591

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

660

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
559

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.

727

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
559

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

727

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
675

This should not be needed anymore.

700

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

727

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.

803

We should make sure that never happens.

pitivi/timeline/elements.py
468

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

632

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
468

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
468

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
803

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
803

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

303–304

indentation

313–314

indentation

449

newline above, for consistency

tests/test_timeline_elements.py
65

why not do this when we set the duration above?

182

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.

155

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.