Fix #102662: NLA-Strip Corrupted after reopening file with library override #108072

Manually merged

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).

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).
Sybren A. Stüvel added this to the 3.6 LTS milestone 2023-05-19 11:49:13 +02:00
Sybren A. Stüvel added the
Module
Animation & Rigging
label 2023-05-19 11:49:13 +02:00
Sybren A. Stüvel added 1 commit 2023-05-19 11:49:25 +02:00
3b159bc74a Fix #102662: NLA-Strip Corrupted after reopening file with library 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).
Sybren A. Stüvel requested review from Bastien Montagne 2023-05-19 11:49:33 +02:00
Bastien Montagne requested changes 2023-05-19 12:37:57 +02:00
Bastien Montagne left a comment
Owner

General logic of the patch LGTM, but think it can get some implementation improvements. ;)

To summarize notes below, I would suggest to:

  • Use std::string instead of C strings in the doversion code (simpler, cleaner, safer ;) ).
  • Make version_liboverride_rnapath_change a liboverride actual API, including all the update of runtime data (rna paths mapping) that is needed.
General logic of the patch LGTM, but think it can get some implementation improvements. ;) To summarize notes below, I would suggest to: * Use `std::string` instead of C strings in the doversion code (simpler, cleaner, safer ;) ). * Make `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 of BKE_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 even BKE_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 (using BLI_strdup or so currently).

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 of `BKE_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 even `BKE_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 (using `BLI_strdup` or so currently).
dr.sybren marked this conversation as resolved
@ -2243,0 +2265,4 @@
/** Delete the property override, if it exists.
*
* No-op if the property voerride cannot be found.

voerride ? ;)

`voerride` ? ;)
Author
Member

yeah, "voer" is Dutch for "feed" or "food", so it's a ride where you're also being fed!

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 ?

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 in version_liboverride_nla_strip_frame_start_end cleaner and safer I think?

Since we are in a C++ file, this could be an `std::string`, would make string manipulation and especially in `version_liboverride_nla_strip_frame_start_end` cleaner and safer I think?
dr.sybren marked this conversation as resolved
@ -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 ;) )

`have a different behavior than when setting` ? (missing comparison, also use US spelling ;) )
dr.sybren marked this conversation as resolved
Author
Member

Thanks for the thorough review!

Use std::string instead of C strings in the doversion code (simpler, cleaner, safer ;) ).

Yeah, I've been pondering that too. However, BLI_sprintfN nicely allocates on the heap, which is what is kinda necessary to assign the op->rna_path pointer. Of course we can allocate a std::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 would rather see that as an API of BKE_lib_override.

👍 I'm all for that.

Thanks for the thorough review! > Use `std::string` instead of C strings in the doversion code (simpler, cleaner, safer ;) ). Yeah, I've been pondering that too. However, `BLI_sprintfN` nicely allocates on the heap, which is what is kinda necessary to assign the `op->rna_path` pointer. Of course we can allocate a `std::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 would rather see that as an API of `BKE_lib_override`. :+1: I'm all for that.
Sybren A. Stüvel added 1 commit 2023-05-19 14:00:28 +02:00
Sybren A. Stüvel added 1 commit 2023-05-19 14:35:37 +02:00
Sybren A. Stüvel added 1 commit 2023-05-19 15:20:07 +02:00
Bastien Montagne requested changes 2023-05-19 15:45:59 +02:00
Bastien Montagne left a comment
Owner

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.

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 (using BLI_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.

This and the `BLI_ghash_remove` call could be a single operation (using `BLI_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.
dr.sybren marked this conversation as resolved
@ -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...

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...
dr.sybren marked this conversation as resolved
Sybren A. Stüvel added 1 commit 2023-05-19 15:59:41 +02:00
Bastien Montagne approved these changes 2023-05-19 16:12:49 +02:00
Bastien Montagne left a comment
Owner

Looks good now, minor detail in comment below, no need for further review once this is addressed!

Looks 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. ;)

You can just as well use `override_runtime` here too. ;)
dr.sybren changed target branch from main to blender-v3.6-release 2023-05-22 10:34:01 +02:00
Sybren A. Stüvel added 2 commits 2023-05-22 10:36:38 +02:00
Sybren A. Stüvel manually merged commit 7a06588f05 into blender-v3.6-release 2023-05-22 14:07:36 +02:00
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
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
EEVEE & Viewport
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
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
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
EEVEE & Viewport
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
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
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
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#108072
No description provided.