Core: Add proper support to add or copy IDs into libraries. #108328

Merged
Bastien Montagne merged 2 commits from mont29/blender:F-idcopy-linkeddata into main 2024-03-06 17:05:21 +01:00

This change allows to assign a library to a newly created (or copied) ID.

Note that there are at least two known usecases for this change:

  • Liboverrides, as with recursive resync and proxies conversion it
    often ends up creating 'virtual' linked data that does not actually
    exists in the library blend files.
  • Complex versionning code (do_versions_after_setup) when it needs
    to create new IDs (currently handling linked data that way is just not
    supported!).

Implements #107847.

This change allows to assign a library to a newly created (or copied) ID. Note that there are at least two known usecases for this change: * Liboverrides, as with recursive resync and proxies conversion it often ends up creating 'virtual' linked data that does not actually exists in the library blend files. * Complex versionning code (`do_versions_after_setup`) when it needs to create new IDs (currently handling linked data that way is just not supported!). Implements #107847.
Bastien Montagne added the
Module
Core
Interest
Overrides
labels 2023-05-26 19:21:40 +02:00
Bastien Montagne added this to the Core project 2023-05-26 19:21:47 +02:00
Bastien Montagne force-pushed F-idcopy-linkeddata from a330a837fe to 229372c049 2024-02-27 18:14:14 +01:00 Compare
Bastien Montagne changed title from WIP Initial quick go at proper support of runtime adding IDs into libraries. to WIP: Initial quick go at proper support of runtime adding IDs into libraries. 2024-02-27 18:14:22 +01:00
Bastien Montagne force-pushed F-idcopy-linkeddata from 229372c049 to 7ac206722f 2024-02-28 18:52:38 +01:00 Compare
Bastien Montagne changed title from WIP: Initial quick go at proper support of runtime adding IDs into libraries. to WIP: Core: Add proper support to add or copy IDs into libraries. 2024-02-28 18:55:22 +01:00
Author
Owner

@blender-bot build

@blender-bot build
Bastien Montagne force-pushed F-idcopy-linkeddata from 7ac206722f to 299fa7c85e 2024-02-29 14:26:26 +01:00 Compare
Bastien Montagne force-pushed F-idcopy-linkeddata from 299fa7c85e to 9fdae215fe 2024-02-29 15:05:35 +01:00 Compare
Bastien Montagne requested review from Brecht Van Lommel 2024-02-29 15:17:36 +01:00
Author
Owner

@blender-bot build

@blender-bot build
Bastien Montagne changed title from WIP: Core: Add proper support to add or copy IDs into libraries. to Core: Add proper support to add or copy IDs into libraries. 2024-02-29 15:18:00 +01:00
Author
Owner

BTW, this PR became a requirement for the GPv3 readfile conversion code to work also on linked data (see !118705). which is why it was resurrected now.

BTW, this PR became a requirement for the GPv3 readfile conversion code to work also on linked data (see !118705). which is why it was resurrected now.
Bastien Montagne force-pushed F-idcopy-linkeddata from 9fdae215fe to df759d7543 2024-03-04 10:50:47 +01:00 Compare
Bastien Montagne requested review from Sergey Sharybin 2024-03-04 10:51:21 +01:00
Author
Owner

@blender-bot build

@blender-bot build
Sergey Sharybin reviewed 2024-03-04 11:38:35 +01:00
Sergey Sharybin left a comment
Owner

I do not fully understand this part:

This change allows to assign a library to a newly created (or copied) ID.

From the change it seems that it basically boils down to id->lib = owner_lib_from_the_function_argument.

