Page MenuHome

Sequencer: Add operators to add and remove fades
AcceptedPublic

Authored by Nathan Lovato (gdquest) on Jul 2 2019, 2:14 PM.

Details

Reviewers
Richard Antalik (ISS)
Group Reviewers
Video Sequencer
Summary

Here are two operators for the VSE that add, update fades, or clear fade animation on all selected sequences. This is a replacement for https://developer.blender.org/D5152, which is based on an old VSE add-on.

This fades add operator does a little more than the other diff: it tries to preserve the user's blend_alpha or volume animation, only clearing existing keyframes that are in the way of the fade. Right now, it also uses the existing animation's maximum value or, if there's no existing animation, the current volume or blend_alpha value as the max_value to fade to.

I've tried to write clean code. There are no guidelines for Python except for following PEP8 formatting so please tell me if I should change anything. For instance, is it okay to use methods like I do in the operator? I use this to make the execute method self-documenting.

Operators demo:

Fades add:

Adds or updates a fade animation for either visual or audio strips.
    Fade options:
    - In, Out, In and Out create a fade animation of the given duration from
    the start of the sequence, to the end of the sequence, or on boths sides
    - From playhead: the fade animation goes from the start of sequences under the playhead to the playhead
    - To playhead: the fade animation goes from the playhead to the end of sequences under the playhead
    By default, the duration of the fade is 1 second.

Fades clear:

Removes fade animation from selected sequences.
Removes opacity or volume animation on selected sequences and resets the
property to a value of 1.0. Works on all types of sequences.

It's my first contribution to Blender, please tell me if I messed up with something or if I can improve anything in general.

Diff Detail

Repository
rB Blender
Branch
fade-operators (branched from master)
Build Status
Buildable 4261
Build 4261: arc lint + arc unit

Event Timeline

Improvement suggested by @Peter Fog (tintwotin):

  • Using the length of the sequences as a maximum length for the fade, so that the operator can still insert a valid fade even if the sequence is too short. This would be consistent with how fade handles work in most video editing programs and daws I've worked with.
Nathan Lovato (gdquest) edited the summary of this revision. (Show Details)Jul 2 2019, 2:59 PM
Peter Fog (tintwotin) added a comment.EditedJul 2 2019, 3:50 PM

It gave me errors, but worked when I changed the names of the registered classes to the 2.80 class naming convention.
I uploaded a diff here: D5167 to fix that.
I also added the operator to the to "add" menu and "context" menu.
I changed the descriptions visible in the menu to capitalized, so they would be in consistency with all other menus.

Suggestions:

  • Fade In/Out to/from the current level(at the next/previous frame of graph) and not the max level of the strip.
  • Avoid operations outside the length of the strip and shorten duration if it is outside of the strip.
  • @William Reynish (billreynish) Enable using 'layout.prop(view, "show_region_hud")' to the Sequencer so it is visible to the users how values like duration and fade type can be changed via the 'Adjust previous operation' panel.
  • Let context.space_data.show_seconds determine if the duration should be shown in frames or sec.
  • Fade In/Out to/from the current level(at the next/previous frame of graph) and not the max level of the strip.: should the fade use the first keyframe on either side of the curve, or use the value right after the fade?

  • Avoid operations outside the length of the strip and shorten duration if it is outside of the strip. -> Makes sense. Should I use the length of each selected strip as individual maximums, or use the shortest sequence's duration? If I use an interactive tool in another video editing program or a daw, it will typically use the shortest strip's duration.
  • Let context.space_data.show_seconds determine if the duration should be shown in frames or sec.: I don't think it's possible. When defining properties, you don't have access to the context or that value. I don't know if it's even okay to have conditional statements when defining properties.

Calculate a maximum fade duration, based on the shortest sequence
Rename bpy.types.Operator -> Operator
Fade to the next keyframe in the curve in the fade direction
Use a try/except block to catch ReferenceErrors

I added the improvements I could based on @Peter Fog (tintwotin)'s feedback. It's ready for review now. I'd just need to know if the operator should work in frames instead of seconds?

Nathan Lovato (gdquest) updated this revision to Diff 16249.

Register the operators in the Add menu

There's code from @Peter Fog (tintwotin) to add the op to the menus. Is there a way to add him as a co-author of the diff or commits?

You simply mention other contributors in the commit itself when it comes to that.

As for the patch, it should use the Adjust Last Operation system, which currently isn't supported in the Sequencer. This should be added first.

Thanks William, I'll update the diff when adjust last op's been added πŸ™‚

