The keyframes of strips attached to the main sequence or the meta strip did not move while moving the meta strip. This patch fixes this issue.
Details
Diff Detail
- Repository
- rB Blender
- Branch
- vse (branched from master)
- Build Status
Buildable 20711 Build 20711: arc lint + arc unit
Event Timeline
This works, with test file with single meta strip and effect, but when there are more animated strips, it will move animation of these strips too.
Not sure if I can come up with perfect solution now, but I think best would be to define this behavior in flushTransSeq(). Since when moving "base" strip, it's effect will be selected (but it's effect will be not!) you can say, "if strip can't be translated, only move animation". But as I said, there are problems still. For example if effect is transition, you don't necessarily want to move animation unless both input strips are selected and transformed.
So I think what should be done is first when operator starts, ensure that strips that are transformed in any way(either completely or animation only) will be selected. Then add exception in flushTransSeq(). To select all strips, function similar to SEQ_query_strip_effect_chain() can be used, but it will have to be specialized, to only downstream effects if all inputs are selected.
There still could be edge cases I did not think about, but this would be good place to start at least.
Shall I leave this for you to implrment?
In the function where all the effects are updated and translated in flushTransSeq(), the function SEQ_offset_animdata(t->scene, seq, offset) can be added to translate the animdata for the effects. This is working so far. Does this implementation work @Richard Antalik (ISS) ?
Something like this:
if (ELEM(t->mode, TFM_SEQ_SLIDE, TFM_TIME_TRANSLATE)) {
for (seq = seqbasep->first; seq; seq = seq->next) {
if (seq->seq1 || seq->seq2 || seq->seq3) {
SEQ_time_update_sequence(t->scene, seqbasep, seq);
SEQ_offset_animdata(t->scene, seq, offset);
}
}
}This would probably work, but function would have to be recursive and if you don't check for full relationship tree, then you can check only seq->seq1 and only single input effects, because for transition this would offset animdata twice as I said above.
I have noticed some code style/formatting issues, not sure if I mentioned style guide: https://wiki.blender.org/wiki/Style_Guide/C_Cpp. formatting is done automatically baced on clang-format rules, so depending on what IDE you are using, setup may be different(should be lined in wiki), or you can run make format.
Actually I have been able to make my example in last inline fail. It is relatively extreme case though:
I will try to find some bulletproof way to get needed strips and will post update if I succeed.
| source/blender/editors/transform/transform_convert_sequencer.c | ||
|---|---|---|
| 589 ↗ | (On Diff #49336) | This is technically correct, but what you really want are "generator" strips, which can be effects (text, color strip). So better would be to check for SEQ_effect_get_num_inputs(seq->type) == 0 But as I was experimenting a bit, I don't even think you need this function at all? Since you will be expanding this to include effects, if there are selected effects already, it doesn't hurt and actually more edge cases can be covered if you use seq_transform_collection_from_transdata instead. |
| source/blender/sequencer/intern/iterator.c | ||
| 362 ↗ | (On Diff #49336) | I think this should be statically defined in transform_convert_sequencer.c, it's pretty obscure function, which won't find much use elsewhere most likely. |
| 362–385 ↗ | (On Diff #49336) | Few points here: I think this function deserves comments otherwise it's quite hard to understand how it works. Then I think this stack of if statements can be optimized a bit. And finally, (seq->seq1->flag & SELECT) works in most cases, but I can still make it fail if multiple effects are under strip. Instead of checking for SELECT flag which may not be set correctly, you can check SEQ_collection_has_strip(collection, seq->seq1). Considering this is done only for each strips in collection that we start with, I think this can fail too though, but with first inline comment done locally I wasn't able to make this fail. So what I ended up looks like this: void SEQ_query_strip_effect_animation_chain(Sequence *seq_reference,
ListBase *seqbase,
SeqCollection *collection)
{
if (!SEQ_collection_append_strip(seq_reference, collection)) {
return; /* Strip is already in set, so all effects connected to it are as well. */
}
/* Find all strips connected to strips in collection, but not connected to any other strip. */
LISTBASE_FOREACH (Sequence *, seq, seqbase) {
if (seq->type & SEQ_TYPE_EFFECT) {
if (seq->seq1 && SEQ_collection_has_strip(seq->seq1, collection)) {
if (seq->seq2 && !SEQ_collection_has_strip(seq->seq2, collection)) {
continue;
}
SEQ_query_strip_effect_animation_chain(seq, seqbase, collection);
}
}
}
} |
| source/blender/editors/transform/transform_convert_sequencer.c | ||
|---|---|---|
| 774 ↗ | (On Diff #49336) | Also forgot, this data must be freed when not needed by SEQ_collection_free |
Sooo I think I have found the solution
I have created function that goes over relationships as you have suggested but I do actually select the strips
/* Basically, select if seq->seq1 is in collection and selected, but if there is seq->seq2, select
* only if both seq1 and seq2 are in collection and selected. */
void transform_convert_sequencer_select_related_strips(SeqCollection *animation_strips)
{
bool strip_selected = true;
while (strip_selected) {
strip_selected = false;
Sequence *seq;
SEQ_ITERATOR_FOREACH (seq, animation_strips) {
if ((seq->flag & SELECT) != 0) {
continue; /* Strip is already selected, skip it. */
}
if (seq->seq1 && (seq->seq1->flag & SELECT)) {
if (seq->seq2 && (seq->seq2->flag & SELECT) == 0) {
continue;
}
strip_selected = true;
seq->flag |= SELECT;
}
}
}
}Then I do this magic:
/* Update the animation of effects*/ ListBase *seqbase = SEQ_active_seqbase_get(SEQ_editing_get(t->scene)); SeqCollection *animation_strips = SEQ_query_selected_strips(seqbase); /* Get effect chain so we can traverse full relationship tree. */ SEQ_collection_expand(seqbase_active_get(t), animation_strips, SEQ_query_strip_effect_chain); /* Select strips with complete relationship (all children selected). */ transform_convert_sequencer_select_related_strips(animation_strips); /* Filter out strips with incomplete relationships, where we don't want to change animation. */ SEQ_filter_selected_strips(animation_strips);
It could fill new collection instead of selecting strip, but for transform operator this is actually good. Now issue is, that this operator already tries to do this, but id fails very badly. So sometimes you can see badly synchronized strips when moving them (should happen with my example file). So if you replace that with this complete relationship parsing, that's pretty much job done.
You can then do
if (!SEQ_transform_sequence_can_be_translated(seq)) SEQ_offset_animdata(t->scene, seq, offset);
With what seq_transform_collection_from_transdata will output. So this will not only fix animation, but also bunch of other problems.
How did you come up with the solution? I mean parsing all the selected strips then adding their effects and removing the unwanted strips ? How did you come up with this magic :)
I have committed 32da64c17e9d509bc8ce534b1fdf03d748ca13c8 which is bit more complicated than what I have suggested. So will close this patch.