Move sequencer sources from blenkernel
ClosedPublic
Actions

Authored by Richard Antalik (ISS) on Aug 7 2020, 7:58 AM.

Details

Reviewers

Sergey Sharybin (sergey)
Brecht Van Lommel (brecht)

Commits

rB18d7aeacf2f7: Move sequencer sources from blenkernel

Summary

This is first step of refactoring task T77580.
Next step would be moving smaller portion of sequencer.c file back into BKE and renaming remaining files to get SEQ_ prefix.

Just a note, that there may be obvious stupidity in some cmakelists files, I mostly looked in existing files and copied what I assume should be in there, and fixed compile errors until I was able to compile sucessfully.

I am dfinitely unsure about chenge in windowmanager library (I was copying what compositor does):

set(LIB
  bf_editor_screen
  bf_sequencer
)

Diff Detail

Repository

rB Blender

Branch

arcpatch-D8492 (branched from master)

Build Status

Buildable 10102
Build 10102: arc lint + arc unit

Event Timeline

Richard Antalik (ISS) requested review of this revision.Aug 7 2020, 7:58 AM

Richard Antalik (ISS) created this revision.

Harbormaster completed remote builds in B9397: Diff 27502.Aug 7 2020, 7:58 AM

Richard Antalik (ISS) edited the summary of this revision. (Show Details)Aug 7 2020, 8:10 AM

Generally fine, but needs a bit of work to polish. Mainly, I don't think sequencer.c belongs to editor folder. See inlined comment about it. And am open for other suggestions.

What I'm wondering about though, is what is the proper approach to migrate from BKE_ prefix. I kind of like the idea of separate commits to move files around and to rename functions after that. Wondering what's Brecht opinion here.

source/blender/sequencer/CMakeLists.txt
23–24	I would suggest against adding those subfolders and move to includes relative to the `source/blender/sequencer`.
44–48	Not sure why `sequencer.c` is in editor. It contains a lot of non-editor related logic and that would need to be moved somewhere. Is this something more like `core` ?
45	I think cache can be moved to own folder. The current `seqcache.c` can be split into in-meory and on-disk caches. Maybe even more, to keep files at a more easy-to-navigate state.
46–48	I would remove `seq` prefix.

I'd first move the files in one commit, then rename functions in another commit. Otherwise I'm not sure git will detect them as moved files.

This revision is now accepted and ready to land.Sep 9 2020, 4:48 PM

Talked to Brecht in real life.

The suggestion is to move all files under source/blender/sequencer/intern.c, and introduce sub-folders under the intern once the files are actually being split.
Another interesting topic we've quickly run over, is where the ID/library management code should live (blenkernel vs. sequencer). From black-boxing point of view they should belong to sequencer, but this could cause linker issues because blenkernel need to access library management functionality.

Remove subfolders from include paths
Move cache to own folder and remove file prefixes

Harbormaster completed remote builds in B10102: Diff 28717.Sep 10 2020, 1:05 AM

In D8492#216533, @Sergey Sharybin (sergey) wrote:

Talked to Brecht in real life.

The suggestion is to move all files under source/blender/sequencer/intern.c, and introduce sub-folders under the intern once the files are actually being split.

Yes most files will be in intern subfolder with own includes

Another interesting topic we've quickly run over, is where the ID/library management code should live (blenkernel vs. sequencer). From black-boxing point of view they should belong to sequencer, but this could cause linker issues because blenkernel need to access library management functionality.

I may not have as deep insight to understand this problem in detail, but I think that ID/library management should be in BKE and follow same rules as other datablocks do. Isn't it possible for these management functions to call functions outside of BKE? I think that data management code should be "self contained" in BKE, with reasonable "interface" for update functions and such if necessary.
I guess It would be better if I had more concrete vision here, but also we aren't really using ID structs in sequencer yet.

source/blender/sequencer/CMakeLists.txt
44–48	It contains quite wide variety of code - lot of low level stuff, lot of rendering functions and finally lot of editor code. Low level stuff is supposed to go to BKE, rendering to render folder, and editing code will be left out here. I am not sure if there is something like "core" really

Move sources to intern folder

Harbormaster completed remote builds in B10298: Diff 29037.Sep 18 2020, 3:19 PM

I would remove intermediate folders as well. Think this is also something Brecht was advising as well:

sergey@ws-sergey:~/Developer/blender/blender (arcpatch-D8492 (stash:4)[M])$ find ./source/blender/sequencer/
./source/blender/sequencer/
./source/blender/sequencer/intern
./source/blender/sequencer/intern/effects.c
./source/blender/sequencer/intern/image_cache.c
./source/blender/sequencer/intern/sequencer.c
./source/blender/sequencer/intern/modifier.c
./source/blender/sequencer/intern/prefetch.c
./source/blender/sequencer/BKE_sequencer.h
./source/blender/sequencer/CMakeLists.txt

Closed by commit rB18d7aeacf2f7: Move sequencer sources from blenkernel (authored by Richard Antalik (ISS)). · Explain WhyOct 5 2020, 3:19 AM

This revision was automatically updated to reflect the committed changes.

Richard Antalik (ISS) added a commit: rB18d7aeacf2f7: Move sequencer sources from blenkernel.