[Bf-blender-cvs] [9c48b41cfc6] greasepencil-object: Renamings and more code documentation
Falk David
noreply at git.blender.org
Mon Feb 7 18:35:15 CET 2022
Commit: 9c48b41cfc6d9f4dfa4cfb2dba25e8db94f863ee
Author: Falk David
Date: Fri Feb 4 12:26:01 2022 +0100
Branches: greasepencil-object
https://developer.blender.org/rB9c48b41cfc6d9f4dfa4cfb2dba25e8db94f863ee
Renamings and more code documentation
Co-authored-by: @yann-lty
===================================================================
M source/blender/blenkernel/intern/gpencil.c
M source/blender/blenkernel/intern/gpencil_update_cache.c
M source/blender/editors/gpencil/gpencil_undo.c
===================================================================
diff --git a/source/blender/blenkernel/intern/gpencil.c b/source/blender/blenkernel/intern/gpencil.c
index 75a2eededfc..6dd1da9dca7 100644
--- a/source/blender/blenkernel/intern/gpencil.c
+++ b/source/blender/blenkernel/intern/gpencil.c
@@ -3000,12 +3000,18 @@ void BKE_gpencil_update_on_write(bGPdata *gpd_orig, bGPdata *gpd_eval)
};
BKE_gpencil_traverse_update_cache(update_cache, &ts, &data);
-
gpd_eval->flag |= GP_DATA_CACHE_IS_DIRTY;
- /* TODO: This might cause issues when we have multiple depsgraphs? */
- if ((gpd_orig->flag & GP_DATA_UPDATE_CACHE_DISPOSABLE) || !GPENCIL_ANY_MODE(gpd_orig) ||
- !U.experimental.use_gpencil_undo_system) {
+ const bool gpencil_undo_system_inactive = !(U.experimental.use_gpencil_undo_system &&
+ GPENCIL_ANY_MODE(gpd_orig));
+ /* If the gpencil undo system is active, make sure to only free the cache if
+ * GP_DATA_UPDATE_CACHE_DISPOSABLE is set. Even though we already used the cache to update the
+ * eval object, it might still be needed for the undo system (e.g if a modal operator is running,
+ * it might call the update-on-write multiple times before an undo step is encoded). Only when
+ * the undo system marks the cache as disposable can we safely free it here.*/
+ if (gpencil_undo_system_inactive || (gpd_orig->flag & GP_DATA_UPDATE_CACHE_DISPOSABLE)) {
+ /* TODO: This might cause issues when we have multiple depsgraphs? Because the cache might be
+ * accessed later/concurrently even if it was freed here? */
BKE_gpencil_free_update_cache(gpd_orig);
}
}
diff --git a/source/blender/blenkernel/intern/gpencil_update_cache.c b/source/blender/blenkernel/intern/gpencil_update_cache.c
index fd183cf4274..480f900ec81 100644
--- a/source/blender/blenkernel/intern/gpencil_update_cache.c
+++ b/source/blender/blenkernel/intern/gpencil_update_cache.c
@@ -88,23 +88,40 @@ static void cache_node_update(void *node, void *data)
GPencilUpdateCache *update_cache = ((GPencilUpdateCacheNode *)node)->cache;
GPencilUpdateCache *new_update_cache = (GPencilUpdateCache *)data;
- /* If the new cache is already "covered" by the current cache, just free it and return. */
- if (new_update_cache->flag < update_cache->flag) {
+ /* IMPORTANT: Because we are comparing the values of the flags here, make sure that any potential
+ * new flag either respects this ordering or changes the following logic. */
+ const bool current_cache_covers_new_cache = new_update_cache->flag < update_cache->flag;
+
+ /* In case:
+ * - the new cache is a no copy
+ * - or the new cache is a light copy and the current cache a full copy
+ * then it means we are already caching "more" and we shouldn't update the current cache.
+ * So we free the structure and return early.
+ */
+ if (current_cache_covers_new_cache) {
update_cache_free(new_update_cache);
return;
}
+ /* In case:
+ * - the cache types are equal
+ * - or the new cache contains more than the current cache (full copy > light copy > no copy)
+ * the data pointer is updated. If the cache types are equal, this might be a no-op (when the new
+ * data pointer is equal to the previous), but is necessary when the data pointer needs to
+ * change. This can for example happen when the underlying data was reallocated, but the cache
+ * type stayed the same.
+ */
update_cache->data = new_update_cache->data;
update_cache->flag = new_update_cache->flag;
/* In case the new cache does a full update, remove its children since they will be all
- * updated by this cache. */
+ * updated by the new cache. */
if (new_update_cache->flag == GP_UPDATE_NODE_FULL_COPY && update_cache->children != NULL) {
/* We don't free the tree itself here, because we just want to clear the children, not delete
* the whole node. */
BLI_dlrbTree_free(update_cache->children, cache_node_free);
}
-
+ /* Once we updated the data pointer and the flag, we can safely free the new cache structure. */
update_cache_free(new_update_cache);
}
diff --git a/source/blender/editors/gpencil/gpencil_undo.c b/source/blender/editors/gpencil/gpencil_undo.c
index d268535b391..55e7e860a03 100644
--- a/source/blender/editors/gpencil/gpencil_undo.c
+++ b/source/blender/editors/gpencil/gpencil_undo.c
@@ -198,10 +198,13 @@ void gpencil_undo_finish(void)
* \{ */
typedef struct GPencilUndoData {
- GPencilUpdateCache *gpd_cache_data;
- /* Scene frame for this step. */
+ /* This is the cache that indicates the differential changes made coming into (one-directional)
+ * this step. The data pointers in the cache are owned by the undo step (i.e. they will be
+ * allocated and freed within the undo system). */
+ GPencilUpdateCache *gpd_cache;
+ /* Scene frame number in this step. */
int cfra;
- /* Store the grease pencil mode we are in. */
+ /* The grease pencil mode we are in this step. */
eObjectMode mode;
} GPencilUndoData;
@@ -210,8 +213,9 @@ typedef struct GPencilUndoStep {
GPencilUndoData *undo_data;
} GPencilUndoStep;
-static bool change_gpencil_mode(bContext *C, Object *ob, eObjectMode mode)
+static bool change_gpencil_mode_if_needed(bContext *C, Object *ob, eObjectMode mode)
{
+ /* No mode change needed if they are the same. */
if (ob->mode == mode) {
return false;
}
@@ -222,7 +226,7 @@ static bool change_gpencil_mode(bContext *C, Object *ob, eObjectMode mode)
return true;
}
-static void gpencil_data_to_undo_data(bGPdata *gpd, GPencilUndoData *gpd_undo_data)
+static void encode_gpencil_data_to_undo_data(bGPdata *gpd, GPencilUndoData *gpd_undo_data)
{
GPencilUpdateCache *update_cache = gpd->runtime.update_cache;
@@ -232,10 +236,10 @@ static void gpencil_data_to_undo_data(bGPdata *gpd, GPencilUndoData *gpd_undo_da
BKE_gpencil_data_duplicate(NULL, gpd, &gpd_copy);
gpd_copy->id.session_uuid = gpd->id.session_uuid;
- gpd_undo_data->gpd_cache_data = BKE_gpencil_create_update_cache(gpd_copy, true);
+ gpd_undo_data->gpd_cache = BKE_gpencil_create_update_cache(gpd_copy, true);
}
else {
- gpd_undo_data->gpd_cache_data = BKE_gpencil_duplicate_update_cache_and_data(update_cache);
+ gpd_undo_data->gpd_cache = BKE_gpencil_duplicate_update_cache_and_data(update_cache);
}
}
@@ -353,11 +357,11 @@ static bool gpencil_decode_undo_data_stroke_cb(GPencilUpdateCache *gps_cache, vo
return false;
}
-static bool gpencil_undo_data_to_gpencil_data(GPencilUndoData *gpd_undo_data,
- bGPdata *gpd,
- bool tag_gpd_update_cache)
+static bool decode_undo_data_to_gpencil_data(GPencilUndoData *gpd_undo_data,
+ bGPdata *gpd,
+ bool tag_gpd_update_cache)
{
- GPencilUpdateCache *update_cache = gpd_undo_data->gpd_cache_data;
+ GPencilUpdateCache *update_cache = gpd_undo_data->gpd_cache;
BLI_assert(update_cache != NULL);
@@ -450,8 +454,7 @@ static bool gpencil_undosys_step_encode(struct bContext *C,
}
}
- /* TODO: Handle this case properly once the update cache is more widly used. We avoid full-copies
- * for now at the expense of to being able to undo them. */
+ /* TODO: Figure out if doing full-copies and using a lot of memory can be solved in some way. */
#if 0
if (!only_frame_changed && gpd->runtime.update_cache == NULL) {
return false;
@@ -467,7 +470,10 @@ static bool gpencil_undosys_step_encode(struct bContext *C,
return true;
}
- gpencil_data_to_undo_data(gpd, us->undo_data);
+ /* Encode the differential changes made to the gpencil data coming into this step. */
+ encode_gpencil_data_to_undo_data(gpd, us->undo_data);
+ /* Because the encoding of a gpencil undo step uses the update cache on the gpencil data, we can
+ * tag it after the encode so that the update-on-write knows that it can be safely disposed. */
gpd->flag |= GP_DATA_UPDATE_CACHE_DISPOSABLE;
return true;
}
@@ -488,65 +494,91 @@ static void gpencil_undosys_step_decode(struct bContext *C,
return;
}
+ /* The decode step of the undo should be the last time we write to the gpd update cache, so tag
+ * it as disposable here and the update-on-write will be able to free it. */
+ if (is_final) {
+ gpd->flag |= GP_DATA_UPDATE_CACHE_DISPOSABLE;
+ }
+
Scene *scene = CTX_data_scene(C);
if (undo_data->cfra != scene->r.cfra) {
scene->r.cfra = undo_data->cfra;
- /* TODO: what if we merged a full copy with a frame change? */
- DEG_id_tag_update(&scene->id, ID_RECALC_AUDIO_MUTE);
- WM_event_add_notifier(C, NC_SCENE | ND_FRAME, NULL);
- }
-
- if (change_gpencil_mode(C, ob, undo_data->mode)) {
if (is_final) {
- gpd->flag |= GP_DATA_UPDATE_CACHE_DISPOSABLE;
- DEG_id_tag_update(&gpd->id, ID_RECALC_TRANSFORM | ID_RECALC_GEOMETRY);
- WM_event_add_notifier(C, NC_GPENCIL | ND_DATA | ND_GPENCIL_EDITMODE, NULL);
- WM_event_add_notifier(C, NC_SCENE | ND_MODE, NULL);
+ DEG_id_tag_update(&scene->id, ID_RECALC_AUDIO_MUTE);
+ WM_event_add_notifier(C, NC_SCENE | ND_FRAME, NULL);
}
- return;
+ }
+
+ /* Check if a mode change needs to happen (by comparing the saved mode flags on the undo step
+ * data) and switch to that mode. */
+ const bool mode_changed = change_gpencil_mode_if_needed(C, ob, undo_data->mode);
+
+ /* If the mode was updated, make sure to tag the ID and add notifiers. */
+ if (mode_changed && is_final) {
+ DEG_id_tag_update(&gpd->id, ID_RECALC_TRANSFORM | ID_RECALC_GEOMETRY);
+ WM_event_add_notifier(C, NC_GPENCIL | ND_DATA | ND_GPENCIL_EDITMODE, NULL);
+ WM_event_add_notifier(C, NC_SCENE | ND_MODE, NULL);
}
if (dir == STEP_UNDO) {
UndoStep *us_iter = us_p;
+ /* Assume that a next steps always exists in case that we undo a step. */
BLI_assert(us_iter->next != NULL);
UndoStep *us_next = us_p->next;
+ /* If we come from a step that was outside the gpencil undo system, assume that this was a mode
+ * change from object mode and that we don't need to decode anything. */
+ if (us_next->type != BKE_UNDOSYS_TYPE_GPENCIL) {
+
@@ Diff output truncated at 10240 characters. @@
More information about the Bf-blender-cvs
mailing list