Fix: VSE Slip Strips operator doesn't move effect strips inside of a meta strip. #113200

ok what · 2023-10-03T12:26:21+02:00

ok what commented

2023-10-03 12:26:21 +02:00

Steps to reproduce:

Add two strips, and add a transition strip to them.
Group all of them inside a meta strip
Use the "Slip Strip Contents" operator on the meta strip.

Result: The transition strip isn't moved, and gets off-sync with the input strips.

Notes:

Should the function that slips the content of a strip be moved inside source/blender/sequencer (eg. strip_time.cc), rather than exposing functions internal to /sequencer?
- I've temporarily moved the internal functions to make them public. If it's better to merge the bug fix first before making other changes, I'll rename them to start with capital "SEQ_"
Slip doesn't move the keyframes of a strip. I will make a separate PR for that. This is why I don't exclude the effect strips directly in slip_add_sequences_recursive(...).
There is a bug with scene strips, where a negative strip offset start value causes the scene audio to play at the wrong time. This is not specific to the slip operator, since it happens when dragging the left strip handle to the left as well.

**Steps to reproduce:** 1. Add two strips, and add a transition strip to them. 2. Group all of them inside a meta strip 3. Use the "Slip Strip Contents" operator on the meta strip. Result: The transition strip isn't moved, and gets off-sync with the input strips. **Notes:** 1. Should the function that slips the content of a strip be moved inside source/blender/sequencer (eg. strip_time.cc), rather than exposing functions internal to /sequencer? - I've temporarily moved the internal functions to make them public. If it's better to merge the bug fix first before making other changes, I'll rename them to start with capital "SEQ_" 2. Slip doesn't move the keyframes of a strip. I will make a separate PR for that. This is why I don't exclude the effect strips directly in slip_add_sequences_recursive(...). 3. There is a bug with scene strips, where a negative strip offset start value causes the scene audio to play at the wrong time. This is not specific to the slip operator, since it happens when dragging the left strip handle to the left as well.

ok what added 1 commit 2023-10-03 12:26:33 +02:00

fcd5a26240 Fix: VSE Slip Strips operator doesn't move effect strips inside of a meta strip.

ok what requested review from Richard Antalik 2023-10-03 12:29:08 +02:00

Richard Antalik commented

2023-10-09 20:19:42 +02:00

Thanks for patch and sorry for late reply.

Should the function that slips the content of a strip be moved inside source/blender/sequencer (eg. strip_time.cc), rather than exposing functions internal to /sequencer?
I've temporarily moved the internal functions to make them public. If it's better to merge the bug fix first before making other changes, I'll rename them to start with capital "SEQ_"

Technically, any code in space_sequencer should not directly change value of Sequence. If it does, things like this would happen...
The lookup function I would rather not expose and exposing seq_time_update_effects_strip_range() is worst case scenario.

Solution should be to use API functions in transseq_backup() and counterparts elsewhere, like:

static void transseq_backup(TransSeq *ts, Sequence *seq)
{
  ts->content_start = SEQ_time_start_frame_get(seq);
  ts->timeline_start = SEQ_time_left_handle_frame_get(scene, seq);
  ts->timeline_end = SEQ_time_right_handle_frame_get(scene, seq);
  ts->anim_startofs = seq->anim_startofs;
  ts->anim_endofs = seq->anim_endofs;
  ts->len = SEQ_time_strip_length_get(scene, seq);
}

All SEQ_time_..._set functions that need to should update effects position.

I think I have tried to do this, but it had some issue. Perhaps that was before I had effects cache, I am not sure now. Note, that is no API to handle anim_startofs and anim_endofs, because I want it gone. That should not cause issues here though.

Slip doesn't move the keyframes of a strip. I will make a separate PR for that. This is why I don't exclude the effect strips directly in slip_add_sequences_recursive(...).

Sounds like a good idea.

There is a bug with scene strips, where a negative strip offset start value causes the scene audio to play at the wrong time. This is not specific to the slip operator, since it happens when dragging the left strip handle to the left as well.

I think this is already reported and classified as known issue. Audio evaluates from single time source, there may be some hacks to work around it, but it's hard to resolve this. Or at least it requires good knowledge of dependency graph and not sure what time investment would be.

