This commit implements the barebones of the new layer system.
- unittest
- DNA
- RNA
- doversioning
Not implemented:
- Full syncing system (of collection trees)
- Filter objects syncing
- Overrides
- User interface
- Getting rid of scene->base
- everything else
Details
Details
- Reviewers
Bastien Montagne (mont29) Julian Eisel (Severin) Sergey Sharybin (sergey) - Commits
- rB619cbca39f25: From review: "doxygen"
rBa3fd4274cf20: From review: curly brackets reinforcement
rBf59b6ff4108f: Using an iterator to go over objects, and use this for library_query
rB0da8957bc59b: From review: move nodetree syncing of layers to util function
rB57a5f2ef443d: Iterator util function
rB3f9432b76ac9: From review: use typedef for callbacks
rB75b7a25014ca: From review: do_versions_after_linking skipped on undo
rB59fbc6db8d4e: Merge remote-tracking branch 'origin/blender2.8' into render-layers
rB29961ad597ce: From review: blo_do_versions_280_after_linking >…
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- render-layers
- Build Status
Buildable 353 Build 353: arc lint + arc unit
Event Timeline
Comment Actions
Some preliminary review.
The most worrying part for me here is that you do some manual work to make sure layers/collections are not referenced from somewhere else on removal. Think now it should be part of relinking function.
But i'm also not sure how much strict we should consider this as a stopper. It's all WIP and be changed soon anyway so mainly it's important to raise possible issues IMO.
| source/blender/blenkernel/BKE_layer.h | ||
|---|---|---|
| 35 | What's this? Also, consider using anonymous enum instead of defines. | |
| source/blender/blenkernel/intern/context.c | ||
| 906 | Picky: Always use parenthesis, avoid noise and possible mistakes in the future. | |
| source/blender/blenkernel/intern/layer.c | ||
| 92 | Shouldn't this be using ID remap thingie from @Bastien Montagne (mont29)? | |
Comment Actions
(...) think now it should be part of relinking function.
Which relinking function?
But i'm also not sure how much strict we should consider this as a stopper. It's all WIP and be changed soon anyway so mainly it's important to raise possible issues IMO.
I think it's fair to assume users shouldn't open files with 2.8 and save them (unless they want to loose their work). So we can live with "out of sync" files for now in my opinion.
| source/blender/blenkernel/BKE_layer.h | ||
|---|---|---|
| 35 | I didn't finish the "syncronization" of the collection trees for all instances. So this is a way for me to track down the known TODOs. Example of uncovered cases:
That's without even getting started on the filter functionality (to add objects to a collection based on their names and a filter). | |
| source/blender/blenkernel/intern/context.c | ||
| 906 | By parenthesis you mean curly brackets? | |
| source/blender/blenkernel/intern/layer.c | ||
| 92 | This part is a copy+paste from BKE_scene_remove_render_layer. | |
Comment Actions
No. Relinking/remapping stuff is for IDs, and IDs only, not their sub-data. So unless you make layers or collections data-blocks, library_remap has nothing to do with their update work.
Besides details noted below, I see two main issues with current patch:
- Update of BKE's library_query.c is totally missing, it should now loop over objects of collections used by defined layers… And you most likely also need to update the pre/post process logic in BKE's library_remap too (things like libblock_remap_data_preprocess_scene_base_unlink etc.). This is absolutely mandatory before it can go into blender2.8 imho.
- Also, the do_versions_after_linking still has the issue raised by @Sergey Sharybin (sergey) the other day (can’t remember where): the only version available after Mains merge and lib linking is the one of the main file, meaning libraries may miss some (or potentially even worse, can see applied to their IDs older do_versions). Easy fix here would be to add .blend file version in Library data-blocks while expanding those, and then check id->lib->version (and only fall back to bmain version in case datablock has no lib), in do_versions_after_linking. And you also have to call that in link code, not only in main .blend reading code...
Point 2. is a bit of an orthogonal issue, should be addressed separately in own patch/commit, prior to applying this one, imho (am happy to work on it if you want).
| source/blender/blenkernel/intern/collection.c | ||
|---|---|---|
| 224 | Please typedef callback signature in header, and do avoid _'ed var names. | |
| source/blender/blenkernel/intern/context.c | ||
| 906 | yes, {/} | |
| source/blender/blenkernel/intern/layer.c | ||
| 92 | Eeeek! Hell no, this has nothing to do in ID remapping, this is not concerning IDs at all actually. But this has nothing to do here either imho, old code was a nice example of bad mixing it seems… This code should be a BKE_ utility defined in BKE_nodes, and called from here (e.g. BKE_nodetree_remove_layer_n(bNodeTree *ntree, const int layer_index) or something like that). | |
| 94–95 | Can define bNode *node directly in for loop | |
| source/blender/blenloader/intern/readfile.c | ||
| 8050 | Not so happy with the name, would keep version number last (so blo_do_versions_after_linking_280)… | |
| 8251–8256 | Should be skipped in undo cases as well (like with 'normal' do_versions call). | |
Comment Actions
- From review: use typedef for callbacks
- From review: blo_do_versions_280_after_linking > blo_do_versions_after_linking_280
- From review: do_versions_after_linking skipped on undo
- From review: move nodetree syncing of layers to util function
- From review: curly brackets reinforcement
Comment Actions
- Iterator util function
- Using an iterator to go over objects, and use this for library_query
@Bastien Montagne (mont29) I wonder if you have a more clever idea for the scenecollection iterator.
I considered adding a *parent to be able to wakl the tree in one go (we now loop over it 3x).
Comment Actions
I thought this over again, but I'd still suggest to replace the master collection with a master layer. It seems pretty inconsequent to allow having this special collection to stand alone. The layer is the trunk of each tree, except for the master collection, where the trunk is a collection. This would be a visible inconsistency for users: In 3D Views they could choose from any regular render layer, maybe the local layer and from the master collection. So they could choose from layers and the master collection instead of just layers.
And what if users want to render the master collection with control over AOVs, overrides, etc? They had to create a new render layer just for the master collection.
I'd also rename LayerCollection to Collection and SceneCollection to CollectionBase. The relation between those two is more important than the data level they belong to, which we might want to change in future even (causing hassles with renaming the DNA structs).
| source/blender/blenkernel/BKE_node.h | ||
|---|---|---|
| 697 | There is no opening bracket for this closing one. | |
| source/blender/blenkernel/intern/collection.c | ||
| 45–48 | Why not using Doxygen comments? /** * ... */ | |
| 66–69 | This does two iterations over all objects of the collection which may be a lot. You can avoid one iteration by calling BLI_remlink within the loop and BLI_assert(BLI_listbase_is_empty(&sc->objects)) after it. | |
Comment Actions
- Reworked logic of iterators
- Use FOREACH_OBJECT_FLAG in more places
That's a big change in the iterator logic. This was intended to make FOREACH_OBJECT_FLAG possible and preserve the current logic of object_relations.c flags.
We still have the problem of over-allocation of SceneCollection arrays (we loop over them 3x).
Comment Actions
I'm not sure I follow. The master collection would not be selectable as if it was a layer. The default setup would be to have one layer which contains the master collection, and that layer would be effectively a master layer. And then a user could decide to keep that one layer with the master collection, or to have two layers that contain the master collection (one for editing and one for rendering), or to have multiple layers each containing only smaller collections.
Now the relations are:
- Scenes contain layers and one master collection
- Layers contain collections
- Collections contain collections and objects
Without the concept of a master collection there would be more I think?
- Scenes contain master layer, other layers and collections
- Layers contain collections and objects
- Collections contain collections and objects
Comment Actions
I thought this over again, but I'd still suggest to replace the master collection with a master layer. It seems pretty inconsequent to allow having this special collection to stand alone. The layer is the trunk of each tree, except for the master collection, where the trunk is a collection. This would be a visible inconsistency for users: In 3D Views they could choose from any regular render layer, maybe the local layer and from the master collection. So they could choose from layers and the master collection instead of just layers.
I confess that I'm a bit confused with the point you are trying to make. Having a master collection actually helped the (internal) design in multiple ways. It's quite elegant. Anyways, just to make sure we are all on the same page, let's keep the current data diagram in mind:
One thing I'm tempted to change is instead of storing a ListBase of LayerCollection in the Layer, to store a ListBase of LinkNode of LayerCollection. But that's a separate thing.
And what if users want to render the master collection with control over AOVs, overrides, etc? They had to create a new render layer just for the master collection.
That's the whole point of having the master collection as the default collection linked in new layers.
I'd also rename LayerCollection to Collection and SceneCollection to CollectionBase. The relation between those two is more important than the data level they belong to, which we might want to change in future even (causing hassles with renaming the DNA structs).
I think you are mistaking LayerCollections and SceneCollections. Technically, the SceneCollection is the Collection, while the LayerCollection is the CollectionBase. Maybe this is why you are suggesting the master collection/master layer changes?
| source/blender/blenkernel/intern/collection.c | ||
|---|---|---|
| 66–69 | I don't think this is performance sensitive. That said, if we are to have a BLI_freelistN_callback I would be the first one to use it! I feel that LinkData is neglected in the API. | |
Comment Actions
Big update on syncing the missing bits are:
- An object is added
- An object is removed
- An object is copied
And all syncing related to filter objects (which is fine since this is not implemented)
Individual commits:
- Make hide/hide_select icons dynamic
- Temporary UI for layer/collections (for debug/testing)
- Add nesting collections into unittest
- Syncing: update layercollection tree when a new scenecollection is added
- Syncing: an object added to a scenecollection
- Layers: unittest to cover unlinking of objects
- unittest: make tests more granular
- Syncing: update layercollection tree when an object is unlinked
- unittest: syncing when unlinking a collection
- Debug UI revamp
- Fixup for rna_SceneCollection_remove
- unittest: new layer, layer_collection_link/unlink
- render_layer.collections.link/unlink()
- unittest touch ups
Comment Actions
- Make sure new objects go to the correct (active) collection
(and incorporate @Bastien Montagne (mont29) patch for after_linking do version)
Comment Actions
Besides changes in library_query needing rework (see detailed comment below), looks like you still did not check library_remap.c file? Again, preprocess/postprocess functions there most certainly need some attention, since they are currently messing with bases, I’d expect them to have to also mess with collections?
| source/blender/blenkernel/intern/library_query.c | ||
|---|---|---|
| 389–397 | This is not going to work at all. If you check CALLBACK_INVOKE and further macros down the path, up till callback definition itself (LibraryIDLinkCallback), you'll notice we are passing a pointer to pointer here (ID **id_pointer), so you absolutely cannot use a temp local pointer for this, and you absolutely cannot use FOREACH_SCENE_OBJECT either. What you need to do instead is looping over *all* collections in the scene and pass their object pointers to the callback. And if a same collection can be used as sub-collection of several others, (not sure design allows that or not?), then you'll have to add a way (a flag probably) to prevent a same collection to be handled more than once. The whole point of this libquery BKE_library_foreach_ID_link function is to be able to edit the pointers themselves, not only the data-blocks they point to. | |
Comment Actions
objects removal fully working (with remap)
I feel it's a bit of overkill to have it handled in pre and post, but at least it's working now.
I will try to refine it later.
Comment Actions
@Bastien Montagne (mont29) how do you think we should treat ob.select? Options:
- expose bContext for get/set functions, and get the corresponding ObjectBase based on context (context gives us the SceneLayer which gives as the ObjectBase for the object).
- replace ob.select with ob.select(action={'TOGGLE', 'SELECT', 'DESELECT'}) (that uses bContext
- drop ob.select entirely and find an alternative (exposing ObjectBase to user?)
My preference is (1), but (2) is not bad either.
Comment Actions
- Unittest: sorted updates
- RNA: bpy.context.scene_collection
- Adding FOREACH_OBJECT iterator
- rename ob_base > base, it will simplify further refactors
- Add new objects and some Base > ObjectBase convertion