Possible improvements:

  1. @Peter Fog (tintwotin) brought up an issue with how the fade works currently. If the user has an existing animation curve with few keyframes, the fades will go directly to the value of the next keyframe on the curve, in the fade direction. As changing the behavior of the fades will produce new edge cases, I would like people to test the tool to see if this is an issue when you're working on real video projects.
  2. @snu told me that he tries to detect existing fades if there is already an animation curve on the strips in his add-on, and if so he only moves the keyframes instead of adding new ones to avoid edge cases linked to adding and removing keyframes from the Fcurve. E.g. having residual keyframes if you create a fade, and later you create a shorter to fade on the same strip. This would also allow the clear fades operator to only remove detected fades.

We'd need some way to draw the fades reliably so users know which strips have fades in, out, or in and out. That'd be really useful. If someone can point me in the right direction, I could give a hand there. Although this feature and the implementation need review first.

I think there will always be *some edge cases* with this approach. As unlike in other audio and video editing programs, there is currently no separation between the concept of fades and the opacity or volume animation. Although fades are conceptually different: they're usually an automated curve that multiplies the user's automation/animation. With visual strips the lack of separation isn't a big issue in my experience, but for audio it can be inconvenient even with simple projects.

The biggest issue right now are, to me:

  • If you extend a strip, the fade animation won't update.
  • There is no drawing to represent fades in the VSE, making it hard to keep track of strips with fade animation.
  • Sequencer: add SequencerFadesClear and SequencerFadeAdd operators
  • Add FadesAdd operator to the Add menu
  • Capitalize names for menu entries
  • Fix edge case when clearing keyframes
  • Ensure the fade is at least 1 frame long

Don't really like the complexity of this OP, but that isn't any dealbreaker, as this is covering functionality, that should be native, thus temporary.

Also I am not used to work with animation API/OPs so I assume this is correct and efficient.
If I will have spare time I could analyze this in detail, but now I will mainly test functionality.

release/scripts/startup/bl_operators/sequencer.py
153

I know that this is logical thing to do and unlikely to impact performance, but it can.

I personally support this approach, but it is considered to be unconventional in blender.
What you should do is check only active strip (even if it can be not a part of selection)

I don't really want you to change this, but other reviewers may insist.
To avoid unnecessary discussions you should change this to active strip.

201

same ^^

305

I am thinking, if these 3 functions wouldn't be nicer as a method of Fade
But I am not sure really.

369

This shouldn't really be here.

Should be called by what it does (seems to calculate number_of_frames_from_seconds)
and be part of some utility lib like release/scripts/modules/bpy/utils/__init__.py if that utility already doesn't have such function.

release/scripts/startup/bl_ui/space_sequencer.py
484

I guess this made it here by accident.

Peter Fog (tintwotin) added a comment.EditedJul 20 2019, 10:29 AM

@Richard Antalik (ISS) Do you have an opinion about preserving the curve between the inserted fades?

And using s.ms instead of frames as value style?

release/scripts/startup/bl_operators/sequencer.py
153

For consistency + Alt could use the operator on the full selection. So in the operator, an enum could determine what selection to work on.

release/scripts/startup/bl_ui/space_sequencer.py
484

Menu entries are not plural. This thing is also fixed here: D5287

The lacking consistency becomes visible when adding "Fade" next to "Transitions".

Nathan Lovato (gdquest) added a comment.EditedSun, Jul 21, 7:38 AM

Regarding complexity: that's what it takes to provide the functionality. There are parts of the API that are a bit inconvenient, e.g. sequences don't have animation data attached to them, so you need code to find and create anim data on the scene's anim action, etc. Same, there's no notion of fades in the VSE's back-end, so you have to code everything. 200 isn't a lot if you keep this in mind + the op's features + the fact that it already accounts for some corner cases.

The main loop is hopefully pretty readable as well, it should be easy to maintain:

for sequence in sequences:
    duration = self.calculate_fade_duration(context, sequence)
    duration = min(duration, max_duration)
    if not self.is_long_enough(sequence, duration):
        continue

    animated_property = 'volume' if hasattr(sequence, 'volume') else 'blend_alpha'
    fade_fcurve = self.fade_find_or_create_fcurve(context, sequence, animated_property)
    fades = self.calculate_fades(sequence, fade_fcurve, animated_property, duration)
    self.fade_animation_clear(context, fade_fcurve, fades)
    self.fade_animation_create(fade_fcurve, fades)

