Changeset View
Standalone View
source/blender/blenkernel/intern/collection.c
| Show First 20 Lines • Show All 1,537 Lines • ▼ Show 20 Lines | |||||
| } | } | ||||
| /** \} */ | /** \} */ | ||||
| /* -------------------------------------------------------------------- */ | /* -------------------------------------------------------------------- */ | ||||
| /** \name Collection move (outliner drag & drop) | /** \name Collection move (outliner drag & drop) | ||||
| * \{ */ | * \{ */ | ||||
| typedef struct LayerCollectionFlag { | |||||
| struct LayerCollectionFlag *next, *prev; | |||||
| int flag; | |||||
mont29: I would also store the `Collection *` pointer itself here, that way you can `BLI_assert` it… | |||||
mont29Unsubmitted Done Inline ActionsAlso, I would add here the ViewLayer * pointer (see other comments about the why). mont29: Also, I would add here the `ViewLayer *` pointer (see other comments about the why). | |||||
| ListBase children; | |||||
| } LayerCollectionFlag; | |||||
| static void store_collection_flag_recursive(LayerCollection *layer_collection, | |||||
mont29Unsubmitted Done Inline Actionslayer_collection_flags_store_recursive mont29: `layer_collection_flags_store_recursive` | |||||
| LayerCollectionFlag *flag) | |||||
| { | |||||
| flag->flag = layer_collection->flag; | |||||
mont29Unsubmitted Done Inline ActionsAlso store here the Collection pointer from the layer_collection into the flag. mont29: Also store here the `Collection` pointer from the `layer_collection` into the `flag`. | |||||
| LISTBASE_FOREACH (LayerCollection *, child, &layer_collection->layer_collections) { | |||||
| LayerCollectionFlag *child_flag = MEM_callocN(sizeof(LayerCollectionFlag), __func__); | |||||
| BLI_addtail(&flag->children, child_flag); | |||||
| store_collection_flag_recursive(child, child_flag); | |||||
| } | |||||
| } | |||||
| /** | |||||
| * For every view layer, find the collection and save flags for its children, which will also be | |||||
| * moved. Store a temporary tree structure for the collection in each view layer since we want to | |||||
| * restore their flags as well. | |||||
| */ | |||||
| static void layer_collection_flags_store(Main *bmain, | |||||
| GHash *view_layer_hash, | |||||
| Collection *collection) | |||||
| { | |||||
| LISTBASE_FOREACH (Scene *, scene, &bmain->scenes) { | |||||
| LISTBASE_FOREACH (ViewLayer *, view_layer, &scene->view_layers) { | |||||
| /* Find the collection in the view layer. */ | |||||
| LayerCollection *layer_collection = BKE_layer_collection_first_from_scene_collection( | |||||
Done Inline ActionsI'd rather have the listbase created in calling code and passed around as a pointer… mont29: I'd rather have the listbase created in calling code and passed around as a pointer… | |||||
| view_layer, collection); | |||||
| if (layer_collection == NULL) { | |||||
| continue; | |||||
| } | |||||
| /* Store the flags for the collection and all of its children. */ | |||||
| LayerCollectionFlag *collection_flag = MEM_callocN(sizeof(LayerCollectionFlag), __func__); | |||||
| /* Recursively save flags from collection children. */ | |||||
| store_collection_flag_recursive(layer_collection, collection_flag); | |||||
| BLI_ghash_insert(view_layer_hash, view_layer, collection_flag); | |||||
mont29Unsubmitted Done Inline ActionsReplace this with inserting into a listbase of 'first level' LayerCollectionFlag items, that also store the ViewLayer pointer. mont29: Replace this with inserting into a listbase of 'first level' `LayerCollectionFlag` items, that… | |||||
| } | |||||
| } | |||||
| } | |||||
| static void restore_collection_flag_recursive(LayerCollection *layer_collection, | |||||
mont29Unsubmitted Done Inline Actionslayer_collection_flags_restore_recursive mont29: `layer_collection_flags_restore_recursive` | |||||
| LayerCollectionFlag *flag) | |||||
| { | |||||
| LayerCollectionFlag *child_flag = flag->children.first; | |||||
mont29Unsubmitted Done Inline ActionsHere you can assert that layer_collection->collection == flag->collection. mont29: Here you can assert that `layer_collection->collection == flag->collection`. | |||||
| LISTBASE_FOREACH (LayerCollection *, child, &layer_collection->layer_collections) { | |||||
| /* Store the next item, as the recursive call will free the current one. */ | |||||
| LayerCollectionFlag *child_flag_next = child_flag->next; | |||||
| restore_collection_flag_recursive(child, child_flag); | |||||
mont29Unsubmitted Done Inline ActionsFree the listbase item properly: BLI_freelinkN(&flag->children, child_flag); mont29: Free the listbase item properly: `BLI_freelinkN(&flag->children, child_flag);` | |||||
HooglyBooglyAuthorUnsubmitted Done Inline ActionsI thought it made a bit more sense to do it all in one call at the end of the loop, that way it doesn't need to be a mutable loop. I can still do it this way if you like, I think the way it is now is a bit cleaner though. HooglyBoogly: I thought it made a bit more sense to do it all in one call at the end of the loop, that way it… | |||||
mont29Unsubmitted Done Inline ActionsThe point of doing it item by item here is that you can then assert (once all children LayerCollection have been processed in upper level) that you also have processed all of the stored LayerCollectionFlag. This gives some basic check against potential discrepancy between the two lists… If you prefer, you can also assert that they have the same length (BLI_listbase_count()) before starting iterating on them, might actually be clearer (and we do not really care about some extra looping on those, especially if only for debug builds). mont29: The point of doing it item by item here is that you can then assert (once all children… | |||||
HooglyBooglyAuthorUnsubmitted Done Inline ActionsOkay, yes, that seems clearer to me, I'll add that assert. HooglyBoogly: Okay, yes, that seems clearer to me, I'll add that assert. | |||||
| child_flag = child_flag_next; | |||||
| } | |||||
mont29Unsubmitted Done Inline ActionsNow here you can assert that BLI_listbase_is_empty(&flag->children). mont29: Now here you can assert that `BLI_listbase_is_empty(&flag->children)`. | |||||
| /* We treat exclude as a special case. | |||||
| * | |||||
| * If in a different view layer the parent collection was disabled (e.g., background) | |||||
| * and now we moved a new collection to be part of the background this collection should | |||||
| * probably be disabled. | |||||
| * | |||||
| * Note: If we were to also keep the exclude flag we would need to re-sync the collections. | |||||
| */ | |||||
| layer_collection->flag = flag->flag | (layer_collection->flag & LAYER_COLLECTION_EXCLUDE); | |||||
| MEM_freeN(flag); | |||||
mont29Unsubmitted Done Inline ActionsThis is not needed anymore. mont29: This is not needed anymore. | |||||
Done Inline ActionsWould move that assert at beginning of the function, with the other one. mont29: Would move that assert at beginning of the function, with the other one. | |||||
| } | |||||
| /** | |||||
| * Restore a collection's (and its children's) flags for each view layer from the structure built | |||||
| * in #layer_collection_flags_store. | |||||
| */ | |||||
| static void layer_collection_flags_restore(GHash *view_layer_hash, Collection *collection) | |||||
| { | |||||
| GHashIterator gh_iter; | |||||
| GHASH_ITER (gh_iter, view_layer_hash) { | |||||
mont29Unsubmitted Done Inline ActionsTo be replaced by LISTBASE_FOREACH_MUTABLE over the top ListBase of LayerCollectionFlag, that now also store their ViewLayer pointer. mont29: To be replaced by `LISTBASE_FOREACH_MUTABLE` over the top `ListBase` of `LayerCollectionFlag`… | |||||
| /* Find the collection in the view layer. */ | |||||
| ViewLayer *view_layer = BLI_ghashIterator_getKey(&gh_iter); | |||||
| LayerCollection *layer_collection = BKE_layer_collection_first_from_scene_collection( | |||||
| view_layer, collection); | |||||
| if (layer_collection == NULL) { | |||||
| continue; | |||||
| } | |||||
| LayerCollectionFlag *flag = BLI_ghashIterator_getValue(&gh_iter); | |||||
| restore_collection_flag_recursive(layer_collection, flag); | |||||
Done Inline ActionsDoesn't need to be mutable? You are not removing any item from that collection, but frees it all at once at the end after looping on it. mont29: Doesn't need to be mutable? You are not removing any item from that collection, but frees it… | |||||
Done Inline ActionsOops, no it does not! HooglyBoogly: Oops, no it does not! | |||||
| } | |||||
| } | |||||
| bool BKE_collection_move(Main *bmain, | bool BKE_collection_move(Main *bmain, | ||||
Done Inline Actionsisn't mont29: `isn't` | |||||
| Collection *to_parent, | Collection *to_parent, | ||||
| Collection *from_parent, | Collection *from_parent, | ||||
| Collection *relative, | Collection *relative, | ||||
| bool relative_after, | bool relative_after, | ||||
Done Inline ActionsYou assert'ed on that, no need to check it anymore. mont29: You assert'ed on that, no need to check it anymore. | |||||
| Collection *collection) | Collection *collection) | ||||
| { | { | ||||
| if (collection->flag & COLLECTION_IS_MASTER) { | if (collection->flag & COLLECTION_IS_MASTER) { | ||||
| return false; | return false; | ||||
| } | } | ||||
| if (BKE_collection_cycle_find(to_parent, collection)) { | if (BKE_collection_cycle_find(to_parent, collection)) { | ||||
| return false; | return false; | ||||
| } | } | ||||
| Show All 21 Lines | if (relative_child) { | ||||
| } | } | ||||
| BKE_collection_object_cache_free(to_parent); | BKE_collection_object_cache_free(to_parent); | ||||
| } | } | ||||
| } | } | ||||
| /* Make sure we store the flag of the layer collections before we remove and re-create them. | /* Make sure we store the flag of the layer collections before we remove and re-create them. | ||||
| * Otherwise they will get lost and everything will be copied from the new parent collection. */ | * Otherwise they will get lost and everything will be copied from the new parent collection. */ | ||||
| GHash *view_layer_hash = BLI_ghash_new(BLI_ghashutil_ptrhash, BLI_ghashutil_ptrcmp, __func__); | GHash *view_layer_hash = BLI_ghash_new(BLI_ghashutil_ptrhash, BLI_ghashutil_ptrcmp, __func__); | ||||
mont29Unsubmitted Done Inline ActionsReplace this by a ListBase of top-level LayerCollectionFlag storing a pointer to matching Viewlayer. mont29: Replace this by a `ListBase` of top-level `LayerCollectionFlag` storing a pointer to matching… | |||||
| layer_collection_flags_store(bmain, view_layer_hash, collection); | |||||
| for (Scene *scene = bmain->scenes.first; scene; scene = scene->id.next) { | |||||
| LISTBASE_FOREACH (ViewLayer *, view_layer, &scene->view_layers) { | |||||
| LayerCollection *layer_collection = BKE_layer_collection_first_from_scene_collection( | |||||
| view_layer, collection); | |||||
| if (layer_collection == NULL) { | |||||
| continue; | |||||
| } | |||||
| BLI_ghash_insert(view_layer_hash, view_layer, POINTER_FROM_INT(layer_collection->flag)); | |||||
| } | |||||
| } | |||||
| /* Create and remove layer collections. */ | /* Create and remove layer collections. */ | ||||
| BKE_main_collection_sync(bmain); | BKE_main_collection_sync(bmain); | ||||
| /* Restore back the original layer collection flags. */ | /* Restore back the original layer collection flags. */ | ||||
| GHashIterator gh_iter; | layer_collection_flags_restore(view_layer_hash, collection); | ||||
| GHASH_ITER (gh_iter, view_layer_hash) { | |||||
| ViewLayer *view_layer = BLI_ghashIterator_getKey(&gh_iter); | |||||
| LayerCollection *layer_collection = BKE_layer_collection_first_from_scene_collection( | |||||
| view_layer, collection); | |||||
| if (layer_collection) { | |||||
| /* We treat exclude as a special case. | |||||
| * | |||||
| * If in a different view layer the parent collection was disabled (e.g., background) | |||||
| * and now we moved a new collection to be part of the background this collection should | |||||
| * probably be disabled. | |||||
| * | |||||
| * Note: If we were to also keep the exclude flag we would need to re-sync the collections. | |||||
| */ | |||||
| layer_collection->flag = POINTER_AS_INT(BLI_ghashIterator_getValue(&gh_iter)) | | |||||
| (layer_collection->flag & LAYER_COLLECTION_EXCLUDE); | |||||
| } | |||||
| } | |||||
| BLI_ghash_free(view_layer_hash, NULL, NULL); | BLI_ghash_free(view_layer_hash, NULL, NULL); | ||||
mont29Unsubmitted Done Inline ActionsTo be replaced by BLI_freelistN call. mont29: To be replaced by `BLI_freelistN` call. | |||||
| /* We need to sync it again to pass the correct flags to the collections objects. */ | /* We need to sync it again to pass the correct flags to the collections objects. */ | ||||
Done Inline Actionsshould go at the end of layer_collection_flags_restore mont29: should go at the end of `layer_collection_flags_restore` | |||||
Done Inline ActionsI don't think it can though, it still needs to be in the recursive function so that children of children would be freed. HooglyBoogly: I don't think it can though, it still needs to be in the recursive function so that children of… | |||||
Done Inline ActionsThose have already been freed inf the _restore_recursive function… Iḿ talking about that call freeing the main 'viewlayers´ list. mont29: Those have already been freed inf the `_restore_recursive` function… Iḿ talking about that call… | |||||
Done Inline ActionsOh sorry, I lost my place and didn't realize which part of this comment applied to. HooglyBoogly: Oh sorry, I lost my place and didn't realize which part of this comment applied to. | |||||
| BKE_main_collection_sync(bmain); | BKE_main_collection_sync(bmain); | ||||
| return true; | return true; | ||||
| } | } | ||||
| /** \} */ | /** \} */ | ||||
| /* -------------------------------------------------------------------- */ | /* -------------------------------------------------------------------- */ | ||||
| ▲ Show 20 Lines • Show All 189 Lines • Show Last 20 Lines | |||||
I would also store the Collection * pointer itself here, that way you can BLI_assert it matches the layer_collection->collection one in restore step.