Thanks for patch and sorry for late reply. >Should the function that slips the content of a strip be moved inside source/blender/sequencer (eg. strip_time.cc), rather than exposing functions internal to /sequencer? I've temporarily moved the internal functions to make them public. If it's better to merge the bug fix first before making other changes, I'll rename them to start with capital "SEQ_" Technically, any code in `space_sequencer` should not directly change value of `Sequence`. If it does, things like this would happen... The lookup function I would rather not expose and exposing `seq_time_update_effects_strip_range()` is worst case scenario. Solution should be to use API functions in `transseq_backup()` and counterparts elsewhere, like: ``` static void transseq_backup(TransSeq *ts, Sequence *seq) { ts->content_start = SEQ_time_start_frame_get(seq); ts->timeline_start = SEQ_time_left_handle_frame_get(scene, seq); ts->timeline_end = SEQ_time_right_handle_frame_get(scene, seq); ts->anim_startofs = seq->anim_startofs; ts->anim_endofs = seq->anim_endofs; ts->len = SEQ_time_strip_length_get(scene, seq); } ``` All `SEQ_time_..._set` functions that need to should update effects position. I think I have tried to do this, but it had some issue. Perhaps that was before I had effects cache, I am not sure now. Note, that is no API to handle `anim_startofs` and `anim_endofs`, because I want it gone. That should not cause issues here though. > Slip doesn't move the keyframes of a strip. I will make a separate PR for that. This is why I don't exclude the effect strips directly in slip_add_sequences_recursive(...). Sounds like a good idea. > There is a bug with scene strips, where a negative strip offset start value causes the scene audio to play at the wrong time. This is not specific to the slip operator, since it happens when dragging the left strip handle to the left as well. I think this is already reported and classified as known issue. Audio evaluates from single time source, there may be some hacks to work around it, but it's hard to resolve this. Or at least it requires good knowledge of dependency graph and not sure what time investment would be.

ok what added 2 commits 2023-12-20 18:10:59 +01:00

d14a71d859 Merge branch 'main' into vse-slip-effect-bug

910690690a - Recursion in #sequencer_slip_recursively

- Relative offset
- Fix bug with canceling slip operator

ok what commented

2023-12-20 18:21:05 +01:00

Sorry for the very late reply.
I've tried to keep the refactoring in this PR to a minimum, but I had to change some things:

recursion for meta strips is now done in sequencer_slip_recursively instead of slip_add_sequences_recursive
i added a SEQ_time_slip_strip function
the offset used for sequencer_slip_recursively is now relative to the strip position on the timeline. This gets rid of the backup and restore functions that caused bugs (canceling the operator wouldn't work properly). At first I tried adapting the existing functions to the API functions, but I think this is less error prone. It also now makes it easy to move the f-curves while slipping.
Removed call to SEQ_add_reload_new_file. Not quite sure why it was in there, but with the relative offset it shouldn't be necessary.

The caching I'm unsure about. Currently SEQ_relations_invalidate_cache_preprocessed is only called for the selected strips, not for strips inside metas. This seems to be the case for SEQ_transform_translate_sequence and the places where it is used as well. The caching system seems a bit much for me to dive into at the moment, so I'm not sure what the correct way to go about it is.

Should SEQ_time_slip_strip be in strip_edit.cc or strip_time.cc?

If you want, I can also try if this could be done with the existing API functions, by exposing seq_time_translate_handles and using SEQ_transform_translate_sequence for meta strip contents. When I initially tried it, it didn't work, because SEQ_time_update_meta_strip_range would end up being called prematurely (and unnecessarily), with strips being calculated in the wrong order. Now that it's recursive anyway and the order is predictable, I think it would work.
I personally think it's much more predictable and understandable the way it is now though, without any extra calculations.

Sorry for the very late reply. I've tried to keep the refactoring in this PR to a minimum, but I had to change some things: - recursion for meta strips is now done in `sequencer_slip_recursively` instead of `slip_add_sequences_recursive` - i added a `SEQ_time_slip_strip` function - the offset used for `sequencer_slip_recursively` is now relative to the strip position on the timeline. This gets rid of the backup and restore functions that caused bugs (canceling the operator wouldn't work properly). At first I tried adapting the existing functions to the API functions, but I think this is less error prone. It also now makes it easy to move the f-curves while slipping. - Removed call to `SEQ_add_reload_new_file`. Not quite sure why it was in there, but with the relative offset it shouldn't be necessary. The caching I'm unsure about. Currently `SEQ_relations_invalidate_cache_preprocessed` is only called for the selected strips, not for strips inside metas. This seems to be the case for `SEQ_transform_translate_sequence` and the places where it is used as well. The caching system seems a bit much for me to dive into at the moment, so I'm not sure what the correct way to go about it is. Should `SEQ_time_slip_strip` be in `strip_edit.cc` or `strip_time.cc`? If you want, I can also try if this could be done with the existing API functions, by exposing `seq_time_translate_handles` and using `SEQ_transform_translate_sequence` for meta strip contents. When I initially tried it, it didn't work, because `SEQ_time_update_meta_strip_range` would end up being called prematurely (and unnecessarily), with strips being calculated in the wrong order. Now that it's recursive anyway and the order is predictable, I think it would work. I personally think it's much more predictable and understandable the way it is now though, without any extra calculations.

