Feature: sequence_editor.display_stack(meta_seq) function
great thanks to @Élie Michel (elie) for his wonderful help!
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- ft_meta_stack_set (branched from master)
- Build Status
Buildable 22701 Build 22701: arc lint + arc unit
Event Timeline
Comment Actions
I think it would be nicer design, if there would be only sequence_editor.display_stack() function and you can provide either sequence or None. So you can do sequence_editor.display_stack(sequence_editor.meta_stack[-1]) to go up one level for example.
I have pretty much same argument for sequencer.meta_toggle operator, I would rather keep it in C as it is core functionality and we may want to modify it in a way, that wouldn't be possible in python.
Changing displayed stack seems to work, but this doenn't refresh view properly and I can see strips from parent stack until I click somewhere. I think this may be because selection must be cleared in all other stacks (in practice only last displayed), but I may be mistaken.
Comment Actions
I agree about the design you propose. It's cleaner. I'll change that.
About the operator, maybe I didn't fix everything correctly, I'll have a look.
Regarding the fact we should keep it in C, I believe, and also I need, to be able to override this kind of operators for my project (Stax). I need to handle Blender native operators with some extra operations to make it transparent for users: they use Blender as they're used to doing it, but Stax does more things in background. If they want to keep using their shortcuts and not Stax's keymap. Currently, I'm forced to create a sequencer.toggle_meta to override the sequencer.meta_toggle and to force Stax's keymap installation at startup, else this operator would be accessible only in the UI and breaks Blender workflow consistency.
I agree we may face a case where an additional feature doesn't exist yet in Python, but a great design would be to have all the python's API functions for core C functions we need to have exposed, and manage operators only in Python.
Comment Actions
- Keep meta_toggle operator to C and API: sequence_editor.display_stack(meta_seq or None)
Comment Actions
There are some issues with functionality.
- When "opening" meta strip, strips from different seqbase are visible. I think this is because strip is selected and drawing code treats this in special way. We should restrict this code to only draw strips in current seqbase. But not 100% sure this is actually what is causing problems. Don't see anything wrong with code in this regard at least...
- You can enter non-meta strip via python API, that shouldn't be possible to do. I think python function should report error in this case.
- Executing C.scene.sequence_editor.display_stack(None) in top seqbase will cause crash, because SEQ_meta_stack_active_get will return NULL
Otherwise this looks nice.
Comment Actions
- Feature: sequence_editor.display_stack(meta_seq) function
- Sentinel for non-meta seq
- Safe display_stack(None)
- Fill seq_ed.meta_stack with created SEQ_get_sequence_metas_ancestors
Comment Actions
@Richard Antalik (ISS) I'd like to add a sequence.metas_hierarchy() which would return the whole tree of metas to the sequence, using the SEQ_get_sequence_metas_ancestors function I introduced here. Shall I add it there or create a new patch when this one will be applied?
Comment Actions
Functionally this looks good.
Now first comment in code, actually may be doable to have only one function, because I wanted to suggest - I think, that the code from rna_SequenceEditor_display_stack which is functional, should actually go to SEQ_display_stack, otherwise I don't think there is even need for having function SEQ_display_stack. In other words, it is not adding any API functionality, only wrapping existing functionality. I will try to do quick diff as example of how this could work.
| source/blender/editors/space_sequencer/sequencer_edit.c | ||
|---|---|---|
| 1924–1931 | I don't think this change is necessary. Since you don't do jumps more than 1 level, I would assume it would be guaranteed, that parent_meta->seqbase == ms->oldbasep so there is no need to use SEQ_find_metastrip_by_sequence at all. On one hand if I know nothing about how this works and I step through the code, this may be quite confusing + adds unnecessary tree traversing, which is not great to add unless there is good reason. On the other hand, there is potential to make this more elegant by eventually elliminating metastack visibility for operators and have only one function for navigation between meta strips, but this would need more work or even design change. So I am undecided here, but would lean towards not changing this operator at all and just adding comment like "TODO: make SEQ_display_stack handle MetaStack internally so there is only 1 function needed for navigation". | |
| source/blender/makesrna/intern/rna_sequencer.c | ||
| 1112 | Eh, what is "sentinel"? Also add fullstop at the end of comments. | |
| 1119 | You can't just clear ListBase, you will have to free elements or you will cause memory leak. BLI_freelistN does free elements and removes them from list. | |
| source/blender/sequencer/SEQ_relations.h | ||
| 84 | I guess this should be SEQ_get_sequence_metas_ancestors | |
| source/blender/sequencer/intern/strip_relations.c | ||
| 475 | Prefer using LISTBASE_FOREACH for iterating - LISTBASE_FOREACH (Sequence *, iseq, seqbase) | |
| 482 | !BLI_listbase_is_empty(&iseq->seqbase) is semantically cleaner and may be even faster by 1.5 CPU cycles :) | |
Comment Actions
So in this patch (P2877), I moved functional stuff to SEQ_display_stack. This allowed me to make SEQ_get_sequence_metas_ancestors static, which allowed me to get rid of passing data via ListBase.
I have renamed these functions to SEQ_meta_stack_set and seq_meta_stack_set_recursive just to not introduce another terms. IMO even display_stack rna function should be renamed to meta_stack_set, since this meta stack is used and known term. maybe even meta_stack_active_set, since there is SEQ_meta_stack_active_get? Not sure, but these are details.
Now back to meta toggle operator, entering meta strip is reduced basically to simple SEQ_meta_stack_set call which is nice. no need to manually alloc or free anything, so these functions can be hidden. but exitting meta is still not nice, so I propose meta_stack_pop function, which can be used for this operator. Still not too great, because this selects strip, which is stored in MetaStack, so there still needs to be SEQ_meta_stack_active_get. However, I can return this strip as reference. So now there is no need to manipulate data manually, all is covered by API. Function SEQ_meta_stack_active_get s still used internally, so I can move it to sequencer.h file with lowercase seq prefix.
I have written this as I went along and modified the code, so hopefully you can follow my train of logic. Patch will show you only final modified version... Also sorry for not making my patch clean, mainly wanted to do quick edits that work (even that is not quite guaranteed, eh)
Also forgot to mention, that now function documentation comments are moved to header files.