The Fade class adds some locs but it also helps to decouple the op. Before I added it, the changes requested in the vse chat were a pain to code.

release/scripts/startup/bl_operators/sequencer.py
153

I'll do this for now, although it'll cause UX issues if we add UI for the operator

305

They wouldn't work as methods for Fades, as they take 1 or 2 fade objects and operate on a sequence. Turning them into methods of the operator.

369

Actually, I was asking if it's okay if the duration the user selects is in seconds? IIRC @Peter Fog (tintwotin) mentioned that in the rest of Blender durations are in frames. That's why I haven't cleaned this up. I was thinking it would have to be removed?

Regarding the name. It calculates a duration in seconds from a duration in frames. How about get_duration_seconds(duration_frames)? You don't use the arguments to shorten function names in Blender's codebase? I think it's common in Python but maybe not in C.

release/scripts/startup/bl_ui/space_sequencer.py
484

Ah yeah, this should be fixed in the operator!

  • Sequencer: add SequencerFadesClear and SequencerFadeAdd operators
  • Add FadesAdd operator to the Add menu
  • Capitalize names for menu entries
  • Fix edge case when clearing keyframes
  • Ensure the fade is at least 1 frame long
  • Fix poll methods, turn fade functions into methods
Nathan Lovato (gdquest) marked 6 inline comments as done.Sun, Jul 21, 7:42 AM

Done with the changes. Only one left: the duration_seconds thing. Should I remove it and have the operator work in frames instead, to be consistent with the rest of Blender?

Ok so I tested this, seems good, but:

  • Clear fades operator isn't available from UI, unless it is searched. It really should be.
  • How can I choose duration as shown in demo? I mean functionality is provided by from/to playhead, but just asking if this was dropped or something.
release/scripts/startup/bl_operators/sequencer.py
369

As for seconds vs frames:
I think in future duration will have to be all in seconds to support mixed framerates.
I don't have strong opinion here. Frames would be cleaner, but seconds are nicer to use.

Function name:
Sorry, I think it's not bad actually.
And in the end I would maybe keep it here...
Can be rafactored later if needed.

Ok so I tested this, seems good, but:

  • Clear fades operator isn't available from UI, unless it is searched. It really should be.

@Peter Fog (tintwotin) where would you put the Fade Clear operator? How about the menu Strip -> Fade Clear under Hold Cut?

  • How can I choose duration as shown in demo? I mean functionality is provided by from/to playhead, but just asking if this was dropped or something.

After calling the operator, F9, or Edit -> Adjust Last Operation. That's how operators work in blender. It'll be more accessible once a floating window like the one in the 3d view gets added. Also, I'd like to turn this into a tool once we have a VSE toolbar.

Nathan Lovato (gdquest) marked an inline comment as done.Fri, Aug 2, 10:19 AM
Nathan Lovato (gdquest) updated this revision to Diff 16793.
  • Sequencer: add SequencerFadesClear and SequencerFadeAdd operators
  • Add FadesAdd operator to the Add menu
  • Capitalize names for menu entries
  • Fix edge case when clearing keyframes
  • Ensure the fade is at least 1 frame long
  • Fix poll methods, turn fade functions into methods
  • Add Fade and Clear Fade to contextual menu in the VSE

Just added the last bit: now Fade and Clear Fade show in the VSE context menu, but only if you have at least 1 sequence selected. Before, Fade would show even without a selection, but would be greyed out.

snu removed a subscriber: snu.Sun, Aug 4, 2:47 AM

Sorry forgot to send this comment

I will accept this, but have to ask if additional review is needed before comitting.

release/scripts/startup/bl_ui/space_sequencer.py
485

This is still problematic because python object has to be created which is source of potential speed issues. Previous use must have slipped through review.

For now let this be like it is, I will consult API function bypassing python object, so it may be acceptable

This revision is now accepted and ready to land.Tue, Aug 6, 2:20 PM

Thanks for the review. I'll be around to maintain my code, ping me anytime if you need me. πŸ™‚

release/scripts/startup/bl_ui/space_sequencer.py
485

There was another check for len(context.selected_sequences) > ... in this code block, I just stored the number of sequences in a variable and reused it. No perf issues here, and it's only in the right click context menu.

Also, the corresponding menu entries should only display if there are at least two selected strips, so we need to be able to check for the number of strips in the sequencer. If this can cause perf issues, maybe we should have some new property or getter in the API to access the sequences count in the context. It's also an issue with the sequencer's poll operators, that let you call operators and leave buttons active when they should not be.