Page MenuHome
Differential D12278

Cleanup and remove SEQ_ALL_BEGIN macro
ClosedPublic
Actions

Authored by Sebastian Parborg (zeddb) on Aug 20 2021, 4:25 PM.

Details

Reviewers
Richard Antalik (ISS)
Bastien Montagne (mont29)
Commits
rBf49d438ced7c: Cleanup and remove SEQ_ALL_BEGIN macro
Summary

We now use a for_each function with callback to iterate through all sequences in the scene.

This has the benefit that we now only loop over the sequences in the scene once.
Before we would loop over them twice and allocate memory to store temporary data.

The allocation of temporary data lead to unintentional memory leaks if the code used returns to exit out of the iteration loop.
The new for_each callback method doesn't allocate any temporary data and only iterates though all sequences once.

Diff Detail

Event Timeline

Sebastian Parborg (zeddb) requested review of this revision.Aug 20 2021, 4:25 PM
Sebastian Parborg (zeddb) updated this revision to Diff 40910.
Sebastian Parborg (zeddb) created this revision.
Sebastian Parborg (zeddb) added a subscriber: Bastien Montagne (mont29).Aug 20 2021, 4:35 PM

I'll just tag @Bastien Montagne (mont29) as he wanted to do a double check on the for_each logic

Richard Antalik (ISS) added a comment.Aug 24 2021, 5:39 AM

I would probably fear, that you would have to use callback custom data struct a lot, but there are only 5 places where this is done, so not bad really. Nice to see .blend read/write code moved to module, and merging functionality with SEQ_seqbase_recursive_apply.

Is it no longer style guideline, that callback functions should use _fn suffix? I can see it only in T73586 not on wiki. Or perhaps it never was?

Just to state my position here too, I think that iterator macro was nice to have for readability in cases where you want to use few statements in loop. But I am OK with this change.
Worst seems to be returning value via custom data if I understood logic correctly.

source/blender/makesrna/intern/rna_color.c
660

I think here it should be if(cb_data.seq)? If so, perhaps it's good idea to signify, that Seq_colorspace_cb_data.seq is actually return value?

source/blender/sequencer/intern/iterator.c
102
293–303

This should work too.

source/blender/sequencer/intern/sequencer.c
745–751

I thought you wanted to remove this code?

Sebastian Parborg (zeddb) marked an inline comment as done.Aug 24 2021, 2:25 PM
Sebastian Parborg (zeddb) added inline comments.
source/blender/makesrna/intern/rna_color.c
660

Right you are! Thanks for catching this.

How do you want me to signify that cb_data.seq is actually written to?

source/blender/sequencer/intern/sequencer.c
745–751

That will be in a later patch. I didn't want to back in the removal/cleanup of this here.

Sebastian Parborg (zeddb) updated this revision to Diff 41061.Aug 24 2021, 7:05 PM
Sebastian Parborg (zeddb) marked 3 inline comments as done.
Richard Antalik (ISS) accepted this revision.Aug 25 2021, 1:36 AM
Richard Antalik (ISS) added inline comments.
source/blender/makesrna/intern/rna_color.c
660

Probably just use name r_seq or comment in code, that callback will return data that will be used elsewhere. Though it is quite obvious from code itself.

This revision is now accepted and ready to land.Aug 25 2021, 1:36 AM
Bastien Montagne (mont29) requested changes to this revision.Aug 25 2021, 3:44 PM
Bastien Montagne (mont29) added inline comments.
source/blender/sequencer/intern/iterator.c
105

Would add a recursive to the name?

Or even better, move code into a private seq_for_each_callback_recursive function, and not return anything from the public main API callback. If the callback wants to stop recursion or have any other kind of custom return value, it should do so through its customdata. That boolean return value should be exclusive to the internal looping mechanism of this foreach looper.

106

Please use a proper public typdef functype, see e.g. ConstraintIDFunc.

This revision now requires changes to proceed.Aug 25 2021, 3:44 PM
Sebastian Parborg (zeddb) updated this revision to Diff 41091.Aug 25 2021, 4:36 PM
Sebastian Parborg (zeddb) marked 2 inline comments as done.
Sebastian Parborg (zeddb) marked 2 inline comments as done.
Bastien Montagne (mont29) accepted this revision.Aug 25 2021, 4:59 PM

LGTM

This revision is now accepted and ready to land.Aug 25 2021, 4:59 PM
Closed by commit rBf49d438ced7c: Cleanup and remove SEQ_ALL_BEGIN macro (authored by Sebastian Parborg (zeddb)). · Explain WhyAug 25 2021, 5:43 PM
This revision was automatically updated to reflect the committed changes.
Sebastian Parborg (zeddb) added a commit: rBf49d438ced7c: Cleanup and remove SEQ_ALL_BEGIN macro.

Revision Contents
Changeset List

PathSize
source/
blender/
blenkernel/
8 lines
intern/
74 lines
100 lines
346 lines
blenloader/
intern/
6 lines
80 lines
96 lines
79 lines
21 lines
32 lines
depsgraph/
intern/
builder/
44 lines
74 lines
eval/
48 lines
editors/
sound/
10 lines
space_sequencer/
26 lines
imbuf/
intern/
16 lines
makesrna/
intern/
40 lines
40 lines
sequencer/
25 lines
20 lines
6 lines
intern/
2 lines
57 lines
2 lines
5 lines
342 lines
35 lines
34 lines

Diff 41091

View Options

source/blender/blenkernel/BKE_scene.h

Loading...
View Options

source/blender/blenkernel/intern/bpath.c

Loading...
View Options

source/blender/blenkernel/intern/ipo.c

Loading...
View Options

source/blender/blenkernel/intern/scene.c

Loading...
View Options

source/blender/blenloader/intern/readfile.c

Loading...
View Options

source/blender/blenloader/intern/versioning_250.c

Loading...
View Options

source/blender/blenloader/intern/versioning_260.c

Loading...
View Options

source/blender/blenloader/intern/versioning_270.c

Loading...
View Options

source/blender/blenloader/intern/versioning_280.c

Loading...
View Options

source/blender/blenloader/intern/versioning_legacy.c

Loading...
View Options

source/blender/depsgraph/intern/builder/deg_builder_nodes.cc

Loading...
View Options

source/blender/depsgraph/intern/builder/deg_builder_relations.cc

Loading...
View Options

source/blender/depsgraph/intern/eval/deg_eval_runtime_backup_sequencer.cc

Loading...
View Options

source/blender/editors/sound/sound_ops.c

Loading...
View Options

source/blender/editors/space_sequencer/sequencer_edit.c

Loading...
View Options

source/blender/imbuf/intern/colormanagement.c

Loading...
View Options

source/blender/makesrna/intern/rna_color.c

Loading...
View Options

source/blender/makesrna/intern/rna_sequencer.c

Loading...
View Options

source/blender/sequencer/SEQ_iterator.h

Loading...
View Options

source/blender/sequencer/SEQ_sequencer.h

Loading...
View Options

source/blender/sequencer/SEQ_utils.h

Loading...
View Options

source/blender/sequencer/intern/clipboard.c

Loading...
View Options

source/blender/sequencer/intern/iterator.c

Loading...
View Options

source/blender/sequencer/intern/proxy.c

Loading...
View Options

source/blender/sequencer/intern/sequencer.h

Loading...
View Options

source/blender/sequencer/intern/sequencer.c

Loading...
View Options

source/blender/sequencer/intern/strip_relations.c

Loading...
View Options

source/blender/sequencer/intern/utils.c

Loading...