Created meta.separate() function
Created SEQ_edit_move_strip_to_seqbase
Details
- Reviewers
Richard Antalik (ISS)
Diff Detail
- Repository
- rB Blender
- Branch
- ft_meta_separate (branched from master)
- Build Status
Buildable 16115 Build 16115: arc lint + arc unit
Event Timeline
I still have to check this out in more detail, but I would probably prefer to keep this operator in C unless there is good reason to move it to python. At this point it's mostly because majority of operators are in C, so it would be a bit odd. Also it may happen that we would want to add some functionality that is not available or even possible to do from python. Sorry if there was some miscommunication on my part.
If the meta.separate() function is valid anyhow we could merge only that.
Regarding the refactor of the operator in python, moving it to python wasn't the biggest part, then this is not a big issue if this is not merged with. It seems I did it properly, then only the first commit is related to the C API and the second one to Python refactor.
I added the Python operator refactor because while I was coding, I remembered this sentence: "Python code is easier to maintain. Python operators will validate and test API. I believe, that all operators should use API only, regardless of language." from https://developer.blender.org/T65724, then I thought to go further and help you one step beyond.
Yes, in general I agree. This patch ticks all boxes too. But not sure if I would start with this one. Also looking at how refactoring is now progressing extremely slowly I wouldn't see it as best move unless it's needed. But if some python operator needed this functionality, it is provided now, which is great.
Okay then, I'll rely on your schedule ;)
Shall I do anything? or everything is up to you now?
Re-reading it, I think it would be better to move that to rna_sequencer.c -> rna_def_meta to have the function accessible only for meta sequences.
That is not bad idea I would say.
My concern is mostly on C side, because I would probably think, that I would like to retain flexibility. So for example let's say, that I want to add support for seperating many metastrips at once - it may be better to move strips from all metas first, then do updates for all strips at once. This is just hypotethical scenario, because strip updates should happen internally in SEQ_ functions and ideally it should be possible to do updates per strip, but it is not always the case.
I was thinking implementation like this:
static void rna_Sequence_separate(ID *id, Sequence *meta_self, Main *bmain, ReportList *reports)
LISTBASE_FOREACH_MUTABLE (Sequence *, seq, &seqm->seqbase) {
SEQ_edit_move_strip_to_seqbase(scene, seq, seqbase); // Does updates and what not, `SEQ_edit_move_strip_to_meta` could also use this function or be replaced by it.
}
SEQ_edit_flag_for_removal(scene, seqbase, seqm);
SEQ_edit_remove_flagged_sequences(scene, seqbase);Idealy this code could be used for C operator, even more ideally this would be portable to python code almost line by line.
So I am not against this API function, but I am against using it for operator. I would check if code above would work for sequencer_meta_separate_exec and rna_Sequence_separate. I wouldn't simplify this to single function like SEQ_meta_separate. If there are issues with code above (shouldn't be) then I think your solution is probably only way to do this currently and would be probably best to go with that. Any eventualities later would have to be dealt with as they come.
Sorry if this is too nitpicky, I just want to explain how I would do this and why.
| source/blender/sequencer/intern/strip_edit.c | ||
|---|---|---|
| 311–330 | This doesn't make sense - you are moving meta strip with it's effects elsewhere and then removing it anyway. | |
Thanks for update, will check this patch next week, I want to focus on going through backlog of bugs reported in 3.0 this week.
Seems to work well. Will commit without changes in sequencer_edit.c, these look like outdated / unintended changes. Sorry for delay BTW, I took week off last week.
Oops managed to add completely unrelated task to commit as reference.
Implemented in 9cf3d841a823a7b015d4f7be7ee27c9ce88ad6e0, so will close revision.