Refactor of 'partial blendfile write' BKE code. #122118

Closed
Bastien Montagne wants to merge 20 commits from mont29:partial-write-refactor into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.

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:

  • Add (aka copy) IDs from the G_MAIN to the partial write context.
    • This process handles dependencies and libraries automatically.
    • A refined handling of dependencies is possible through an optional 'filtering' callback.
  • Keep track of added IDs, to allow de-duplication in case data is added more than once.
  • Cleanup the context (i.e. remove unused IDs).
  • Write the context to disk as a blendfile.

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.

Note


Ultimately, this new system will replace all usages of the existing BKE_blendfile_write_partial API. As a demo, the copy buffer for the VSE and the python's API to write into libraries (bpy.data.libraries.write()) have been updated to use the new partial write context. This means that the blendfile_io unittest also uses this new code.

Note


This PR will be split in several commits (utils ones like the change to blender::Set, implementation of the PartialWriteContext itself, and its different usages.

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).
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: * Add (aka copy) IDs from the G_MAIN to the partial write context. * This process handles dependencies and libraries automatically. * A refined handling of dependencies is possible through an optional 'filtering' callback. * Keep track of added IDs, to allow de-duplication in case data is added more than once. * Cleanup the context (i.e. remove unused IDs). * Write the context to disk as a blendfile. 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. > **Note** > Ultimately, this new system will replace all usages of the existing `BKE_blendfile_write_partial` API. As a demo, the copy buffer for the VSE and the python's API to write into libraries (`bpy.data.libraries.write()`) have been updated to use the new partial write context. This means that the `blendfile_io` unittest also uses this new code. > **Note** > This PR will be split in several commits (utils ones like the change to `blender::Set`, implementation of the `PartialWriteContext` itself, and its different usages. ### Main Known TODOs * [x] ~~Check again on how to handle best the remapping of relative file paths.~~ * [x] ~~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).~~
Bastien Montagne force-pushed partial-write-refactor from ab39b2d9c7 to acd5f01790 2024-05-22 21:24:36 +02:00 Compare
Author
Owner

@blender-bot build

@blender-bot build
Bastien Montagne force-pushed partial-write-refactor from acd5f01790 to 5670843564 2024-05-28 09:32:56 +02:00 Compare
Bastien Montagne force-pushed partial-write-refactor from 5670843564 to cf4267937a 2024-06-07 17:59:46 +02:00 Compare
Author
Owner

@blender-bot build

@blender-bot build
Bastien Montagne force-pushed partial-write-refactor from cdb44e4236 to fa4c80d4dd 2024-06-17 15:10:19 +02:00 Compare
Bastien Montagne changed title from WIP: Refactor of 'partial blendfile write' BKE code. to Refactor of 'partial blendfile write' BKE code. 2024-06-17 15:11:08 +02:00
Author
Owner

@blender-bot build

@blender-bot build
Bastien Montagne requested review from Brecht Van Lommel 2024-06-17 15:12:41 +02:00
Bastien Montagne requested review from Campbell Barton 2024-06-17 15:12:42 +02:00
Bastien Montagne requested review from Jacques Lucke 2024-06-17 15:12:42 +02:00
Bastien Montagne requested review from Hans Goudey 2024-06-17 15:12:43 +02:00
Member

his PR adds a pop function to blender::Set

The current implementation takes O(n^2) for popping n elements, which is quite bad. Python added extra data to the set data structure to handle this case more efficiently (source). Better use blender::VectorSet which handles this use case more efficiently. It already has a pop function too.

