Core: Add proper support to add or copy IDs into libraries. #108328
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
Code Documentation
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#108328
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "mont29/blender:F-idcopy-linkeddata"
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?
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:
often ends up creating 'virtual' linked data that does not actually
exists in the library blend files.
do_versions_after_setup
) when it needsto create new IDs (currently handling linked data that way is just not
supported!).
Implements #107847.
a330a837fe
to229372c049
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.229372c049
to7ac206722f
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.@blender-bot build
7ac206722f
to299fa7c85e
299fa7c85e
to9fdae215fe
@blender-bot build
WIP: Core: Add proper support to add or copy IDs into libraries.to Core: Add proper support to add or copy IDs into libraries.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.
9fdae215fe
todf759d7543
@blender-bot build
I do not fully understand this part:
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 thebmain
instead of simply doingid->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).
I am not really sure
std::optional<Library *>
is the most clear API. At least not without extra elaboration between difference between usingstd::nullopt
andnullptr
.Note that it might be the proper choice for the API, I am just lacking some background information.
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
.Using
std::nullopt
(andstd::optional
in fact) is the simplest, least noisy way to avoid having two dedicated codepaths all over the place, since we cannot rely onnullptr
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.
I think simple tweak would be welcome then:
\param owner_library the Library to 'assign' the newly created ID to. Use
nullptrto 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.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. " ?In most typical cases, copied or created IDs are local data (
nullptr
lib). Added some comments there (and inBKE_lib_id
itself).@ -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
@ -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?
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. :(
@ -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.
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.
@ -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?
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
totrue
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.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?
@ -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?
Probably not, nor the code in the
if
just below... But would rather cleanup this in a separate commit tbh :)f366dab997
tobcdb3f1f99
@blender-bot build