Richard Antalik approved these changes 2023-12-20 21:02:09 +01:00

Richard Antalik left a comment

I started review before finishing reading your comment, so I suggested to not do recursive call of seq_time_slip_strip_ex and use SEQ_transform_translate_sequence() for moving stuff in metas... Also tried it quickly and it didn't work nicely. IMO the code would be nicer that way, but seems this is not practical for now.

Also was thinking of using seq_time_translate_handles just to avoid start/enddisp magic, but this also updates effects, so not sure if that would be practical either.

That said, I think I will accept this as is. I mean, this is supposed to be bugfix anyway. Still have minor nitpicks. But I can change these before committing.

I started review before finishing reading your comment, so I suggested to not do recursive call of `seq_time_slip_strip_ex` and use `SEQ_transform_translate_sequence()` for moving stuff in metas... Also tried it quickly and it didn't work nicely. IMO the code would be nicer that way, but seems this is not practical for now. Also was thinking of using `seq_time_translate_handles` just to avoid start/enddisp magic, but this also updates effects, so not sure if that would be practical either. That said, I think I will accept this as is. I mean, this is supposed to be bugfix anyway. Still have minor nitpicks. But I can change these before committing.

source/blender/editors/space_sequencer/sequencer_edit.cc Outdated

						
				@ -605,3 +559,3 @@

				}

				static void sequencer_slip_recursively(Scene *scene, SlipData *data, int offset)

				static void sequencer_slip_recursively(Scene *scene, SlipData *data, int delta)

Richard Antalik commented

2023-12-20 20:16:18 +01:00

I guess this function is not recursive anymore?

source/blender/sequencer/intern/strip_time.cc

						
				@ -579,0 +602,4 @@

				      return;

				    }

				    Sequence *seq_child;

				    for (seq_child = static_cast<Sequence *>(seq->seqbase.first); seq_child;

Richard Antalik commented

2023-12-20 20:19:36 +01:00

Please use LISTBASE_FOREACH macro

Please use `LISTBASE_FOREACH` macro

ok_what marked this conversation as resolved

ok what added 1 commit 2023-12-21 15:47:05 +01:00

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

693dfa3495 Changes from code review suggestions

ok what changed title from ~~WIP: Fix: VSE Slip Strips operator doesn't move effect strips inside of a meta strip.~~ to Fix: VSE Slip Strips operator doesn't move effect strips inside of a meta strip.

2023-12-21 15:48:18 +01:00

ok what requested review from Richard Antalik 2023-12-21 15:54:43 +01:00

Richard Antalik commented

2023-12-21 17:13:26 +01:00

Eeh and let's not forget this time.. @blender-bot build
Seem it doesn't work this way, wanted to try it anyway :)

Eeh and let's not forget this time.. @blender-bot build Seem it doesn't work this way, wanted to try it anyway :)

Richard Antalik commented

2023-12-21 17:13:58 +01:00

@blender-bot build

Richard Antalik merged commit 04efca91f9 into main

2023-12-21 18:05:07 +01:00

Richard Antalik referenced this issue from a commit