From 1fc1878cd6fc3c9a75049ea6caac59ffe930f831 Mon Sep 17 00:00:00 2001 From: John Kiril Swenson Date: Thu, 16 Jan 2025 01:40:36 +0100 Subject: [PATCH] VSE: Add effect/transition UI/code cleanup Part of the Jan 2025 Code Quality project described in #130975. This patch aims to improve user-facing messaging when adding effects or transitions, properly polling them out based on context, and avoiding unnecessary error messages when possible. - Rearrange "add Effect Strip" UI in order of required number of selected strips to create the effect strip, from 0 -> 1 -> 2 - Properly poll out these operators if not enough non-sound strips (i.e. any strips with video content) are selected. - Note that this does not require any extra iterations over the entire seqbase. - Gracefully avoid errors with trying to add effect/transition strips when sound strips are part of the selection: for example, when the user has selected connected strips. - In these cases, it is clear that the user wishes to operate on the strips with video content. - Refactor `seq_effect_find_selected` to fix bugs and account for all cases, removing TODOs in place. Rename it `seq_effect_get_new_inputs` to more accurately express its purpose. - Rename various `last_seq` to `active_strip` to adhere to new conventions laid out in #132736 - Update UI tooltips for effect, transition, and fades to make their use clearer. Pull Request: https://projects.blender.org/blender/blender/pulls/132672 --- scripts/startup/bl_ui/space_sequencer.py | 80 +++++----- .../editors/space_sequencer/sequencer_add.cc | 37 +++-- .../editors/space_sequencer/sequencer_edit.cc | 140 ++++++++---------- .../space_sequencer/sequencer_intern.hh | 12 +- 4 files changed, 133 insertions(+), 136 deletions(-) diff --git a/scripts/startup/bl_ui/space_sequencer.py b/scripts/startup/bl_ui/space_sequencer.py index afd63c804b9..138c4dc48d5 100644 --- a/scripts/startup/bl_ui/space_sequencer.py +++ b/scripts/startup/bl_ui/space_sequencer.py @@ -30,11 +30,15 @@ def _space_view_types(st): ) -def selected_sequences_len(context): - selected_sequences = getattr(context, "selected_sequences", None) - if selected_sequences is None: - return 0 - return len(selected_sequences) +def selected_strips_count(context): + selected_strips = getattr(context, "selected_sequences", None) + if selected_strips is None: + return 0, 0 + + total_count = len(selected_strips) + nonsound_count = sum(1 for strip in selected_strips if strip.type != 'SOUND') + + return total_count, nonsound_count def draw_color_balance(layout, color_balance): @@ -424,12 +428,13 @@ class SEQUENCER_MT_proxy(Menu): def draw(self, context): layout = self.layout - st = context.space_data + _, nonsound = selected_strips_count(context) + col = layout.column() col.operator("sequencer.enable_proxies", text="Setup") col.operator("sequencer.rebuild_proxy", text="Rebuild") - col.enabled = selected_sequences_len(context) >= 1 + col.enabled = nonsound >= 1 layout.prop(st, "proxy_render_size", text="") @@ -742,13 +747,16 @@ class SEQUENCER_MT_add(Menu): layout.operator_context = 'INVOKE_DEFAULT' layout.menu("SEQUENCER_MT_add_effect", icon='SHADERFX') + total, nonsound = selected_strips_count(context) + col = layout.column() col.menu("SEQUENCER_MT_add_transitions", icon='ARROW_LEFTRIGHT') - col.enabled = selected_sequences_len(context) >= 2 + # Enable for video transitions or sound crossfade. + col.enabled = nonsound == 2 or (nonsound == 0 and total == 2) col = layout.column() col.operator_menu_enum("sequencer.fades_add", "type", text="Fade", icon='IPO_EASE_IN_OUT') - col.enabled = selected_sequences_len(context) >= 1 + col.enabled = total >= 1 class SEQUENCER_MT_add_scene(Menu): @@ -792,22 +800,24 @@ class SEQUENCER_MT_add_transitions(Menu): bl_label = "Transition" def draw(self, context): + total, nonsound = selected_strips_count(context) layout = self.layout col = layout.column() - col.operator("sequencer.crossfade_sounds", text="Sound Crossfade") + col.enabled = (nonsound == 0 and total == 2) - col.separator() + layout.separator() + col = layout.column() col.operator("sequencer.effect_strip_add", text="Cross").type = 'CROSS' col.operator("sequencer.effect_strip_add", text="Gamma Cross").type = 'GAMMA_CROSS' col.separator() col.operator("sequencer.effect_strip_add", text="Wipe").type = 'WIPE' - col.enabled = selected_sequences_len(context) >= 2 + col.enabled = nonsound == 2 class SEQUENCER_MT_add_effect(Menu): @@ -817,6 +827,23 @@ class SEQUENCER_MT_add_effect(Menu): layout = self.layout layout.operator_context = 'INVOKE_REGION_WIN' + _, nonsound = selected_strips_count(context) + + layout.operator("sequencer.effect_strip_add", text="Multicam Selector").type = 'MULTICAM' + + layout.separator() + + col = layout.column() + col.operator("sequencer.effect_strip_add", text="Transform").type = 'TRANSFORM' + col.operator("sequencer.effect_strip_add", text="Speed Control").type = 'SPEED' + + col.separator() + + col.operator("sequencer.effect_strip_add", text="Glow").type = 'GLOW' + col.operator("sequencer.effect_strip_add", text="Gaussian Blur").type = 'GAUSSIAN_BLUR' + col.enabled = nonsound == 1 + + layout.separator() col = layout.column() col.operator( @@ -854,23 +881,7 @@ class SEQUENCER_MT_add_effect(Menu): text="Color Mix", text_ctxt=i18n_contexts.id_sequence, ).type = 'COLORMIX' - col.enabled = selected_sequences_len(context) >= 2 - - layout.separator() - - layout.operator("sequencer.effect_strip_add", text="Multicam Selector").type = 'MULTICAM' - - layout.separator() - - col = layout.column() - col.operator("sequencer.effect_strip_add", text="Transform").type = 'TRANSFORM' - col.operator("sequencer.effect_strip_add", text="Speed Control").type = 'SPEED' - - col.separator() - - col.operator("sequencer.effect_strip_add", text="Glow").type = 'GLOW' - col.operator("sequencer.effect_strip_add", text="Gaussian Blur").type = 'GAUSSIAN_BLUR' - col.enabled = selected_sequences_len(context) != 0 + col.enabled = nonsound == 2 class SEQUENCER_MT_strip_transform(Menu): @@ -1247,23 +1258,22 @@ class SEQUENCER_MT_context_menu(Menu): if strip: strip_type = strip.type - selected_sequences_count = selected_sequences_len(context) + total, nonsound = selected_strips_count(context) layout.separator() layout.operator_menu_enum("sequencer.strip_modifier_add", "type", text="Add Modifier") layout.operator("sequencer.strip_modifier_copy", text="Copy Modifiers to Selection") - if strip_type != 'SOUND': - if selected_sequences_count >= 2: + if total == 2: + if nonsound == 2: layout.separator() col = layout.column() col.menu("SEQUENCER_MT_add_transitions", text="Add Transition") - else: - if selected_sequences_count >= 2: + elif nonsound == 0: layout.separator() layout.operator("sequencer.crossfade_sounds", text="Crossfade Sounds") - if selected_sequences_count >= 1: + if total >= 1: col = layout.column() col.operator_menu_enum("sequencer.fades_add", "type", text="Fade") layout.operator("sequencer.fades_clear", text="Clear Fade") diff --git a/source/blender/editors/space_sequencer/sequencer_add.cc b/source/blender/editors/space_sequencer/sequencer_add.cc index 72381c4436f..c134f5458b9 100644 --- a/source/blender/editors/space_sequencer/sequencer_add.cc +++ b/source/blender/editors/space_sequencer/sequencer_add.cc @@ -1467,10 +1467,10 @@ static int sequencer_add_effect_strip_exec(bContext *C, wmOperator *op) SeqLoadData load_data; load_data_init_from_operator(&load_data, C, op); load_data.effect.type = RNA_enum_get(op->ptr, "type"); + const int num_inputs = SEQ_effect_get_num_inputs(load_data.effect.type); Strip *seq1, *seq2; - if (!strip_effect_find_selected(scene, nullptr, load_data.effect.type, &seq1, &seq2, &error_msg)) - { + if (!strip_effect_get_new_inputs(scene, false, num_inputs, &seq1, &seq2, &error_msg)) { BKE_report(op->reports, RPT_ERROR, error_msg); return OPERATOR_CANCELLED; } @@ -1537,37 +1537,44 @@ static std::string sequencer_add_effect_strip_get_description(bContext * /*C*/, switch (type) { case STRIP_TYPE_CROSS: - return TIP_("Add a crossfade transition to the sequencer"); + return TIP_("Add a crossfade transition strip for two selected strips with video content"); case STRIP_TYPE_ADD: - return TIP_("Add an add effect strip to the sequencer"); + return TIP_("Add an add blend mode effect strip for two selected strips with video content"); case STRIP_TYPE_SUB: - return TIP_("Add a subtract effect strip to the sequencer"); + return TIP_( + "Add a subtract blend mode effect strip for two selected strips with video content"); case STRIP_TYPE_ALPHAOVER: - return TIP_("Add an alpha over effect strip to the sequencer"); + return TIP_( + "Add an alpha over blend mode effect strip for two selected strips with video content"); case STRIP_TYPE_ALPHAUNDER: - return TIP_("Add an alpha under effect strip to the sequencer"); + return TIP_( + "Add an alpha under blend mode effect strip for two selected strips with video content"); case STRIP_TYPE_GAMCROSS: - return TIP_("Add a gamma cross transition to the sequencer"); + return TIP_("Add a gamma cross transition strip for two selected strips with video content"); case STRIP_TYPE_MUL: - return TIP_("Add a multiply effect strip to the sequencer"); + return TIP_( + "Add a multiply blend mode effect strip for two selected strips with video content"); case STRIP_TYPE_OVERDROP: - return TIP_("Add an alpha over drop effect strip to the sequencer"); + return TIP_( + "Add an alpha over drop blend mode effect strip for two selected strips with video " + "content"); case STRIP_TYPE_WIPE: - return TIP_("Add a wipe transition to the sequencer"); + return TIP_("Add a wipe transition strip for two selected strips with video content"); case STRIP_TYPE_GLOW: - return TIP_("Add a glow effect strip to the sequencer"); + return TIP_("Add a glow effect strip for a single selected strip with video content"); case STRIP_TYPE_TRANSFORM: - return TIP_("Add a transform effect strip to the sequencer"); + return TIP_("Add a transform effect strip for a single selected strip with video content"); case STRIP_TYPE_COLOR: return TIP_("Add a color strip to the sequencer"); case STRIP_TYPE_SPEED: - return TIP_("Add a speed effect strip to the sequencer"); + return TIP_("Add a video speed effect strip for a single selected strip with video content"); case STRIP_TYPE_MULTICAM: return TIP_("Add a multicam selector effect strip to the sequencer"); case STRIP_TYPE_ADJUSTMENT: return TIP_("Add an adjustment layer effect strip to the sequencer"); case STRIP_TYPE_GAUSSIAN_BLUR: - return TIP_("Add a gaussian blur effect strip to the sequencer"); + return TIP_( + "Add a gaussian blur effect strip for a single selected strip with video content"); case STRIP_TYPE_TEXT: return TIP_("Add a text strip to the sequencer"); case STRIP_TYPE_COLORMIX: diff --git a/source/blender/editors/space_sequencer/sequencer_edit.cc b/source/blender/editors/space_sequencer/sequencer_edit.cc index 7facd2d57e8..233fe129968 100644 --- a/source/blender/editors/space_sequencer/sequencer_edit.cc +++ b/source/blender/editors/space_sequencer/sequencer_edit.cc @@ -1200,111 +1200,91 @@ void SEQUENCER_OT_refresh_all(wmOperatorType *ot) /** \name Reassign Inputs Operator * \{ */ -int strip_effect_find_selected(Scene *scene, - Strip *activeseq, - int type, - Strip **r_selseq1, - Strip **r_selseq2, - const char **r_error_str) +bool strip_effect_get_new_inputs(Scene *scene, + bool ignore_active, + int num_inputs, + Strip **r_seq1, + Strip **r_seq2, + const char **r_error_str) { Editing *ed = SEQ_editing_get(scene); Strip *seq1 = nullptr, *seq2 = nullptr; *r_error_str = nullptr; - if (!activeseq) { - seq2 = SEQ_select_active_get(scene); + if (num_inputs == 0) { + *r_seq1 = *r_seq2 = nullptr; + return true; } - if (SEQ_effect_get_num_inputs(type) == 0) { - *r_selseq1 = *r_selseq2 = nullptr; - return 1; + blender::VectorSet new_inputs = SEQ_query_selected_strips(ed->seqbasep); + // Ignore sound strips for now (avoids unnecessary errors when connected strips are + // selected together, and the intent to operate on strips with video content is clear). + new_inputs.remove_if([&](Strip *strip) { return strip->type == STRIP_TYPE_SOUND_RAM; }); + + if (ignore_active) { + // If `ignore_active` is true, this function is being called from the reassign inputs + // operator, meaning the active strip must be the effect strip to reassign. + Strip *active_strip = SEQ_select_active_get(scene); + new_inputs.remove_if([&](Strip *strip) { return strip == active_strip; }); } - LISTBASE_FOREACH (Strip *, strip, ed->seqbasep) { - if (strip->flag & SELECT) { - if (strip->type == STRIP_TYPE_SOUND_RAM) { - *r_error_str = N_("Cannot apply effects to audio sequence strips"); - return 0; - } - if (!ELEM(strip, activeseq, seq2)) { - if (seq2 == nullptr) { - seq2 = strip; - } - else if (seq1 == nullptr) { - seq1 = strip; - } - else { - *r_error_str = N_("Cannot apply effect to more than 2 sequence strips"); - return 0; - } - } + if (new_inputs.size() > 2) { + *r_error_str = N_("Cannot apply effect to more than 2 sequence strips with video content"); + return false; + } + + if (num_inputs == 2) { + if (new_inputs.size() != 2) { + *r_error_str = N_("Exactly 2 selected sequence strips with video content are needed"); + return false; } + seq1 = new_inputs[0]; + seq2 = new_inputs[1]; + } + else if (num_inputs == 1) { + if (new_inputs.size() != 1) { + *r_error_str = N_("Exactly one selected sequence strip with video content is needed"); + return false; + } + seq1 = new_inputs[0]; } - switch (SEQ_effect_get_num_inputs(type)) { - case 1: - if (seq2 == nullptr) { - *r_error_str = N_("At least one selected sequence strip is needed"); - return 0; - } - if (seq1 == nullptr) { - seq1 = seq2; - } - ATTR_FALLTHROUGH; - case 2: - if (seq1 == nullptr || seq2 == nullptr) { - *r_error_str = N_("2 selected sequence strips are needed"); - return 0; - } - break; - } + *r_seq1 = seq1; + *r_seq2 = seq2; - if (seq1 == nullptr && seq2 == nullptr) { - *r_error_str = N_("TODO: in what cases does this happen?"); - return 0; - } - - *r_selseq1 = seq1; - *r_selseq2 = seq2; - - /* TODO(Richard): This function needs some refactoring, this is just quick hack for #73828. */ - if (SEQ_effect_get_num_inputs(type) < 2) { - *r_selseq2 = nullptr; - } - - return 1; + return true; } static int sequencer_reassign_inputs_exec(bContext *C, wmOperator *op) { Scene *scene = CTX_data_scene(C); - Strip *seq1, *seq2, *last_seq = SEQ_select_active_get(scene); + Strip *seq1, *seq2; + Strip *active_strip = SEQ_select_active_get(scene); const char *error_msg; + const int num_inputs = SEQ_effect_get_num_inputs(active_strip->type); - if (SEQ_effect_get_num_inputs(last_seq->type) == 0) { + if (num_inputs == 0) { BKE_report(op->reports, RPT_ERROR, "Cannot reassign inputs: strip has no inputs"); return OPERATOR_CANCELLED; } - if (!strip_effect_find_selected(scene, last_seq, last_seq->type, &seq1, &seq2, &error_msg) || - SEQ_effect_get_num_inputs(last_seq->type) == 0) - { + if (!strip_effect_get_new_inputs(scene, true, num_inputs, &seq1, &seq2, &error_msg)) { BKE_report(op->reports, RPT_ERROR, error_msg); return OPERATOR_CANCELLED; } /* Check if reassigning would create recursivity. */ - if (SEQ_relations_render_loop_check(seq1, last_seq) || - SEQ_relations_render_loop_check(seq2, last_seq)) + if (SEQ_relations_render_loop_check(seq1, active_strip) || + SEQ_relations_render_loop_check(seq2, active_strip)) { BKE_report(op->reports, RPT_ERROR, "Cannot reassign inputs: recursion detected"); return OPERATOR_CANCELLED; } - last_seq->seq1 = seq1; - last_seq->seq2 = seq2; + active_strip->seq1 = seq1; + active_strip->seq2 = seq2; - int old_start = last_seq->start; + int old_start = active_strip->start; /* Force time position update for reassigned effects. * TODO(Richard): This is because internally startdisp is still used, due to poor performance of @@ -1312,8 +1292,8 @@ static int sequencer_reassign_inputs_exec(bContext *C, wmOperator *op) SEQ_strip_lookup_invalidate(scene); SEQ_time_left_handle_frame_set(scene, seq1, SEQ_time_left_handle_frame_get(scene, seq1)); - SEQ_relations_invalidate_cache_preprocessed(scene, last_seq); - SEQ_offset_animdata(scene, last_seq, (last_seq->start - old_start)); + SEQ_relations_invalidate_cache_preprocessed(scene, active_strip); + SEQ_offset_animdata(scene, active_strip, (active_strip->start - old_start)); WM_event_add_notifier(C, NC_SCENE | ND_SEQUENCER, scene); @@ -1326,8 +1306,8 @@ static bool sequencer_effect_poll(bContext *C) Editing *ed = SEQ_editing_get(scene); if (ed) { - Strip *last_seq = SEQ_select_active_get(scene); - if (last_seq && (last_seq->type & STRIP_TYPE_EFFECT)) { + Strip *active_strip = SEQ_select_active_get(scene); + if (active_strip && (active_strip->type & STRIP_TYPE_EFFECT)) { return true; } } @@ -1359,18 +1339,18 @@ void SEQUENCER_OT_reassign_inputs(wmOperatorType *ot) static int sequencer_swap_inputs_exec(bContext *C, wmOperator *op) { Scene *scene = CTX_data_scene(C); - Strip *strip, *last_seq = SEQ_select_active_get(scene); + Strip *active_strip = SEQ_select_active_get(scene); - if (last_seq->seq1 == nullptr || last_seq->seq2 == nullptr) { + if (active_strip->seq1 == nullptr || active_strip->seq2 == nullptr) { BKE_report(op->reports, RPT_ERROR, "No valid inputs to swap"); return OPERATOR_CANCELLED; } - strip = last_seq->seq1; - last_seq->seq1 = last_seq->seq2; - last_seq->seq2 = strip; + Strip *strip = active_strip->seq1; + active_strip->seq1 = active_strip->seq2; + active_strip->seq2 = strip; - SEQ_relations_invalidate_cache_preprocessed(scene, last_seq); + SEQ_relations_invalidate_cache_preprocessed(scene, active_strip); WM_event_add_notifier(C, NC_SCENE | ND_SEQUENCER, scene); diff --git a/source/blender/editors/space_sequencer/sequencer_intern.hh b/source/blender/editors/space_sequencer/sequencer_intern.hh index 3c72e07eea9..cb53d146ce3 100644 --- a/source/blender/editors/space_sequencer/sequencer_intern.hh +++ b/source/blender/editors/space_sequencer/sequencer_intern.hh @@ -177,12 +177,12 @@ void channel_draw_context_init(const bContext *C, void strip_rectf(const Scene *scene, const Strip *strip, rctf *r_rect); Strip *find_neighboring_sequence(Scene *scene, Strip *test, int lr, int sel); void recurs_sel_seq(Strip *strip_meta); -int strip_effect_find_selected(Scene *scene, - Strip *activeseq, - int type, - Strip **r_selseq1, - Strip **r_selseq2, - const char **r_error_str); +bool strip_effect_get_new_inputs(Scene *scene, + bool ignore_active, + int strip_type, + Strip **r_seq1, + Strip **r_seq2, + const char **r_error_str); /* Operator helpers. */ bool sequencer_edit_poll(bContext *C);