Page MenuHome

Sequencer: Add operators to add and remove fades
Needs RevisionPublic

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

Details

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

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?

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
502

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
502

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.EditedJul 21 2019, 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
502

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.Jul 21 2019, 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.
  • 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.Aug 4 2019, 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
503

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.Aug 6 2019, 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
503

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.

This revision was automatically updated to reflect the committed changes.
This revision is now accepted and ready to land.Sep 14 2019, 11:02 PM
Campbell Barton (campbellbarton) requested changes to this revision.EditedSep 14 2019, 11:03 PM

Using eval(...) on user data (animation data paths) is a security risk.

It should be possible to avoid using eval in this case.

The issue is similar to this CVE https://www.cvedetails.com/cve/CVE-2005-3302

I've disabled the operators until this can be addressed.

This revision now requires changes to proceed.Sep 14 2019, 11:03 PM

@Campbell Barton (campbellbarton) Haven't noticed this during review.

Not sure what reopening this diff means - can I or author just make fix for this issue and commit separately?

This is just

@Campbell Barton (campbellbarton) Haven't noticed this during review.
Not sure what reopening this diff means - can I or author just make fix for this issue and commit separately?

Re-opened just to mark it as needing changes. Talked to @Nathan Lovato (gdquest) in chat, he's looking into an alternative to eval(..)

Another thing I haven't noticed is that there are some warnings caused by this patch:

WARN (rna.define): c:\blender-git\2\source\blender\makesrna\intern\rna_define.c:1239 RNA_def_struct_ui_text: 'Removes fade animation from selected sequences.' description from 'SEQUENCER_OT_fades_clear' '' ends with a '.' !
WARN (rna.define): c:\blender-git\2\source\blender\makesrna\intern\rna_define.c:1647 RNA_def_property_ui_text: 'Fade in, out, both in and out, to, or from the playhead. Default is both in and out.' description from 'type' '' ends with a '.' !
WARN (rna.define): c:\blender-git\2\source\blender\makesrna\intern\rna_define.c:1239 RNA_def_struct_ui_text: 'Adds or updates a fade animation for either visual or audio strips.' description from 'SEQUENCER_OT_fades_add' '' ends with a '.' !

Here is the patch to fix the issue: https://developer.blender.org/D5807

Another thing I haven't noticed is that there are some warnings caused by this patch:

I think Campbell already fixed them. I double-checked and descriptions don't end with '.' in the latest master.

Just a heads-up that this was already solved, the feature is in Blender 2.81, so you can close the diff. I can't see an option to mark it as done/closed myself.