Fix #102662: NLA-Strip Corrupted after reopening file with library override #108072
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#108072
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "dr.sybren/blender:fix/102662-nla-override-corruption"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Mark
NlaStrip.frame_{start,end}
andNlaStrip.frame_{start,end}_ui
asto-be-ignored for the library override system, and add a new set of RNA
properties
frame_{start,end}_raw
that the library override system canuse.
Versioning code ensures that overrides on
frame_{start,end}
arealtered 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 withoutdoing 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).
General logic of the patch LGTM, but think it can get some implementation improvements. ;)
To summarize notes below, I would suggest to:
std::string
instead of C strings in the doversion code (simpler, cleaner, safer ;) ).version_liboverride_rnapath_change
a liboverride actual API, including all the update of runtime data (rna paths mapping) that is needed.@ -2243,0 +2248,4 @@
* \param to_rna_path The new RNA path. This should be allocated on the heap, and ownership will be
* transferred to the library override system.
*/
static void version_liboverride_rnapath_change(IDOverrideLibrary *liboverride,
I would rather see that as an API of
BKE_lib_override
. On top of preventing external code from generating invalid liboverride data (and relying on said external code to reliably call the proper extra function to make it valid again), it also allows to remove altogether the need ofBKE_lib_override_library_rna_path_cache_clear
.The suggested
BKE_lib_override_library_property_rna_path_change(IDOverrideLibrary *liboverride, IDOverrideLibraryProperty *op, char *new_rna_path)
(or maybe evenBKE_lib_override_library_property_rna_path_change(IDOverrideLibrary *liboverride, const char *old_rna_path, char *new_rna_path)
?) would then be able to also update the internal mapping (and potentially any other future related runtime data). I also think it should not assume that it can steal owner ship of the string, and just do its own memory management (usingBLI_strdup
or so currently).@ -2243,0 +2265,4 @@
/** Delete the property override, if it exists.
*
* No-op if the property voerride cannot be found.
voerride
? ;)yeah, "voer" is Dutch for "feed" or "food", so it's a ride where you're also being fed!
@ -2243,0 +2267,4 @@
*
* No-op if the property voerride cannot be found.
*/
static void version_liboverride_property_delete(IDOverrideLibrary *liboverride,
This could honestly also be moved into BKE_lib_override...
BKE_lib_override_library_property_search_and_delete
?@ -2318,0 +2411,4 @@
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);
Since we are in a C++ file, this could be an
std::string
, would make string manipulation and especially inversion_liboverride_nla_strip_frame_start_end
cleaner and safer I think?@ -760,2 +787,4 @@
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 behaviour when setting their non-`..._ui` counterparts. */
have a different behavior than when setting
? (missing comparison, also use US spelling ;) )Thanks for the thorough review!
Yeah, I've been pondering that too. However,
BLI_sprintfN
nicely allocates on the heap, which is what is kinda necessary to assign theop->rna_path
pointer. Of course we can allocate astd::string
on the heap as well, but that then cannot be directly assigned either, so making more copies would be required. BUT, with the API changes you suggest, I think this will be easier to do. Might cost a bit more copying, but doing the API part the proper way removes a whole lot of clearing & rebuilding that cache, so in the end it'll be faster anyway.👍 I'm all for that.
Talked about it IRL, to summarize think
BKE_lib_override_library_property_rna_path_change
can go a bit lower-level in handling of the rna paths cache, and therefore be also more efficient in it.@ -3530,0 +3544,4 @@
const char *new_rna_path)
{
IDOverrideLibraryProperty *override_property;
override_property = BKE_lib_override_library_property_find(override, old_rna_path);
This and the
BLI_ghash_remove
call could be a single operation (usingBLI_ghash_popkey
), i.e. one lookup instead of two.Not sure if it's worth doing that now, but otherwise I would at least leave a comment about this potential optimization in case this function is called in a performance-critical context in the future.
This could be implemented as a private
lib_override_library_property_find_pop(IDOverrideLibrary *override, char *rna_path)
utility e.g.@ -3530,0 +3551,4 @@
}
/* Remove the property from the lookup mapping. */
const bool has_lookup = !ELEM(
NOTE: At least as a comment, that this will always be true in current logic, since
BKE_lib_override_library_property_find
will always ensure that there is a mapping...BKE_lib_override_library_property_rna_path_change
95a4f1c79eLooks good now, minor detail in comment below, no need for further review once this is addressed!
@ -3530,0 +3557,4 @@
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->rna_path_to_override_properties,
You can just as well use
override_runtime
here too. ;)