Refactor of 'partial blendfile write' BKE code. #122118
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 & 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
Asset System
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline & 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
5 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#122118
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "mont29:partial-write-refactor"
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 commit introduces a new
PartialWriteContext
class, which wraps around a regular Main struct. It is designed to make writing a set of IDs easy and safe, and to prepare for future 'asset library editing' low-level code.The main goal of this refactor is to provide the same functionalities (or better ones) than existing partial write code, without the very bad hacks currently done. It will replace within the coming weeks all current usages of the
BKE_blendfile_write_partial
API.Essentially, it allows to:
Since the context keeps information to find matches between its content and IDs from the G_MAIN, its lifespan is expected to be very short.
Otherwise, changes in G_MAIN (relationships between IDs, their session uid, etc.) cannot be tracked by the context, leading to inconsistencies.
A partial write context should typically be created, filled, written and deleted within a same function.
Main Known TODOs
Check again on how to handle best the remapping of relative file paths.Properly handle the case where one of the libraries would also be the saved partial blendfile path (IDs from this library then need to be made local, and it can also create broken dependencies in other linked data).ab39b2d9c7
toacd5f01790
@blender-bot build
acd5f01790
to5670843564
5670843564
tocf4267937a
@blender-bot build
cdb44e4236
tofa4c80d4dd
WIP: Refactor of 'partial blendfile write' BKE code.to Refactor of 'partial blendfile write' BKE code.@blender-bot build
The current implementation takes
O(n^2)
for popping n elements, which is quite bad. Python added extra data to theset
data structure to handle this case more efficiently (source). Better useblender::VectorSet
which handles this use case more efficiently. It already has apop
function too.@blender-bot build
Only looked at the headers so far, left some comments inline.
@ -145,0 +155,4 @@
/**
* Partial blendfile writing.
*
* This wrapper around the Main struct is designed to have a very short life span, during which it
Instead or in addition to saying "very short life span", I think it would be good to be a bit more specific. For example you could say that all the added IDs and their dependencies must not change.
@ -145,0 +158,4 @@
* This wrapper around the Main struct is designed to have a very short life span, during which it
* will contain independent copies of the IDs that are added to it.
*
* It also has advanced ways to handle dependencies and libraries for linked IDs.
Any example for what "advanced" means here? Is there a difference between simply "correct" and "advanced"/"smart" features here?
Means specific per-ID usage handling (between #AddIDOptions flags). Updated the comment.
@ -145,0 +171,4 @@
private:
/**
* The filepath that should be used a root for IDs _added_ to the context, when handling
typo (
as
)@ -145,0 +198,4 @@
* context, the added one should be able to 'steal' that session_uid in the context, and
* re-assign a new one to the other ID.
*/
void preempt_session_uid_(ID *ctx_id, unsigned int session_uid);
I'm not necessarily against it, but currently we don't have a rule to add a trailing underscore to private methods and I haven't seen it in Blender before.
It's in our style guide afaik?
https://developer.blender.org/docs/handbook/guidelines/c_cpp/#class-data-member-names
Oh, this is only for data... my bad
Note the data part of "Private/protected data members ". Methods are not data members.
@ -145,0 +221,4 @@
* Ensure that the given library path has a matching Library ID in the context, creating a new
* one if needed.
*/
Library *ensure_library_(blender::StringRefNull library_absolute_path);
Unnecessary
blender::
prefix. Same below.@ -145,0 +230,4 @@
PartialWriteContext(blender::StringRefNull reference_root_filepath);
~PartialWriteContext();
/* Delete regular copy constructor, such that only the default move copy constructor (and
* assignement operator) can be used. */
typo (
assignment
)@ -145,0 +232,4 @@
/* Delete regular copy constructor, such that only the default move copy constructor (and
* assignement operator) can be used. */
PartialWriteContext(const PartialWriteContext &) = delete;
PartialWriteContext(PartialWriteContext &&) = default;
I'd recommend making this type non-copyable and non-movable. It avoids some common pitfalls and this doesn't seem like a type that really has to support moving (use smart pointers if its ownership needs to be moved around).
Even now data members like
matching_uid_map_
don't support moving properly, and I don't think they have to.You can also use
class PartialWriteContext : NonCopyable, NonMovable
to avoid some boilerplate.@ -145,0 +242,4 @@
* nor hanlded as a regular dependency. Instead, the library is _always_ added to the context
* data, and never duplicated. Also, library matching always happens based on absolute filepath.
*/
enum AddIDOptions {
Bit of a personal taste, but I like
AddIDFlags
more because it indicates that this is a bit-flag.What I'd like best and is most future proof is something like this:
Would actually call it
AddIDOperations
or something like that.flags
feel way to generic here.Personally, I don't like enforcing that options have to be bitflags. So I kind of like having this more general struct that then contains a flag and potentially other stuff. I don't have a strong opinion here in this specific case.
Moved the bitflags into an options struct. Kinda makes things more verbose, but agree about it being more future-proof.
@ -145,0 +263,4 @@
* WARNING: This also means that dependencies like obdata, shapekeys or actions are not
* duplicated either.
*/
CLEAR_DEPENDENCIES = 1 << 8,
Any reason for why this is suddenly
1 << 8
instead of1 << 2
?When defining enum bitflags, I try to group related options together, and keep some free bits around them in case more similar flags are added in the future. Helps limiting cases where either related flags are spread all over the place, or having to change values of several unrelated flags when 'inserting' a new one.
@ -145,0 +294,4 @@
* \return The pointer to the duplicated ID in the partial write context.
*/
ID *id_add(const ID *id,
PartialWriteContext::AddIDOptions options,
Shouldn't be necessary to have the
PartialWriteContext::
prefix here I think.@ -145,0 +342,4 @@
/**
* Fully empty the partial write context.
*/
void clear(void);
No need for the
void
parameter in C++.@ -262,0 +268,4 @@
/**
* Make given \a bmain empty again, and free all runtime mappings.
*
* \note: Unlike #BKE_main_free, only process the given \a bmain, without handling any potential
Would be good to mention if this is essentially the same as
destroy+init
or if this has different behavior.Similar with some slight differences, added precision in the comment.
@blender-bot build
@blender-bot build
IDAddOperations
bitflags.@blender-bot build
@ -1655,0 +1663,4 @@
PartialWriteContext::PartialWriteContext(StringRefNull reference_root_filepath)
: reference_root_filepath_(reference_root_filepath)
{
memset(&this->bmain, 0, sizeof(this->bmain));
Cleaner to do
Main main = {};
in the header if that works.@ -1655,0 +1666,4 @@
memset(&this->bmain, 0, sizeof(this->bmain));
BKE_main_init(this->bmain);
if (!reference_root_filepath_.empty()) {
BLI_strncpy(
Use
STRNCPY
@ -1655,0 +1764,4 @@
void PartialWriteContext::make_local(ID *ctx_id, const int make_local_flags)
{
/* Making an ID local typically resets its session UID, here we want to keep the same value. */
Maybe this should be a flag rather than removing from
id_map
?I'm not familiar with the internals of this, but it's hard to tell if something in
BKE_lib_id_make_local
depends on theid_map
being valid and complete.For the time being I'd rather keep this out of
BKE_lib_id_make_local
, this make local code is already (extremely) complex, and the situation here (wanting to keep the same session uid) is not a 'normal' expected usecase imho.Might reconsider are some point if that kind of manipulation become more common, but I would not expect that.
TBH, I was even considering trying to not use
BKE_lib_id_make_local
at all here, but this is not that trivial either.As for
id_map
, this is currently considered as a pure runtime, short-lifespan util data, so ifBKE_lib_id_make_local
were to use it, it would have to create it itself, and not rely on it existing already.@ -1655,0 +1833,4 @@
*/
BLI_assert(ctx_root_id->session_uid == id->session_uid);
this->ensure_id_user(ctx_root_id, set_fake_user);
return ctx_root_id;
I guess this doesn't work correctly if
DUPLICATE_DEPENDENCIES
is passed in but was not previously used.For the existing usage it seems the options are fixed for the entire context anyway? But maybe there is a use case for having them per
id_add
?It would indeed be a weird case to add (directly or indirectly) an ID without
DUPLICATE_DEPENDENCIES
, and then add it again directly withDUPLICATE_DEPENDENCIES
defined...Right now, in such case dependencies would not be duplicated indeed, but not sure what would be the exact expected behavior here anyway. And I don't think this is a practical issue for the time being?
Added some comment about this limitation now.
@blender-bot build
@ -145,0 +252,4 @@
* \warning By default, when #ADD_DEPENDENCIES is defined, this will also apply to all
* dependencies as well.
*/
MAKE_LOCAL = 1 << 0,
Name of enum item should be capitalized
https://developer.blender.org/docs/handbook/guidelines/c_cpp/#macros-enums-inline-functions
@mod_moder and? What is not capitalized here?
I means
MAKE_LOCAL
->MakeLocal
@mod_moder no, here capitalized means 'IN ALL CAPS' (just like 99% of our current enum values btw...)
Yeah, all new code (and already cleanuped) use this style, for instance
AttrDomain::Point
.MakeLocal
would be called (upper) camel case, btwAs discussed in the chat, out current guideline is (and has been for years already) 'in all capitals'.
I pointed out two weaknesses in the implementation, but up to you as maintainer if you want to consider those important enough to address.
@JacquesLucke do you want to do more review on this one?
@blender-bot build
I'll do another review pass until monday, thanks for asking.
@ -145,0 +295,4 @@
* Options passed to the #id_add method.
*/
struct IDAddOptions {
IDAddOperations operations;
I wonder if the data model (and relations between flags) can be made more explicit here by not using a single bit-flag for everything.
In my experience that simplifies debugging and may also enforce certain invariants by design (e.g. that
CLEAR_DEPENDENCIES
andADD_DEPENDENCIES
are mutually exclusive).It also removes the need for these gaps in the bit flags mentioned in an earlier review pass for unrelated flags. Obviously, this makes the
IDAddOptions
struct slightly bigger, which doesn't matter here imo.I'm not a big fan of adding more parameters than needed here. Issue is not the size, but the fact that it essentially forces the calling code to define the option data before actually calling the function. Not critical, but would rather avoid until it is actually needed.
@ -1655,0 +1746,4 @@
(LIB_ID_CREATE_NO_MAIN | LIB_ID_CREATE_NO_USER_REFCOUNT));
ctx_root_id->tag |= LIB_TAG_TEMP_MAIN;
if (regenerate_session_uid) {
/* Callling #BKE_lib_libblock_session_uid_renew is not needed here, copying already generated a
typo (
calling
)@ -704,3 +704,3 @@
data.id_dst = newid;
data.flag = flag;
BKE_library_foreach_ID_link(bmain, newid, id_copy_libmanagement_cb, &data, IDWALK_NOP);
/* When copying an embedded ID, typically at this point its owner ID pinter will still point to
typo (
pinter
)@ -706,1 +706,3 @@
BKE_library_foreach_ID_link(bmain, newid, id_copy_libmanagement_cb, &data, IDWALK_NOP);
/* When copying an embedded ID, typically at this point its owner ID pinter will still point to
* the owner of the source, this code has no access to its valid (i.e. destination) owner. This
* can be added at some point is needed, but currently the #id_copy_libmanagement_cb callback
typo (
is -> if
)@ -195,3 +236,2 @@
BLI_assert(copy_buffer.is_valid());
/* Ensure that there are no old copy tags around */
BKE_blendfile_write_partial_begin(bmain_src);
While looking for remaining uses of
BKE_blendfile_write_partial_begin
, I noticed thatBKE_blendfile_workspace_config_write
is unused and should probably be removed.Should all of the uses eventually be moved to the new system? If yes, this would be good to mention in the commit/PR description.
Yes the goal is to completely remove
BKE_blendfile_write_partial
API usages. Moving the remaining usecases and cleanup will be done once this PR has landed.@ -149,0 +130,4 @@
scene_name,
nullptr,
{blendfile::PartialWriteContext::IDAddOperations::SET_FAKE_USER}));
id_fake_user_set(&scene_dst->id);
Is this necessary even if
blendfile::PartialWriteContext::IDAddOperations::SET_FAKE_USER
is passed in above?I'm wondering something similar about the
id_fake_user_set
call below.Indeed, these are leftovers from previous versions of this patch, will update the code.
@ -187,0 +193,4 @@
return IDWALK_RET_NOP;
}
/* The Action ID of the destination scene has already been added (created actually) in the copy
Why has this been added already? What is special about the Action ID?
This is added through
copy_buffer.id_create(ID_AC,
few lines above, because the action stored in the paste buffer is a (very) partial copy of the source scene's Action (it only contains FCurves affecting the sequencer). This is handled by the calls tosequencer_copy_animation()
another few lines above.Will update the comment to make it more clear.
@ -187,0 +223,4 @@
};
id_dst = copy_buffer.id_add(
id_src,
{blendfile::PartialWriteContext::IDAddOperations::CLEAR_DEPENDENCIES},
It feels wrong to pass in
CLEAR_DEPENDENCIES
and also a callback for the dependency filter. Why is the filter necessary if dependencies are cleared. Or, if they are not cleared, why is that flag passed in?Passing a value here is not necessary, we can use
IDAddOperations::NOP
instead...TBH I do not have a strong opinion, it just felt best to me to pass a 'default, safe' behavior, as way to say 'this is the default operation, and the callback will override it in some specific cases'.
@blender-bot build
Merged through commits from !123993
Pull request closed