From 7a06588f059e2ece64d838b71664651b18c46346 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= <sybren@blender.org>
Date: Mon, 22 May 2023 10:36:50 +0200
Subject: [PATCH] Fix #102662: NLA-Strip Corrupted after reopening file w/ lib
override
Mark `NlaStrip.frame_{start,end}` and `NlaStrip.frame_{start,end}_ui` as
to-be-ignored for the library override system, and add a new set of RNA
properties `frame_{start,end}_raw` that the library override system can
use.
Versioning code ensures that overrides on `frame_{start,end}` are
altered to be applied to the `..._raw` counterpart instead.
The override system uses RNA to update properties one-by-one, and the
RNA code trying its best to keep things consistent / valid. This is very
much desired behaviour while a human is editing the data.
However, when the library override system is doing this, it is not
replaying the individual steps (that each end in a valid configuration),
but just setting each property one by one. As a result, the intermediate
state can be invalid (for example moving one strip into another) even
when the end result is perfectly fine.
This is what the `..._raw` properties do -- they set the values without
doing any validation, so they allow the library overrides system to move
strips around.
This assumes that the result of the override is still valid. Logic to
detect invalid situations, and reshuffle the NLA strips if necessary, is
left for a future commit as it is related to #107990 (NLA Vertical
Reorder).
Additionally, this commit adds functions
`BKE_lib_override_library_property_rna_path_change()` and
`BKE_lib_override_library_property_search_and_delete()` to the library
override API. The former is used to change RNA paths of property
overrides, and the latter is used to remove a property override
identified by its RNA path.
---
source/blender/blenkernel/BKE_lib_override.h | 21 ++++++
.../blender/blenkernel/intern/lib_override.cc | 35 ++++++++++
.../blenloader/intern/versioning_300.cc | 69 +++++++++++++++++++
source/blender/makesrna/intern/rna_nla.c | 33 +++++++++
4 files changed, 158 insertions(+)
diff --git a/source/blender/blenkernel/BKE_lib_override.h b/source/blender/blenkernel/BKE_lib_override.h
index 990517f128e..70b717004c0 100644
--- a/source/blender/blenkernel/BKE_lib_override.h
+++ b/source/blender/blenkernel/BKE_lib_override.h
@@ -303,6 +303,27 @@ struct IDOverrideLibraryProperty *BKE_lib_override_library_property_get(
*/
void BKE_lib_override_library_property_delete(struct IDOverrideLibrary *override,
struct IDOverrideLibraryProperty *override_property);
+/**
+ * Delete a property override from the given ID \a override, if it exists.
+ *
+ * \return True when the property was found (and thus deleted), false if it wasn't found.
+ */
+bool BKE_lib_override_library_property_search_and_delete(struct IDOverrideLibrary *override,
+ const char *rna_path);
+
+/** Change the RNA path of a library override on a property.
+ *
+ * No-op if the property override cannot be found.
+ *
+ * \param from_rna_path The RNA path of the property to change.
+ * \param to_rna_path The new RNA path. The library override system will copy the string to its own
+ * memory; the caller will retain ownership of the passed pointer.
+ * \return True if the property was found (and thus changed), false if it wasn't found.
+ */
+bool BKE_lib_override_library_property_rna_path_change(IDOverrideLibrary *liboverride,
+ const char *old_rna_path,
+ const char *new_rna_path);
+
/**
* Get the RNA-property matching the \a library_prop override property. Used for UI to query
* additional data about the overridden property (e.g. UI name).
diff --git a/source/blender/blenkernel/intern/lib_override.cc b/source/blender/blenkernel/intern/lib_override.cc
index 69933617f5f..859c7b44106 100644
--- a/source/blender/blenkernel/intern/lib_override.cc
+++ b/source/blender/blenkernel/intern/lib_override.cc
@@ -3530,6 +3530,41 @@ void lib_override_library_property_clear(IDOverrideLibraryProperty *op)
BLI_freelistN(&op->operations);
}
+bool BKE_lib_override_library_property_search_and_delete(IDOverrideLibrary *override,
+ const char *rna_path)
+{
+ IDOverrideLibraryProperty *override_property = BKE_lib_override_library_property_find(override,
+ rna_path);
+ if (override_property == nullptr) {
+ return false;
+ }
+ BKE_lib_override_library_property_delete(override, override_property);
+ return true;
+}
+
+bool BKE_lib_override_library_property_rna_path_change(IDOverrideLibrary *override,
+ const char *old_rna_path,
+ const char *new_rna_path)
+{
+ /* Find the override property by its old RNA path. */
+ GHash *override_runtime = override_library_rna_path_mapping_ensure(override);
+ IDOverrideLibraryProperty *override_property = static_cast<IDOverrideLibraryProperty *>(
+ BLI_ghash_popkey(override_runtime, old_rna_path, nullptr));
+
+ if (override_property == nullptr) {
+ return false;
+ }
+
+ /* Switch over the RNA path. */
+ MEM_SAFE_FREE(override_property->rna_path);
+ override_property->rna_path = BLI_strdup(new_rna_path);
+
+ /* Put property back into the lookup mapping, using the new RNA path. */
+ BLI_ghash_insert(override_runtime, override_property->rna_path, override_property);
+
+ return true;
+}
+
void BKE_lib_override_library_property_delete(IDOverrideLibrary *override,
IDOverrideLibraryProperty *override_property)
{
diff --git a/source/blender/blenloader/intern/versioning_300.cc b/source/blender/blenloader/intern/versioning_300.cc
index 841ba4a5339..0ebf1ed9a06 100644
--- a/source/blender/blenloader/intern/versioning_300.cc
+++ b/source/blender/blenloader/intern/versioning_300.cc
@@ -2321,6 +2321,73 @@ static void version_ensure_missing_regions(ScrArea *area, SpaceLink *sl)
}
}
+/**
+ * Change override RNA path from `frame_{start,end}` to `frame_{start,end}_raw`.
+ * See #102662.
+ */
+static void version_liboverride_nla_strip_frame_start_end(IDOverrideLibrary *liboverride,
+ const char *parent_rna_path,
+ NlaStrip *strip)
+{
+ if (!strip) {
+ return;
+ }
+
+ /* Escape the strip name for inclusion in the RNA path. */
+ char name_esc_strip[sizeof(strip->name) * 2];
+ BLI_str_escape(name_esc_strip, strip->name, sizeof(name_esc_strip));
+
+ const std::string rna_path_strip = std::string(parent_rna_path) + ".strips[\"" + name_esc_strip +
+ "\"]";
+
+ { /* Rename .frame_start -> .frame_start_raw: */
+ const std::string rna_path_prop = rna_path_strip + ".frame_start";
+ BKE_lib_override_library_property_rna_path_change(
+ liboverride, rna_path_prop.c_str(), (rna_path_prop + "_raw").c_str());
+ }
+
+ { /* Rename .frame_end -> .frame_end_raw: */
+ const std::string rna_path_prop = rna_path_strip + ".frame_end";
+ BKE_lib_override_library_property_rna_path_change(
+ liboverride, rna_path_prop.c_str(), (rna_path_prop + "_raw").c_str());
+ }
+
+ { /* Remove .frame_start_ui: */
+ const std::string rna_path_prop = rna_path_strip + ".frame_start_ui";
+ BKE_lib_override_library_property_search_and_delete(liboverride, rna_path_prop.c_str());
+ }
+
+ { /* Remove .frame_end_ui: */
+ const std::string rna_path_prop = rna_path_strip + ".frame_end_ui";
+ BKE_lib_override_library_property_search_and_delete(liboverride, rna_path_prop.c_str());
+ }
+
+ /* Handle meta-strip contents. */
+ LISTBASE_FOREACH (NlaStrip *, substrip, &strip->strips) {
+ version_liboverride_nla_strip_frame_start_end(liboverride, rna_path_strip.c_str(), substrip);
+ }
+}
+
+/** Fix the `frame_start` and `frame_end` overrides on NLA strips. See #102662. */
+static void version_liboverride_nla_frame_start_end(ID *id, AnimData *adt, void * /*user_data*/)
+{
+ IDOverrideLibrary *liboverride = id->override_library;
+ if (!liboverride) {
+ return;
+ }
+
+ int track_index;
+ LISTBASE_FOREACH_INDEX (NlaTrack *, track, &adt->nla_tracks, track_index) {
+ char *rna_path_track = BLI_sprintfN("animation_data.nla_tracks[%d]", track_index);
+
+ LISTBASE_FOREACH (NlaStrip *, strip, &track->strips) {
+ version_liboverride_nla_strip_frame_start_end(liboverride, rna_path_track, strip);
+ }
+
+ MEM_freeN(rna_path_track);
+ }
+}
+
/* NOLINTNEXTLINE: readability-function-size */
void blo_do_versions_300(FileData *fd, Library * /*lib*/, Main *bmain)
{
@@ -4417,5 +4484,7 @@ void blo_do_versions_300(FileData *fd, Library * /*lib*/, Main *bmain)
*/
{
/* Keep this block, even when empty. */
+
+ BKE_animdata_main_cb(bmain, version_liboverride_nla_frame_start_end, NULL);
}
}
diff --git a/source/blender/makesrna/intern/rna_nla.c b/source/blender/makesrna/intern/rna_nla.c
index 6ba6973de26..0e9658e46f6 100644
--- a/source/blender/makesrna/intern/rna_nla.c
+++ b/source/blender/makesrna/intern/rna_nla.c
@@ -739,6 +739,10 @@ static void rna_def_nlastrip(BlenderRNA *brna)
RNA_def_property_ui_text(prop, "Start Frame", "");
RNA_def_property_update(
prop, NC_ANIMATION | ND_NLA | NA_EDITED, "rna_NlaStrip_transform_update");
+ /* The `frame_start` and `frame_end` properties should NOT be considered for library overrides,
+ * as their setters always enforce a valid state. While library overrides are applied, the
+ * intermediate state may be invalid, even when the end state is valid. */
+ RNA_def_property_override_flag(prop, PROPOVERRIDE_NO_COMPARISON);
prop = RNA_def_property(srna, "frame_end", PROP_FLOAT, PROP_TIME);
RNA_def_property_float_sdna(prop, NULL, "end");
@@ -746,6 +750,29 @@ static void rna_def_nlastrip(BlenderRNA *brna)
RNA_def_property_ui_text(prop, "End Frame", "");
RNA_def_property_update(
prop, NC_ANIMATION | ND_NLA | NA_EDITED, "rna_NlaStrip_transform_update");
+ /* The `frame_start` and `frame_end` properties should NOT be considered for library overrides,
+ * as their setters always enforce a valid state. While library overrides are applied, the
+ * intermediate state may be invalid, even when the end state is valid. */
+ RNA_def_property_override_flag(prop, PROPOVERRIDE_NO_COMPARISON);
+
+ /* Strip extents without enforcing a valid state. */
+ prop = RNA_def_property(srna, "frame_start_raw", PROP_FLOAT, PROP_TIME);
+ RNA_def_property_float_sdna(prop, NULL, "start");
+ RNA_def_property_ui_text(prop,
+ "Start Frame (raw value)",
+ "Same as frame_start, except that any value can be set, including ones "
+ "that create an invalid state");
+ RNA_def_property_update(
+ prop, NC_ANIMATION | ND_NLA | NA_EDITED, "rna_NlaStrip_transform_update");
+
+ prop = RNA_def_property(srna, "frame_end_raw", PROP_FLOAT, PROP_TIME);
+ RNA_def_property_float_sdna(prop, NULL, "end");
+ RNA_def_property_ui_text(prop,
+ "End Frame (raw value)",
+ "Same as frame_end, except that any value can be set, including ones "
+ "that create an invalid state");
+ RNA_def_property_update(
+ prop, NC_ANIMATION | ND_NLA | NA_EDITED, "rna_NlaStrip_transform_update");
/* Strip extents, when called from UI elements : */
prop = RNA_def_property(srna, "frame_start_ui", PROP_FLOAT, PROP_TIME);
@@ -759,6 +786,9 @@ static void rna_def_nlastrip(BlenderRNA *brna)
"property instead");
RNA_def_property_update(
prop, NC_ANIMATION | ND_NLA | NA_EDITED, "rna_NlaStrip_transform_update");
+ /* The `..._ui` properties should NOT be considered for library overrides, as they are meant to
+ * have different behavior than when setting their non-`..._ui` counterparts. */
+ RNA_def_property_override_flag(prop, PROPOVERRIDE_NO_COMPARISON);
prop = RNA_def_property(srna, "frame_end_ui", PROP_FLOAT, PROP_TIME);
RNA_def_property_float_sdna(prop, NULL, "end");
@@ -771,6 +801,9 @@ static void rna_def_nlastrip(BlenderRNA *brna)
"changed, see the \"frame_end\" property instead");
RNA_def_property_update(
prop, NC_ANIMATION | ND_NLA | NA_EDITED, "rna_NlaStrip_transform_update");
+ /* The `..._ui` properties should NOT be considered for library overrides, as they are meant to
+ * have different behavior than when setting their non-`..._ui` counterparts. */
+ RNA_def_property_override_flag(prop, PROPOVERRIDE_NO_COMPARISON);
/* Blending */
prop = RNA_def_property(srna, "blend_in", PROP_FLOAT, PROP_NONE);
--
GitLab