Page MenuHome

Move sequencer sources from blenkernel
ClosedPublic

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

Details

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

Event Timeline

Richard Antalik (ISS) requested review of this revision.Aug 7 2020, 7:58 AM
Richard Antalik (ISS) created this revision.
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.

Richard Antalik (ISS) marked 4 inline comments as done.
  • Remove subfolders from include paths
  • Move cache to own folder and remove file prefixes

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

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
This revision was automatically updated to reflect the committed changes.