Why do we need to modify the API (and potentially make it more confusing, when the owner_library is not from the bmain instead of simply doing id->lib = my_non_trivial_lib after copy in those non-trivial? Is it because those non-trivial cases do a deep-copy, and require to set library for all IDs which are being copied during the deep-copy?

I do not fully understand this part: > This change allows to assign a library to a newly created (or copied) ID. From the change it seems that it basically boils down to `id->lib = owner_lib_from_the_function_argument`. Why do we need to modify the API (and potentially make it more confusing, when the `owner_library` is not from the `bmain` instead of simply doing `id->lib = my_non_trivial_lib` after copy in those non-trivial? Is it because those non-trivial cases do a deep-copy, and require to set library for all IDs which are being copied during the deep-copy?
@ -93,0 +97,4 @@
* Same as #BKE_animdata_copy, but allows to duplicate Action IDs into a library.
*
* \param owner_library the Library to 'assign' the newly created ID to. Use std::nullopt for
* default behavior (i.e. behavior of the #BKE_animdata_copy function).

Use std::nullopt

I am not really sure std::optional<Library *> is the most clear API. At least not without extra elaboration between difference between using std::nullopt and nullptr.

Note that it might be the proper choice for the API, I am just lacking some background information.

for default behavior (i.e. behavior of the #BKE_animdata_copy function).

It would be easier to understand if the behavior is described explicitly, rather than referring to "default" or "same as BKE_animdata_copy". Something like: When library is passed as std::nullopt the new data-block is assigned to the same library as the source.

> Use std::nullopt I am not really sure `std::optional<Library *>` is the most clear API. At least not without extra elaboration between difference between using `std::nullopt` and `nullptr`. Note that it might be the proper choice for the API, I am just lacking some background information. > for default behavior (i.e. behavior of the #BKE_animdata_copy function). It would be easier to understand if the behavior is described explicitly, rather than referring to "default" or "same as BKE_animdata_copy". Something like: `When library is passed as std::nullopt the new data-block is assigned to the same library as the source`.
Author
Owner

Using std::nullopt (and std::optional in fact) is the simplest, least noisy way to avoid having two dedicated codepaths all over the place, since we cannot rely on nullptr as a 'do not use' value here (null library actually means 'local ID'...).

Unfortunately the 'default behavior' is far from simple and clear - it changes depending on whether we are copying a 'regular' Main ID, vs. a non-main one, and a few other parameters.

Using `std::nullopt` (and `std::optional` in fact) is the simplest, least noisy way to avoid having two dedicated codepaths all over the place, since we cannot rely on `nullptr` as a 'do not use' value here (null library actually means 'local ID'...). Unfortunately the 'default behavior' is far from simple and clear - it changes depending on whether we are copying a 'regular' Main ID, vs. a non-main one, and a few other parameters.

Using std::nullopt (and std::optional in fact) is the simplest, least noisy way to avoid having two dedicated codepaths all over the place, since we cannot rely on nullptr as a 'do not use' value here (null library actually means 'local ID'...).

I think simple tweak would be welcome then: \param owner_library the Library to 'assign' the newly created ID to. Use nullptr to make ID not use any library. Use std::nullopt for default behavior (i.e. behavior of the #BKE_animdata_copy function).. It will help a lot to people who do not have that much proficiency in the low-level code.

Unfortunately the 'default behavior' is far from simple and clear - it changes depending on whether we are copying a 'regular' Main ID, vs. a non-main one, and a few other parameters.

That's not very great, as it is not documented, so referring to BKE_animdata_copy does not really help understanding what's going on. Are there some simple rules to word it something like "The behavior of the library assignment could be tricky for certain cases. For simple cases when copying user-facing IDs the library of the new ID is the same as the source. " ?

> Using std::nullopt (and std::optional in fact) is the simplest, least noisy way to avoid having two dedicated codepaths all over the place, since we cannot rely on nullptr as a 'do not use' value here (null library actually means 'local ID'...). I think simple tweak would be welcome then: `\param owner_library the Library to 'assign' the newly created ID to. Use `nullptr` to make ID not use any library. Use std::nullopt for default behavior (i.e. behavior of the #BKE_animdata_copy function).`. It will help a lot to people who do not have that much proficiency in the low-level code. > Unfortunately the 'default behavior' is far from simple and clear - it changes depending on whether we are copying a 'regular' Main ID, vs. a non-main one, and a few other parameters. That's not very great, as it is not documented, so referring to `BKE_animdata_copy` does not really help understanding what's going on. Are there some simple rules to word it something like "The behavior of the library assignment could be tricky for certain cases. For simple cases when copying user-facing IDs the library of the new ID is the same as the source. <and maybe some other very common use-cases>" ?
Author
Owner

In most typical cases, copied or created IDs are local data (nullptr lib). Added some comments there (and in BKE_lib_id itself).

In most typical cases, copied or created IDs are local data (`nullptr` lib). Added some comments there (and in `BKE_lib_id` itself).
mont29 marked this conversation as resolved
@ -93,0 +99,4 @@
* \param owner_library the Library to 'assign' the newly created ID to. Use std::nullopt for
* default behavior (i.e. behavior of the #BKE_animdata_copy function).
*/
struct AnimData *BKE_animdata_copy_in_lib(Main *bmain,

struct AnimData -> AnimData

`struct AnimData` -> `AnimData`
mont29 marked this conversation as resolved
@ -1251,2 +1276,4 @@
ListBase *lb = which_libbase(bmain, type);
/* This is important in 'readfile doversion after liblink' context mainly, but is a good
* consistency change in general: ID created for a Main should get that main's current

By the change do you refer to the change in the ID library pointer, or the change as in this patch?

By the change do you refer to the change in the ID library pointer, or the change as in this patch?
Author
Owner

This comment was already there before this patch (see a bit below in original code). Will remove the reference to 'change' though this should never have made it into into a code comment, since by definition you do not have 'commit context' when reading code comments. :(

This comment was already there before this patch (see a bit below in original code). Will remove the reference to 'change' though this should never have made it into into a code comment, since by definition you do not have 'commit context' when reading code comments. :(
mont29 marked this conversation as resolved
@ -239,6 +239,9 @@ static void main_namemap_populate(
/* Insert the full name into the set. */
UniqueName_Key key;
STRNCPY(key.name, id->name + 2);
if (STREQ("HairCubeArmatureGroup", key.name)) {

Some debug code sneaked into the patch.

Some debug code sneaked into the patch.
mont29 marked this conversation as resolved
Author
Owner

I do not fully understand this part:

This change allows to assign a library to a newly created (or copied) ID.

From the change it seems that it basically boils down to id->lib = owner_lib_from_the_function_argument.

Why do we need to modify the API (and potentially make it more confusing, when the owner_library is not from the bmain instead of simply doing id->lib = my_non_trivial_lib after copy in those non-trivial? Is it because those non-trivial cases do a deep-copy, and require to set library for all IDs which are being copied during the deep-copy?

It is indeed not as simple. Some IDs are (potentially, depending on parameters) deep-copied (Actions, ShapeKeys, and embedded IDs). This is why this new optional parameter needs to be passed all over copy code, including the ID types' own callbacks.

A later goal behind this patch is also to make the main, basic API way simpler, predictable, and have way less weird heuristics about how to create or copy an ID. Then code that needs more complex behaviors should only call the 'complex' API, with more explicit parameters (e.g. which library to use, instead of heuristics like 'use the Main library pointer if we create an ID in a Main, copy the source ID lib pointer if we create a non-Main ID, etc.').

Areas of code needing complex behaviors include versioning (this one has way too many loose ends currently), depsgraph, liboverrides...

> I do not fully understand this part: > > This change allows to assign a library to a newly created (or copied) ID. > > From the change it seems that it basically boils down to `id->lib = owner_lib_from_the_function_argument`. > > Why do we need to modify the API (and potentially make it more confusing, when the `owner_library` is not from the `bmain` instead of simply doing `id->lib = my_non_trivial_lib` after copy in those non-trivial? Is it because those non-trivial cases do a deep-copy, and require to set library for all IDs which are being copied during the deep-copy? It is indeed not as simple. Some IDs are (potentially, depending on parameters) deep-copied (Actions, ShapeKeys, and embedded IDs). This is why this new optional parameter needs to be passed all over copy code, including the ID types' own callbacks. A later goal behind this patch is also to make the main, basic API way simpler, predictable, and have way less weird heuristics about how to create or copy an ID. Then code that needs more complex behaviors should only call the 'complex' API, with more explicit parameters (e.g. which library to use, instead of heuristics like 'use the Main library pointer if we create an ID in a Main, copy the source ID lib pointer if we create a non-Main ID, etc.'). Areas of code needing complex behaviors include versioning (this one has way too many loose ends currently), depsgraph, liboverrides...

It would indeed be great to simplify the API. I believe even for the tricky cases we can find a better solution than potentially-conflicting parameters. However, that's not something we should be doing to unlock the current project.

It would indeed be great to simplify the API. I believe even for the tricky cases we can find a better solution than potentially-conflicting parameters. However, that's not something we should be doing to unlock the current project.
Sergey Sharybin reviewed 2024-03-04 12:22:11 +01:00
@ -1253,3 +1300,3 @@
BKE_main_lock(bmain);
BLI_addtail(lb, id);
BKE_id_new_name_validate(bmain, lb, id, name, false);
BKE_id_new_name_validate(bmain, lb, id, name, true);

What are the consequences of this change? Is it just potentially performance, or could it also lead to functional changes?
Shall the library assignment be delayed for until after the unique name is ensured to minimize the amount of functional changes?

What are the consequences of this change? Is it just potentially performance, or could it also lead to functional changes? Shall the library assignment be delayed for until after the unique name is ensured to minimize the amount of functional changes?
Author
Owner

Well, it is delayed in 'split Main' case (i.e. readfile situation, where we have on e Main for each library). For regular monolithic Main case however, it is critical for the lib pointer to have its definitive value before we call this function, to ensure that a 'linked' ID gets a lib-relative unique name (and is sorted appropriately) in the Main.

The change from false to true here simply ensures that the name validation does not skip linked data, which becomes mandatory in this PR, since it allows creating linked IDs at runtime.

Well, it is delayed in 'split Main' case (i.e. readfile situation, where we have on e Main for each library). For regular monolithic Main case however, it is critical for the `lib` pointer to have its definitive value _before_ we call this function, to ensure that a 'linked' ID gets a lib-relative unique name (and is sorted appropriately) in the Main. The change from `false` to `true` here simply ensures that the name validation does not skip linked data, which becomes mandatory in this PR, since it allows creating linked IDs at runtime.
Sergey Sharybin reviewed 2024-03-04 14:54:00 +01:00
Sergey Sharybin left a comment
Owner

Just couple of more small things I've noticed after re-reading the code once again.

It does feel unideal, but I can not think of a better alternative which can be easily fit into the current design, so is something we'd need to accept until a bigger change.

Just couple of more small things I've noticed after re-reading the code once again. It does feel unideal, but I can not think of a better alternative which can be easily fit into the current design, so is something we'd need to accept until a bigger change.
@ -1253,0 +1284,4 @@
* from that library. In that case, the library can be set later, and it avoids
* synchronization issues in the namemap between the one of that temp 'library' Main and
* the library ID runtime namemap itself. In a way, the ID can be assumed local to the
* current Main, for its assignement to this Main.

assignement -> assignment?

assignement -> assignment?
mont29 marked this conversation as resolved
@ -276,4 +278,3 @@
}
id_us_min(local_id);
/* TODO: Handle this properly in LIB_NO_MAIN case as well (i.e. resync case). Or offload to

Is this TODO still relevant?

Is this TODO still relevant?
Author
Owner

Probably not, nor the code in the if just below... But would rather cleanup this in a separate commit tbh :)

_Probably_ not, nor the code in the `if` just below... But would rather cleanup this in a separate commit tbh :)
Sergey Sharybin approved these changes 2024-03-04 16:10:58 +01:00
Bastien Montagne force-pushed F-idcopy-linkeddata from f366dab997 to bcdb3f1f99 2024-03-06 16:39:58 +01:00 Compare
Author
Owner

@blender-bot build

@blender-bot build
Bastien Montagne merged commit ea73438e8a into main 2024-03-06 17:05:21 +01:00
Bastien Montagne deleted branch F-idcopy-linkeddata 2024-03-06 17:05:23 +01:00
Sign in to join this conversation.
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#108328
No description provided.