> his PR adds a `pop` function to `blender::Set` The current implementation takes `O(n^2)` for popping n elements, which is quite bad. Python added extra data to the `set` data structure to handle this case more efficiently ([source](https://stackoverflow.com/a/66592754)). Better use `blender::VectorSet` which handles this use case more efficiently. It already has a `pop` function too.
Bastien Montagne added 2 commits 2024-06-17 17:34:01 +02:00
Address review: use VectorSet instead of Set.
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
4cb720fbe0
Author
Owner

@blender-bot build

@blender-bot build
Jacques Lucke reviewed 2024-06-17 17:38:59 +02:00
Jacques Lucke left a comment
Member

Only looked at the headers so far, left some comments inline.

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
Member

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.

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.
mont29 marked this conversation as resolved
@ -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.
Member

Any example for what "advanced" means here? Is there a difference between simply "correct" and "advanced"/"smart" features here?

Any example for what "advanced" means here? Is there a difference between simply "correct" and "advanced"/"smart" features here?
Author
Owner

Means specific per-ID usage handling (between #AddIDOptions flags). Updated the comment.

Means specific per-ID usage handling (between #AddIDOptions flags). Updated the comment.
mont29 marked this conversation as resolved
@ -145,0 +171,4 @@
private:
/**
* The filepath that should be used a root for IDs _added_ to the context, when handling
Member

typo (as)

typo (`as`)
mont29 marked this conversation as resolved
@ -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);
Member

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.

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.
Author
Owner
It's in our style guide afaik? https://developer.blender.org/docs/handbook/guidelines/c_cpp/#class-data-member-names
Author
Owner

Oh, this is only for data... my bad

Oh, this is only for data... my bad
Member

Note the data part of "Private/protected data members ". Methods are not data members.

Note the **data** part of "Private/protected data members ". Methods are not data members.
mont29 marked this conversation as resolved
@ -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);
Member

Unnecessary blender:: prefix. Same below.

Unnecessary `blender::` prefix. Same below.
mont29 marked this conversation as resolved
@ -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. */
Member

typo (assignment)

typo (`assignment`)
mont29 marked this conversation as resolved
@ -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;
Member

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.

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.
mont29 marked this conversation as resolved
@ -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 {
Member

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:

struct AddIDOptions {
  AddIDFlags flags;
};
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: ``` struct AddIDOptions { AddIDFlags flags; }; ```
Author
Owner

Would actually call it AddIDOperations or something like that. flags feel way to generic here.

Would actually call it `AddIDOperations` or something like that. `flags` feel way to generic here.
Member

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.

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.
Author
Owner

Moved the bitflags into an options struct. Kinda makes things more verbose, but agree about it being more future-proof.

Moved the bitflags into an options struct. Kinda makes things more verbose, but agree about it being more future-proof.
mont29 marked this conversation as resolved
@ -145,0 +263,4 @@
* WARNING: This also means that dependencies like obdata, shapekeys or actions are not
* duplicated either.
*/
CLEAR_DEPENDENCIES = 1 << 8,
Member

Any reason for why this is suddenly 1 << 8 instead of 1 << 2?

Any reason for why this is suddenly `1 << 8` instead of `1 << 2`?
Author
Owner

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.

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.
mont29 marked this conversation as resolved
@ -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,
Member

Shouldn't be necessary to have the PartialWriteContext:: prefix here I think.

Shouldn't be necessary to have the `PartialWriteContext::` prefix here I think.
mont29 marked this conversation as resolved
@ -145,0 +342,4 @@
/**
* Fully empty the partial write context.
*/
void clear(void);
Member

No need for the void parameter in C++.

No need for the `void` parameter in C++.
mont29 marked this conversation as resolved
@ -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
Member

Would be good to mention if this is essentially the same as destroy+init or if this has different behavior.

Would be good to mention if this is essentially the same as `destroy+init` or if this has different behavior.
Author
Owner

Similar with some slight differences, added precision in the comment.

Similar with some slight differences, added precision in the comment.
mont29 marked this conversation as resolved
Bastien Montagne added 1 commit 2024-06-17 18:05:43 +02:00
Bastien Montagne added 1 commit 2024-06-17 19:05:36 +02:00
Updates from review.
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
985bbc474a
Author
Owner

@blender-bot build

@blender-bot build
Author
Owner

@blender-bot build

@blender-bot build
Bastien Montagne added 1 commit 2024-06-17 21:13:04 +02:00
Use a 'IDAddOptions' struct around IDAddOperations bitflags.
All checks were successful
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
e289c23588
Author
Owner

@blender-bot build

@blender-bot build
Brecht Van Lommel requested changes 2024-06-24 11:31:52 +02:00
Dismissed
@ -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.

Cleaner to do `Main main = {};` in the header if that works.
mont29 marked this conversation as resolved
@ -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

Use `STRNCPY`
mont29 marked this conversation as resolved
@ -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 the id_map being valid and complete.

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 the `id_map` being valid and complete.
Author
Owner

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 if BKE_lib_id_make_local were to use it, it would have to create it itself, and not rely on it existing already.

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 if `BKE_lib_id_make_local` were to use it, it would have to create it itself, and not rely on it existing already.
mont29 marked this conversation as resolved
@ -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?

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`?
Author
Owner

It would indeed be a weird case to add (directly or indirectly) an ID without DUPLICATE_DEPENDENCIES, and then add it again directly with DUPLICATE_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?

It would indeed be a weird case to add (directly or indirectly) an ID without `DUPLICATE_DEPENDENCIES`, and then add it again directly with `DUPLICATE_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?
Author
Owner

Added some comment about this limitation now.

Added some comment about this limitation now.
mont29 marked this conversation as resolved
Bastien Montagne added 2 commits 2024-06-24 12:47:10 +02:00
Updates from review.
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
ebb4c359f2
Bastien Montagne requested review from Brecht Van Lommel 2024-06-24 12:52:22 +02:00
Author
Owner

@blender-bot build

@blender-bot build
Bastien Montagne added 2 commits 2024-06-24 14:22:10 +02:00
Iliya Katushenock reviewed 2024-06-24 14:26:08 +02:00
@ -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
Author
Owner

@mod_moder and? What is not capitalized here?

@mod_moder and? What is not capitalized here?

I means MAKE_LOCAL -> MakeLocal

I means `MAKE_LOCAL` -> `MakeLocal`
Author
Owner

@mod_moder no, here capitalized means 'IN ALL CAPS' (just like 99% of our current enum values btw...)

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

Yeah, all new code (and already cleanuped) use this style, for instance `AttrDomain::Point`.
Author
Owner

MakeLocal would be called (upper) camel case, btw

`MakeLocal` would be called (upper) camel case, btw
Author
Owner

As discussed in the chat, out current guideline is (and has been for years already) 'in all capitals'.

As discussed in the chat, out current guideline is (and has been for years already) 'in all capitals'.
mont29 marked this conversation as resolved
Brecht Van Lommel approved these changes 2024-06-24 14:45:13 +02:00
Brecht Van Lommel left a comment
Owner

I pointed out two weaknesses in the implementation, but up to you as maintainer if you want to consider those important enough to address.

I pointed out two weaknesses in the implementation, but up to you as maintainer if you want to consider those important enough to address.
Bastien Montagne added 1 commit 2024-06-24 15:37:09 +02:00
Bastien Montagne added 1 commit 2024-06-28 13:57:09 +02:00
Merge branch 'main' into partial-write-refactor
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
73420a4aa2
Author
Owner

@JacquesLucke do you want to do more review on this one?

@JacquesLucke do you want to do more review on this one?
Author
Owner

@blender-bot build

@blender-bot build
Member

I'll do another review pass until monday, thanks for asking.

I'll do another review pass until monday, thanks for asking.
Jacques Lucke reviewed 2024-06-30 12:13:22 +02:00
@ -145,0 +295,4 @@
* Options passed to the #id_add method.
*/
struct IDAddOptions {
IDAddOperations operations;
Member

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.

bool make_local;
bool set_fake_user;
DependencyHandling dependencies; /* Enum with `CLEAR` and `ADD`. */
bool duplicate_dependencies;

In my experience that simplifies debugging and may also enforce certain invariants by design (e.g. that CLEAR_DEPENDENCIES and ADD_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 wonder if the data model (and relations between flags) can be made more explicit here by not using a single bit-flag for everything. ``` bool make_local; bool set_fake_user; DependencyHandling dependencies; /* Enum with `CLEAR` and `ADD`. */ bool duplicate_dependencies; ``` In my experience that simplifies debugging and may also enforce certain invariants by design (e.g. that `CLEAR_DEPENDENCIES` and `ADD_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.
Author
Owner

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.

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
Member

typo (calling)

typo (`calling`)
mont29 marked this conversation as resolved
@ -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
Member

typo (pinter)

typo (`pinter`)
mont29 marked this conversation as resolved
@ -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
Member

typo (is -> if)

typo (`is -> if`)
mont29 marked this conversation as resolved
@ -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);
Member

While looking for remaining uses of BKE_blendfile_write_partial_begin, I noticed that BKE_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.

While looking for remaining uses of `BKE_blendfile_write_partial_begin`, I noticed that `BKE_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.
Author
Owner

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.

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.
mont29 marked this conversation as resolved
@ -149,0 +130,4 @@
scene_name,
nullptr,
{blendfile::PartialWriteContext::IDAddOperations::SET_FAKE_USER}));
id_fake_user_set(&scene_dst->id);
Member

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.

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.
Author
Owner

Indeed, these are leftovers from previous versions of this patch, will update the code.

Indeed, these are leftovers from previous versions of this patch, will update the code.
mont29 marked this conversation as resolved
@ -187,0 +193,4 @@
return IDWALK_RET_NOP;
}
/* The Action ID of the destination scene has already been added (created actually) in the copy
Member

Why has this been added already? What is special about the Action ID?

Why has this been added already? What is special about the Action ID?
Author
Owner

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 to sequencer_copy_animation() another few lines above.

Will update the comment to make it more clear.

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 to `sequencer_copy_animation()` another few lines above. Will update the comment to make it more clear.
mont29 marked this conversation as resolved
@ -187,0 +223,4 @@
};
id_dst = copy_buffer.id_add(
id_src,
{blendfile::PartialWriteContext::IDAddOperations::CLEAR_DEPENDENCIES},
Member

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?

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?
Author
Owner

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

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'.
mont29 marked this conversation as resolved
Bastien Montagne added 1 commit 2024-06-30 14:42:53 +02:00
Bastien Montagne added 1 commit 2024-06-30 14:43:56 +02:00
Updates from review.
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
9f3b092eae
Author
Owner

@blender-bot build

@blender-bot build
Jacques Lucke approved these changes 2024-07-01 08:36:25 +02:00
Author
Owner

Merged through commits from !123993

Merged through commits from !123993
Bastien Montagne closed this pull request 2024-07-01 15:32:23 +02:00
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.

Pull request closed

Sign in to join this conversation.
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
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#122118
No description provided.