Sequencer file based copy paste #114703

Merged
Sebastian Parborg merged 18 commits from ZedDB/blender:copy_paste into main 2023-12-07 15:40:01 +01:00

This is a smaller rewrite/refactor of the VSE copy paste code to use the file copy buffer logic that is used in other places of Blender.

This makes Blender able to copy paste between Blender processes.
It can also paste successfully after closing and reopening Blender.

Other than that, the functionally should remain the exact same as the current copy paste operator with one exception: Scene strips does not retain their scene pointer when pasting into the same file.

This is to make it consistent with how it was before when copy pasting between .blend files.
(The scene data for the scene strips were never copied in when doing that)
Logic for pulling in or linking scenes into new or current files are purposely left for later when a proper proposal of how this would work in a nice fashion is done.

Now the strip data gets copied either fully or partially besides for scene strips. There only the script data gets copied and not the scene data.
If there is a audio/video data block of the same name as in the paste data, it will be reused to reduce potential duplication of data.

This is a smaller rewrite/refactor of the VSE copy paste code to use the file copy buffer logic that is used in other places of Blender. This makes Blender able to copy paste between Blender processes. It can also paste successfully after closing and reopening Blender. Other than that, the functionally should remain the exact same as the current copy paste operator with one exception: Scene strips does not retain their scene pointer when pasting into the same file. This is to make it consistent with how it was before when copy pasting between .blend files. (The scene data for the scene strips were never copied in when doing that) Logic for pulling in or linking scenes into new or current files are purposely left for later when a proper proposal of how this would work in a nice fashion is done. Now the strip data gets copied either fully or partially besides for scene strips. There only the script data gets copied and not the scene data. If there is a audio/video data block of the same name as in the paste data, it will be reused to reduce potential duplication of data.
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR114703) when ready.
Sebastian Parborg force-pushed copy_paste from 067d53e50c to 204c7e7e5b 2023-11-10 18:56:12 +01:00 Compare
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR114703) when ready.
Sebastian Parborg requested review from Sergey Sharybin 2023-11-10 19:22:18 +01:00
Sebastian Parborg requested review from Bastien Montagne 2023-11-10 20:13:42 +01:00
Sergey Sharybin reviewed 2023-11-13 08:59:02 +01:00
Sergey Sharybin left a comment
Owner

Thats a very neat feature being cooked here. While it is WIP, I gave it whirl anyway. Some observations:

  • Make sure you use proper clang-format version
  • Even though the work is WIP it would be very useful to have corresponding TODO list, and, more importantly, design around the feature.

There needs to be a design w.r.t how the ID referenced from the strips and drivers are handled.

Prior to the patch a "weak linking" is used: the clipboard tries to match IDs referenced by name. With this patch the ID is brought into the edit file every time you paste. While it might be OK if that is what happens first time you paste a scene strip and the scene is not in your edit file, it is quite bad that copy-pasting scene strips within one file creates new scenes every time you paste.

Perhaps mechanism of placeholder IDs can be used here. This is how saving file with missing references survives saving and opening later when referenced files are brought to their intended state.

If we want some smart things like "bring scene in unless its there already" then placeholders do not really help that much. Not sure if that is important, it has implementation complexity, performance impact on copy (entire scene needs to be written), and it is not how the current clipboard behaves (point being: behavior of weak linking was not known to me as the biggest problem with the clipboard).

On a code side I'd keep the clipboard implementation in its own file. There is no need to add all those tricky partial blend file and ID manipulation in a generic file.

Thats a very neat feature being cooked here. While it is WIP, I gave it whirl anyway. Some observations: - Make sure you use proper clang-format version - Even though the work is WIP it would be very useful to have corresponding TODO list, and, more importantly, design around the feature. There needs to be a design w.r.t how the ID referenced from the strips and drivers are handled. Prior to the patch a "weak linking" is used: the clipboard tries to match IDs referenced by name. With this patch the ID is brought into the edit file every time you paste. While it might be OK if that is what happens first time you paste a scene strip and the scene is not in your edit file, it is quite bad that copy-pasting scene strips within one file creates new scenes every time you paste. Perhaps mechanism of placeholder IDs can be used here. This is how saving file with missing references survives saving and opening later when referenced files are brought to their intended state. If we want some smart things like "bring scene in unless its there already" then placeholders do not really help that much. Not sure if that is important, it has implementation complexity, performance impact on copy (entire scene needs to be written), and it is not how the current clipboard behaves (point being: behavior of weak linking was not known to me as the biggest problem with the clipboard). On a code side I'd keep the clipboard implementation in its own file. There is no need to add all those tricky partial blend file and ID manipulation in a generic file.
@ -1466,2 +1471,3 @@
if (SEQ_time_right_handle_frame_get(scene, seq) == split_frame &&
seq->machine == split_channel) {
seq->machine == split_channel)
{

While this is seemingly more proper formatting according to our .clang-format, this is not how the clang-format we use formats this specific code. Make sure you use clang-format which comes with Blender libraries, or a matching version.

While this is seemingly more proper formatting according to our .clang-format, this is not how the clang-format we use formats this specific code. Make sure you use clang-format which comes with Blender libraries, or a matching version.

There needs to be a design w.r.t how the ID referenced from the strips and drivers are handled.

Agreed.

Prior to the patch a "weak linking" is used: the clipboard tries to match IDs referenced by name. With this patch the ID is brought into the edit file every time you paste. While it might be OK if that is what happens first time you paste a scene strip and the scene is not in your edit file, it is quite bad that copy-pasting scene strips within one file creates new scenes every time you paste.

Perhaps mechanism of placeholder IDs can be used here. This is how saving file with missing references survives saving and opening later when referenced files are brought to their intended state.

If we want some smart things like "bring scene in unless its there already" then placeholders do not really help that much. Not sure if that is important, it has implementation complexity, performance impact on copy (entire scene needs to be written), and it is not how the current clipboard behaves (point being: behavior of weak linking was not known to me as the biggest problem with the clipboard).

For context, that behavior is already there when copy-pasting e.g. an object into the same file, you'll get duplicates of all of the other IDs (obdata, material...) as well.

This behavior is indeed disappointing at best, but not new regarding ID copy-paste. While in theory the append-or-reuse code could be used here, in practice it would need more work, since the buffer file should not be used as actual source of the appended IDs, but rather the file they have been copied (placed into buffer) from, Then we could get smart deduplication when re-pasting into same blend file.

Would rather consider this as a separate project/design though. Drafts could also in theory help on this topic, but we need to have them first before even considering that option.

On a code side I'd keep the clipboard implementation in its own file. There is no need to add all those tricky partial blend file and ID manipulation in a generic file.

Agreed.

> There needs to be a design w.r.t how the ID referenced from the strips and drivers are handled. Agreed. > Prior to the patch a "weak linking" is used: the clipboard tries to match IDs referenced by name. With this patch the ID is brought into the edit file every time you paste. While it might be OK if that is what happens first time you paste a scene strip and the scene is not in your edit file, it is quite bad that copy-pasting scene strips within one file creates new scenes every time you paste. > > Perhaps mechanism of placeholder IDs can be used here. This is how saving file with missing references survives saving and opening later when referenced files are brought to their intended state. > > If we want some smart things like "bring scene in unless its there already" then placeholders do not really help that much. Not sure if that is important, it has implementation complexity, performance impact on copy (entire scene needs to be written), and it is not how the current clipboard behaves (point being: behavior of weak linking was not known to me as the biggest problem with the clipboard). For context, that behavior is already there when copy-pasting e.g. an object into the same file, you'll get duplicates of all of the other IDs (obdata, material...) as well. This behavior is indeed disappointing at best, but not new regarding ID copy-paste. While in theory the append-or-reuse code could be used here, in practice it would need more work, since the buffer file should not be used as actual source of the appended IDs, but rather the file they have been copied (placed into buffer) from, Then we could get smart deduplication when re-pasting into same blend file. Would rather consider this as a separate project/design though. Drafts could also in theory help on this topic, but we need to have them first before even considering that option. > On a code side I'd keep the clipboard implementation in its own file. There is no need to add all those tricky partial blend file and ID manipulation in a generic file. Agreed.
Sebastian Parborg force-pushed copy_paste from 204c7e7e5b to 5480812338 2023-11-13 18:12:13 +01:00 Compare
Sebastian Parborg changed title from WIP: Sequencer file based copy paste to Sequencer file based copy paste 2023-11-16 18:06:47 +01:00
Author
Member

I've updated the code and the PR description.

After talking with Fransesco it was decided to simply null the scene strip pointers to mirror the current behavior when pasting between .blend files.

The functionality should now almost be exactly the same as it was before.

I've updated the code and the PR description. After talking with Fransesco it was decided to simply null the scene strip pointers to mirror the current behavior when pasting between .blend files. The functionality should now almost be exactly the same as it was before.
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR114703) when ready.

From a functional design point of view:

  • image, video, color, text strip -> full strip copy
  • audio and movieclip strip -> strip + referenced datablock (in case it does not exist, otherwise use the existing one)
  • scene strip -> scene without reference

All relative paths are remapped.

From a functional design point of view: - image, video, color, text strip -> full strip copy - audio and movieclip strip -> strip + referenced datablock (in case it does not exist, otherwise use the existing one) - scene strip -> scene without reference All relative paths are remapped.
Sergey Sharybin requested changes 2023-11-20 12:28:03 +01:00
Sergey Sharybin left a comment
Owner

Did a pass of testing and a quick pass on code review. I did not go into verifying it line-by-line. There are some simple naming improvements possible.

Not sure if that is expected/desired side-effect, but the relative paths are now remapped. Which, I think it is fine.

Did a pass of testing and a quick pass on code review. I did not go into verifying it line-by-line. There are some simple naming improvements possible. Not sure if that is expected/desired side-effect, but the relative paths are now remapped. Which, I think it is fine.
@ -10,183 +10,377 @@
#include <cstring>
#include "BKE_lib_query.h"

Should be grouped with the other BKE headers.

Should be grouped with the other BKE headers.
ZedDB marked this conversation as resolved
@ -23,0 +32,4 @@
#include "BKE_appdir.h"
#include "BKE_blender_copybuffer.h"
#include "BKE_blendfile.h"
#include "BKE_context.h"

BKE_context.hh

`BKE_context.hh`
ZedDB marked this conversation as resolved
@ -52,3 +66,1 @@
void seq_clipboard_pointers_free(ListBase *seqbase);
void SEQ_clipboard_free()
static void null_scene_strips(Sequence *seq)

set_strip_scene_to_null_recursive.

`set_strip_scene_to_null_recursive`.
ZedDB marked this conversation as resolved
@ -192,0 +293,4 @@
}
}
int stripts_to_paste = 0;

num_stripts_to_paste

`num_stripts_to_paste `
ZedDB marked this conversation as resolved
@ -192,0 +305,4 @@
}
Main *bmain = CTX_data_main(C);
std::pair<Main *, Main *> pair_main = std::pair(bmain, temp_bmain);

std::pair<Main *, Main *> pair_main(bmain, temp_bmain);

But I'd also suggest having a dedicated struct for this, as it is more future proof, more semantic, and less of an ugly cast.

`std::pair<Main *, Main *> pair_main(bmain, temp_bmain);` But I'd also suggest having a dedicated struct for this, as it is more future proof, more semantic, and less of an ugly cast.
ZedDB marked this conversation as resolved
Sergey Sharybin requested review from Richard Antalik 2023-11-20 12:28:18 +01:00
Sebastian Parborg force-pushed copy_paste from 68b686f0e0 to 910f3ac379 2023-11-20 14:39:46 +01:00 Compare
Author
Member

Addressed all the review comments.
I had to rebase as BKE_context.hh was a recent change that wasn't in my branch.

Not sure if that is expected/desired side-effect, but the relative paths are now remapped. Which, I think it is fine.

This is intentional. I forgot to mention this as this is a side effect of moving over to using the file based copy paste code, sorry.

Addressed all the review comments. I had to rebase as `BKE_context.hh` was a recent change that wasn't in my branch. > Not sure if that is expected/desired side-effect, but the relative paths are now remapped. Which, I think it is fine. This is intentional. I forgot to mention this as this is a side effect of moving over to using the file based copy paste code, sorry.
Sergey Sharybin reviewed 2023-11-20 15:08:38 +01:00
@ -164,0 +206,4 @@
/* Paste Operator Helper functions
*/
struct Main_pair {

ReuseIDsData as it is used as a data container for the paste_strips_data_ids_reuse_or_add , or MainPair to comply with the code style guide (CamelCase for type names).

Wiki is more down than up, s can't link the page at this time.

`ReuseIDsData` as it is used as a data container for the `paste_strips_data_ids_reuse_or_add `, or `MainPair` to comply with the code style guide (CamelCase for type names). Wiki is more down than up, s can't link the page at this time.
ZedDB marked this conversation as resolved
Sergey Sharybin approved these changes 2023-11-20 16:01:39 +01:00
Sergey Sharybin left a comment
Owner

Form my side I think it is all fine.

Not sure if Bastien or Richard want to have a look before landing. So make sure to communicate with them.

Form my side I think it is all fine. Not sure if Bastien or Richard want to have a look before landing. So make sure to communicate with them.
Bastien Montagne requested changes 2023-11-20 19:22:02 +01:00
Bastien Montagne left a comment
Owner

General logic and code organization looks fine. I have three main issues with current patch though:

Naming

It can still be improved quite a lot imho, see comments below.

In general, would be good to have more coherence in the way things are named, did not comment much in the Paste area, but it seems even worse than in the Copy one. Would recommend using the same logic in both cases, with as systematic as possible usage of the _src and _dst postfixes.

ID Refcounting

That one seems totally off to me.

Copy:

One cannot mix refcounting and non-refcounting behavior in the same IDs! Either you use normal, complete refcounting everywhere (which I would recommend for simplicity), or you use LIB_ID_CREATE_NO_USER_REFCOUNT for all the temp data, not only for the duplicated VSE strips.

Paste:

There are some very weird things going on with ID refcounting in this code, needs to be triple-checked and commented appropriately (or fixed!).

Paste de-duplication detection and remapping.

Think these two steps (detection, and actual remapping) should be properly split. Further more, detection should be recursive (even though it's likely not a huge practical issue currently, the IDs used by the strips may themselves use other IDs, and so on).

  • Detection (paste_strips_data_ids_reuse_or_add):
    • Only detect things, do not change anything.
    • Use 'IDWALK_RECURSE' recursive behavior of foreachid code.
    • Store found matching pairs (could be even directly an IDRemapper data) and 'to be duplicated' IDs as part of the suggested SEQPasteData structure.
  • Then do a single remapping call to BKE_libblock_remap_multiple, and copy over all necessary unmatched IDs.
General logic and code organization looks fine. I have three main issues with current patch though: ### Naming It can still be improved quite a lot imho, see comments below. In general, would be good to have more coherence in the way things are named, did not comment much in the Paste area, but it seems even worse than in the Copy one. Would recommend using the same logic in both cases, with as systematic as possible usage of the `_src` and `_dst` postfixes. ### ID Refcounting That one seems totally off to me. #### Copy: One cannot mix refcounting and non-refcounting behavior in the same IDs! Either you use normal, complete refcounting everywhere (which I would recommend for simplicity), or you use `LIB_ID_CREATE_NO_USER_REFCOUNT` for all the temp data, not only for the duplicated VSE strips. #### Paste: There are some very weird things going on with ID refcounting in this code, needs to be triple-checked and commented appropriately (or fixed!). ### Paste de-duplication detection and remapping. Think these two steps (detection, and actual remapping) should be properly split. Further more, detection should be recursive (even though it's likely not a huge practical issue currently, the IDs used by the strips may themselves use other IDs, and so on). * Detection (`paste_strips_data_ids_reuse_or_add`): * Only detect things, do not change anything. * Use 'IDWALK_RECURSE' recursive behavior of foreachid code. * Store found matching pairs (could be even directly an `IDRemapper` data) and 'to be duplicated' IDs as part of the suggested `SEQPasteData` structure. * Then do a single remapping call to `BKE_libblock_remap_multiple`, and copy over all necessary unmatched IDs.
@ -57,3 +77,2 @@
LISTBASE_FOREACH_MUTABLE (Sequence *, seq, &seqbase_clipboard) {
seq_free_sequence_recurse(nullptr, seq, false);
static void sequencer_copy_animation_listbase(Scene *scene,

again, parameter names consistency (scene -> scene_src etc., see below).

again, parameter names consistency (`scene` -> `scene_src` etc., see below).
Author
Member

Does it really make sense to rename variables in break out functions like these?

To me naming something X_src implies that there will be a X_dst in the variable scope.
I can see how it might fit into the grander scheme of things. But locally I don't it it makes too much sense.

After I renamed the variables in both sequencer_copy_animation and sequencer_copy_animation_listbase I felt that it was just more confusing as there were no equvivalent mirror _src/_dst varaible.

Perhaps just dropping the suffix on the variables for these functions would be a better solution?

Does it really make sense to rename variables in break out functions like these? To me naming something `X_src` implies that there will be a `X_dst` in the variable scope. I can see how it might fit into the grander scheme of things. But locally I don't it it makes too much sense. After I renamed the variables in both `sequencer_copy_animation` and `sequencer_copy_animation_listbase` I felt that it was just more confusing as there were no equvivalent mirror `_src/_dst` varaible. Perhaps just dropping the suffix on the variables for these functions would be a better solution?
ZedDB marked this conversation as resolved
@ -76,0 +103,4 @@
static void sequencer_copy_animation(Scene *scene_src,
ListBase *copied_fcurves,
ListBase *copied_drivers,
Sequence *seq)

copied_fcurves -> fcurves_dst
copied_drivers -> drivers_dst
seq -> seq_dst

`copied_fcurves` -> `fcurves_dst` `copied_drivers` -> `drivers_dst` `seq` -> `seq_dst`
ZedDB marked this conversation as resolved
@ -129,0 +141,4 @@
/* Save current frame and active strip. */
scene_dst->r.cfra = scene_src->r.cfra;
Sequence *prev_active_seq = SEQ_select_active_get(scene_src);

prev_active_seq -> active_seq_src

`prev_active_seq` -> `active_seq_src`
ZedDB marked this conversation as resolved
@ -129,0 +143,4 @@
scene_dst->r.cfra = scene_src->r.cfra;
Sequence *prev_active_seq = SEQ_select_active_get(scene_src);
if (prev_active_seq) {
LISTBASE_FOREACH (Sequence *, seq, &scene_dst->ed->seqbase) {

seq -> seq_dst

`seq` -> `seq_dst`
ZedDB marked this conversation as resolved
@ -134,1 +153,3 @@
ID_PT = static_cast<ID *>(id_restore);
ListBase fcurves_dst = {nullptr, nullptr};
ListBase drivers_dst = {nullptr, nullptr};
LISTBASE_FOREACH (Sequence *, seq, &scene_dst->ed->seqbase) {

seq -> seq_dst

`seq` -> `seq_dst`
ZedDB marked this conversation as resolved
@ -135,0 +160,4 @@
/* Copy animation curves from seq (if any). */
sequencer_copy_animation(scene_src, &fcurves_dst, &drivers_dst, seq);
}
if (BLI_listbase_count(&fcurves_dst) != 0 || BLI_listbase_count(&drivers_dst) != 0) {

use BLI_listbase_is_empty instead.

use `BLI_listbase_is_empty` instead.
ZedDB marked this conversation as resolved
@ -135,0 +161,4 @@
sequencer_copy_animation(scene_src, &fcurves_dst, &drivers_dst, seq);
}
if (BLI_listbase_count(&fcurves_dst) != 0 || BLI_listbase_count(&drivers_dst) != 0) {
bAction *act = ED_id_action_ensure(bmain_src, &scene_dst->id);

act -> act_dst

`act` -> `act_dst`
ZedDB marked this conversation as resolved
@ -136,0 +173,4 @@
/* Create the copy/paste temp file */
bool retval = BKE_copybuffer_copy_end(bmain_src, filepath, reports);
/* Cleanup the dummy scene file */
BKE_id_delete(bmain_src, scene_dst);

I don't think this is actually working as expected? Since above you duplicate the strips with LIB_ID_CREATE_NO_USER_REFCOUNT, think doing regular ID deletion here will wrongly give one less users to data-blocks referenced from strips (audio, images, etc.)?

I don't think this is actually working as expected? Since above you duplicate the strips with `LIB_ID_CREATE_NO_USER_REFCOUNT`, think doing regular ID deletion here will wrongly give one less users to data-blocks referenced from strips (audio, images, etc.)?
Author
Member

Right you are!
I could have sworn I tested this, but I just now noticed that the outliner id counts do not update unless make it redraw. So I guess I must have tested it, not noticed that the id count in the outliner wasn't up to date and thought it was ok.

I guess I just drop LIB_ID_CREATE_NO_USER_REFCOUNT as we clean up the scene with BKE_id_delete afterwards?

Right you are! I could have sworn I tested this, but I just now noticed that the outliner id counts do not update unless make it redraw. So I guess I must have tested it, not noticed that the id count in the outliner wasn't up to date and thought it was ok. I guess I just drop `LIB_ID_CREATE_NO_USER_REFCOUNT` as we clean up the scene with `BKE_id_delete` afterwards?
ZedDB marked this conversation as resolved
@ -136,0 +174,4 @@
bool retval = BKE_copybuffer_copy_end(bmain_src, filepath, reports);
/* Cleanup the dummy scene file */
BKE_id_delete(bmain_src, scene_dst);

you are also missing deletion of the Action ID potentially created above byt the call to ED_id_action_ensure.

you are also missing deletion of the `Action` ID potentially created above byt the call to `ED_id_action_ensure`.
Author
Member

This isn't cleaned up by BKE_id_delete?

I got the impression that ED_id_action_ensure would properly tie the action to the scene, so when the scene is deleted, it is cleaned up as well.

But perhaps I got this wrong?

This isn't cleaned up by `BKE_id_delete`? I got the impression that `ED_id_action_ensure` would properly tie the action to the scene, so when the scene is deleted, it is cleaned up as well. But perhaps I got this wrong?
ZedDB marked this conversation as resolved
@ -164,0 +206,4 @@
/* Paste Operator Helper functions
*/
struct MainPair {

I would much rather give some actual semantic to this struct. Like naming it SEQPasteData. And using same names for the two Mains as everywhere else in the paste code.

I would much rather give some actual semantic to this struct. Like naming it `SEQPasteData`. And using same names for the two Mains as everywhere else in the paste code.
Author
Member

I think renaming the struct would make it harder to know what it contains.

To me SEQPasteData would indicate that this would be some non trivial data structure that isn't a standard type in the CPP std library (it was previously just a std::pair), but was changed because of the feedback here.

I think renaming the struct would make it harder to know what it contains. To me `SEQPasteData` would indicate that this would be some non trivial data structure that isn't a standard type in the CPP std library (it was previously just a `std::pair`), but was changed because of the feedback here.
ZedDB marked this conversation as resolved
@ -168,0 +220,4 @@
MainPair *pair_main = static_cast<MainPair *>(cb_data->user_data);
Main *bmain = pair_main->first;
Main *temp_bmain = pair_main->second;
ID *id_p = *cb_data->id_pointer;

id_p -> id

`id_p` -> `id`
ZedDB marked this conversation as resolved
@ -168,0 +222,4 @@
Main *temp_bmain = pair_main->second;
ID *id_p = *cb_data->id_pointer;
if (id_p && cb_data->cb_flag & IDWALK_CB_USER) {

Skipping if cb_data->cb_flag & IDWALK_CB_USER is false is very dangerous here, I cannot see any reason for this check to be needed - and it will break things if there ever are non-refcounting usages of IDs in VSE strips.

Skipping if `cb_data->cb_flag & IDWALK_CB_USER` is false is _very_ dangerous here, I cannot see any reason for this check to be needed - and it will break things if there ever are non-refcounting usages of IDs in VSE strips.
ZedDB marked this conversation as resolved
@ -168,0 +223,4 @@
ID *id_p = *cb_data->id_pointer;
if (id_p && cb_data->cb_flag & IDWALK_CB_USER) {
ID_Type type = GS((id_p)->name);

type -> id_type

`type` -> `id_type`
ZedDB marked this conversation as resolved
@ -168,0 +230,4 @@
}
ListBase *lb = which_libbase(bmain, type);
ID *id_local = static_cast<ID *>(BLI_findstring(lb, (id_p)->name + 2, offsetof(ID, name) + 2));

This is too naive, will break on linked data case. You also need to check on the lib pointer, and if both non-NULL, compare their filepath

This is too naive, will break on linked data case. You also need to check on the `lib` pointer, and if both non-NULL, compare their filepath
ZedDB marked this conversation as resolved
@ -168,0 +236,4 @@
/* A data block with the same name already exists.
* Don't copy over any new datablocks, reuse the data block that exists.
*/
id_us_min(id_local);

Why are you touching to local's usercount here at all? And even worse, decreasing it?

_Why_ are you touching to local's usercount here at all? And even worse, decreasing it?
ZedDB marked this conversation as resolved
@ -168,0 +237,4 @@
* Don't copy over any new datablocks, reuse the data block that exists.
*/
id_us_min(id_local);
BKE_libblock_remap(temp_bmain, id_p, id_local, ID_REMAP_SKIP_INDIRECT_USAGE);

You should never call the remap ID functions while you are already iterating over ID usages... this is a very dangerous game that may bite you back in... unexpected ways.

You should never call the remap ID functions while you are already iterating over ID usages... this is a very dangerous game that may bite you back in... unexpected ways.
ZedDB marked this conversation as resolved
@ -168,0 +241,4 @@
}
else {
/* No data block of the same name already exists, transfer it over from temp_bmain. */
id_us_min(id_p);

Why do you need to decrease its usercount here?

Why do you need to decrease its usercount here?
ZedDB marked this conversation as resolved
@ -186,2 +274,4 @@
BLI_addtail(&scene->adt->drivers, BKE_fcurve_copy(fcu));
}
}
int SEQ_clipboard_paste_exec(bContext *C, wmOperator *op)

needs empty line inbetween functions

needs empty line inbetween functions
ZedDB marked this conversation as resolved
@ -192,0 +298,4 @@
}
int num_strips_to_paste = 0;
if (paste_scene && paste_scene->ed) {

This check can be done before declaring num_strips_to_paste, and inverted, to get an early out with a more appropriate message (since 'no scene to paste VSE data from' is not exactly same as `no strips to paste from clipboard Scene').

it also mean that you can then do directly
const int num_strips_to_paste = BLI_listbase_count(&paste_scene->ed->seqbase);

This check can be done before declaring `num_strips_to_paste`, and inverted, to get an early out with a more appropriate message (since 'no scene to paste VSE data from' is not exactly same as `no strips to paste from clipboard Scene'). it also mean that you can then do directly `const int num_strips_to_paste = BLI_listbase_count(&paste_scene->ed->seqbase);`
ZedDB marked this conversation as resolved
Richard Antalik requested changes 2023-11-21 05:12:24 +01:00
Richard Antalik left a comment
Member

Overall the code looks OK, but you are adding operator code to sequencer core - source/blender/sequencer. This code is for low level data manipulation (equivalent of BKE), as such it must not need editors/include.

There is virtually no low level VSE data manipulation here, so the code could be in sequencer_edit.cc or own file in space_sequencer folder.

Overall the code looks OK, but you are adding operator code to sequencer core - `source/blender/sequencer`. This code is for low level data manipulation (equivalent of BKE), as such it must not need `editors/include`. There is virtually no low level VSE data manipulation here, so the code could be in `sequencer_edit.cc` or own file in `space_sequencer` folder.
Sergey Sharybin added this to the Video Sequencer project 2023-11-22 11:55:41 +01:00

It is a lot of low-level manipulations going on.

Not immediately clear why outliner header is needed. For the rest I think passing ed would avoid most of the sequencer editor includes?

If the editors include is no avoidable, then is better to have dedicated file in the editors/sequencer. We shouldn't be making files with that many thousand lines of code.

It is a lot of low-level manipulations going on. Not immediately clear why outliner header is needed. For the rest I think passing `ed` would avoid most of the sequencer editor includes? If the editors include is no avoidable, then is better to have dedicated file in the editors/sequencer. We shouldn't be making files with that many thousand lines of code.
Author
Member

I've only addressed Bastien's feedback so far as I want to get that part right before I start moving things around again.

Most of the editor includes are because of ED_ functions like ED_id_action_ensure etc.

I think it might be easier to just move this into a dedicated clipboard file in editors/sequencer as you guys suggested. I'll do that after I've gotten a green light from Bastien.

I've only addressed Bastien's feedback so far as I want to get that part right before I start moving things around again. Most of the editor includes are because of `ED_` functions like `ED_id_action_ensure` etc. I think it might be easier to just move this into a dedicated clipboard file in `editors/sequencer` as you guys suggested. I'll do that after I've gotten a green light from Bastien.
Author
Member

@blender-bot build

@blender-bot build
Bastien Montagne requested changes 2023-11-27 14:19:10 +01:00
Bastien Montagne left a comment
Owner

We are getting there, but think this still needs a bit more work. Mainly:

  • In pasting code, checking logic for existing IDs to remap to is not fully correct yet.
  • In copy code, only scenes used by Scene strips are excluded, think there are other cases (IDProp, drivers...) that can also pull in unwanted data. Should be handled in a more generic way, similar to how remapping vs. copying is done in the pasting code.
We are getting there, but think this still needs a bit more work. Mainly: - In pasting code, checking logic for existing IDs to remap to is not fully correct yet. - In copy code, only scenes used by Scene strips are excluded, think there are other cases (IDProp, drivers...) that can also pull in unwanted data. Should be handled in a more generic way, similar to how remapping vs. copying is done in the pasting code.
@ -57,0 +71,4 @@
}
}
else if (seq->type == SEQ_TYPE_SCENE) {
seq->scene = nullptr;

Scene usage from VSE strips is not refcounting, there should be a comment about it (since it explains why there is no need to decrease scene's usercount here).

Scene usage from VSE strips is not refcounting, there should be a comment about it (since it explains why there is no need to decrease scene's usercount here).
ZedDB marked this conversation as resolved
@ -60,0 +78,4 @@
static void sequencer_copy_animation_listbase(Scene *scene_src,
Sequence *seq_dst,
ListBase *clipboard_dst,
ListBase *fcurve_base)

Would also call this fcurve_base_src... same in other functions of course.

Would also call this `fcurve_base_src`... same in other functions of course.
Author
Member

This is the only function that this name is used? There are no other places to rename this variable in.

This is the only function that this name is used? There are no other places to rename this variable in.
ZedDB marked this conversation as resolved
@ -67,3 +94,2 @@
LISTBASE_FOREACH_MUTABLE (FCurve *, fcu, &drivers_clipboard) {
BKE_fcurve_free(fcu);
GSET_FOREACH_BEGIN (FCurve *, fcu, fcurves) {

fcu -> fcu_src
fcurves -> fcurves_src

`fcu` -> `fcu_src` `fcurves` -> `fcurves_src`
ZedDB marked this conversation as resolved
@ -129,0 +142,4 @@
scene_dst->r.cfra = scene_src->r.cfra;
Sequence *active_seq_src = SEQ_select_active_get(scene_src);
if (active_seq_src) {
LISTBASE_FOREACH (Sequence *, seq_dst, &scene_dst->ed->seqbase) {

Use BLI_findstring instead.

Use `BLI_findstring` instead.
ZedDB marked this conversation as resolved
@ -135,0 +152,4 @@
ListBase fcurves_dst = {nullptr, nullptr};
ListBase drivers_dst = {nullptr, nullptr};
LISTBASE_FOREACH (Sequence *, seq_dst, &scene_dst->ed->seqbase) {
/* Null and scene pointers in scene strips. We don't want to copy whole scenes.

Null and scene -> Nullify all scene ?

`Null and scene` -> `Nullify all scene` ?

This actually triggers another potential issue here: Other references to scenes or other undesirable ID types (e.g. from IDProperties, or drivers...). Would suspect you'll need some more ID pointer checks. ;)

Similar to pasting code, could be simpler to use the BKE_library_foreach_ID_link on the newly duplicated scene and check for any unwanted ID types (Scene, Objects, etc.?). Store all IDs to be remapped to nullptr in an IDRemapper data, and then call BKE_libblock_relink_multiple on the duplicated temp scene ID.

That way other things like usercount etc. are also handled automatically by the generic API, instead of having to be taken care of manually.

This actually triggers another potential issue here: Other references to scenes or other undesirable ID types (e.g. from IDProperties, or drivers...). Would suspect you'll need some more ID pointer checks. ;) Similar to pasting code, could be simpler to use the `BKE_library_foreach_ID_link` on the newly duplicated scene and check for any unwanted ID types (Scene, Objects, etc.?). Store all IDs to be remapped to `nullptr` in an IDRemapper data, and then call `BKE_libblock_relink_multiple` on the duplicated temp scene ID. That way other things like usercount etc. are also handled automatically by the generic API, instead of having to be taken care of manually.
ZedDB marked this conversation as resolved
@ -176,0 +250,4 @@
/* A data block with the same name already exists.
* Don't copy over any new datablocks, reuse the data block that exists.
*/
if (id_local->lib != nullptr && id->lib != nullptr) {

Logic here is not good enough, if one ID has a lib pointer and the other not, you assume that they can be remapped, which is most likely not the case (although doing a check on lib path vs. main path would also be needed in that case).

Logic here is not good enough, if one ID has a lib pointer and the other not, you assume that they can be remapped, which is most likely not the case (although doing a check on lib path vs. main path would also be needed in that case).
Author
Member

What is the solution?
I understand the issue. But I don't see any concrete solution proposed here?

What is the solution? I understand the issue. But I don't see any concrete solution proposed here?
ZedDB marked this conversation as resolved
@ -176,0 +251,4 @@
* Don't copy over any new datablocks, reuse the data block that exists.
*/
if (id_local->lib != nullptr && id->lib != nullptr) {
/* Check if they are using the same the same filepath. */

Only one the same is enough

Only one `the same` is enough
ZedDB marked this conversation as resolved
@ -192,0 +320,4 @@
}
if (!paste_scene || !paste_scene->ed) {
BKE_report(op->reports, RPT_INFO, "No clipboard scene to paste VSE data from");

This should be at least a RPT_WARNING, or maybe even a RPT_ERROR, since this situation is not expected to happen.

This should be at least a `RPT_WARNING`, or maybe even a `RPT_ERROR`, since this situation is not expected to happen.
ZedDB marked this conversation as resolved
Sebastian Parborg force-pushed copy_paste from bcb52d097e to 9aa05b2c0b 2023-11-29 15:44:28 +01:00 Compare
First-time contributor

Now every strip gets copied either fully or partially besides scene strips.

Scene strips are currently the only strip type with a "missing media" indication, with a red line on top of the strip. So, maybe it could work to copy the Scene strips, since, I guess, they would just get the "missing media" indication?

> Now every strip gets copied either fully or partially besides scene strips. Scene strips are currently the only strip type with a "missing media" indication, with a red line on top of the strip. So, maybe it could work to copy the Scene strips, since, I guess, they would just get the "missing media" indication?
Author
Member

I guess, they would just get the "missing media" indication?

That is how it works with this change.
The strip itself gets copied, but not the scene it refers to.
I see now that it wasn't clear that I was talking about the IDs that the strips were refering to when I wrote: "Now every strip gets copied either fully or partially besides scene strips."

I'll update it to be:

Now the strip data gets copied either fully or partially besides for scene strips. There only the script data gets copied and not the scene data.

>I guess, they would just get the "missing media" indication? > That is how it works with this change. The strip itself gets copied, but not the scene it refers to. I see now that it wasn't clear that I was talking about the IDs that the strips were refering to when I wrote: "Now every strip gets copied either fully or partially besides scene strips." I'll update it to be: > Now the strip data gets copied either fully or partially besides for scene strips. There only the script data gets copied and not the scene data.
Bastien Montagne requested changes 2023-12-04 16:36:23 +01:00
Bastien Montagne left a comment
Owner

Besides minor points, copy code is OK I think now.

Paste however is not, especially I think the handling of Library IDs is still logically wrong (although it likely 'happens to work' in basic tests because Lib IDs are named from their library files, but this is absolutely not a reliable source of data).

I would suggest carefully checking the code of BKE_main_merge, if it cannot be used directly, at least the underlying logic should be reusable as-is. In a nutshell:

  • Library IDs should be handled as other IDs, with special points:
    • They need to be remapped before IDs are moved to the destination Main (so require a dedicated remapper).
    • They need to be compared based on filepath, not ID name.
    • Special care must be taken with source library having same filepath as destination Main.
    • A Library should never be remapped to NULL (this is making an ID local, which would require additional processing, not supported/implemented currently, and should not be needed in current usecase).

Then you only need three type of handling for all IDs in source main:

  • COPY moves the ID from source to destination Main.
  • REMAP remaps usages from an ID in source Main to a matching ID in destination main.
  • IGNORE leaves the ID in source main, and remap its usages to NULL.
Besides minor points, copy code is OK I think now. Paste however is not, especially I think the handling of Library IDs is still logically wrong (although it likely 'happens to work' in basic tests because Lib IDs are named from their library files, but this is absolutely not a reliable source of data). I would suggest carefully checking the code of `BKE_main_merge`, if it cannot be used directly, at least the underlying logic should be reusable as-is. In a nutshell: * Library IDs should be handled as other IDs, with special points: * They need to be remapped _before_ IDs are moved to the destination Main (so require a dedicated remapper). * They need to be compared based on filepath, not ID name. * Special care must be taken with source library having same filepath as destination Main. * A Library should never be remapped to NULL (this is making an ID local, which would require additional processing, not supported/implemented currently, and should not be needed in current usecase). Then you only need three type of handling for all IDs in source main: * `COPY` moves the ID from source to destination Main. * `REMAP` remaps usages from an ID in source Main to a matching ID in destination main. * `IGNORE` leaves the ID in source main, and remap its usages to NULL.
@ -76,12 +76,27 @@ static void copybuffer_append(BlendfileLinkAppendContext *lapp_context,
/* Tag existing IDs in given `bmain_dst` as already existing. */
BKE_main_id_tag_all(bmain, LIB_TAG_PRE_EXISTING, true);
std::string bmain_file_path = bmain->filepath;

the changes in this file are good to go, and I think they are important enough to earn their own dedicated commit in main, separated for the rest of this PR.

the changes in this file are good to go, and I think they are important enough to earn their own dedicated commit in main, separated for the rest of this PR.
ZedDB marked this conversation as resolved
@ -58,2 +71,2 @@
LISTBASE_FOREACH_MUTABLE (Sequence *, seq, &seqbase_clipboard) {
seq_free_sequence_recurse(nullptr, seq, false);
/* We don't care about embedded, loopback, or internal IDs. */
if (cb_data->cb_flag & (IDWALK_CB_EMBEDDED | IDWALK_CB_LOOPBACK | IDWALK_CB_INTERNAL)) {

These actually need different handling imho:

  • IDWALK_CB_EMBEDDED should return IDWALK_RET_NOP, since you still do want to process the embedded ID pointers.
  • IDWALK_CB_LOOPBACK and IDWALK_CB_INTERNAL should return IDWALK_RET_STOP_RECURSION, to avoid going deeper into these (iirc this is implemented behavior for at least IDWALK_CB_LOOPBACK anyway, but still better to be logical and consistent here)
These actually need different handling imho: * `IDWALK_CB_EMBEDDED` should return `IDWALK_RET_NOP`, since you still do want to process the embedded ID pointers. * `IDWALK_CB_LOOPBACK` and `IDWALK_CB_INTERNAL` should return `IDWALK_RET_STOP_RECURSION`, to avoid going deeper into these (iirc this is implemented behavior for at least `IDWALK_CB_LOOPBACK` anyway, but still better to be logical and consistent here)
ZedDB marked this conversation as resolved
@ -101,0 +78,4 @@
/* Nullify everything that is not:
* Sound, Movieclip, Image, Text, Vfont, Action, or Collection IDs.
*/
if (!ELEM(id_type, ID_SO, ID_MC, ID_IM, ID_TXT, ID_VF, ID_AC, ID_GR)) {

Wondering why you are keeping Collection here? This should not be needed (and the embedded one, aka Scene master collection, should already be skipped by the check above)

Wondering why you are keeping Collection here? This should not be needed (and the embedded one, aka Scene master collection, should already be skipped by the check above)
ZedDB marked this conversation as resolved
@ -101,0 +80,4 @@
*/
if (!ELEM(id_type, ID_SO, ID_MC, ID_IM, ID_TXT, ID_VF, ID_AC, ID_GR)) {
BKE_id_remapper_add(id_remapper, id, nullptr);
return IDWALK_RET_NOP;

You should return IDWALK_RET_STOP_RECURSION, as you do not need to recurse into this ID, you already know you are not using it.

You should return `IDWALK_RET_STOP_RECURSION`, as you do not need to recurse into this ID, you already know you are not using it.
ZedDB marked this conversation as resolved
@ -105,0 +136,4 @@
ReportList *reports)
{
Scene *scene_dst = BKE_scene_add(bmain_src, "copybuffer_vse_scene");

Could use a comment stating that ideally this should not be added to the global Main, but that currently there is no good solution to avoid it.

Could use a comment stating that ideally this should not be added to the global Main, but that currently there is no good solution to avoid it.
ZedDB marked this conversation as resolved
@ -105,0 +237,4 @@
std::vector<ID *> ids_to_copy;
};
enum {

Comment needed about the fact that values here are also used as 'priority' levels (if (copy_method_iter > copy_method) check in calling code).

Comment needed about the fact that values here are also used as 'priority' levels (`if (copy_method_iter > copy_method)` check in calling code).
ZedDB marked this conversation as resolved
@ -105,0 +243,4 @@
ID_REMAP,
};
static int should_copy_id(ID *id_dst, ID *id_src, Main *bmain_dst)

Think this is missing proper handling for Libraries IDs themselves.

These you do not want to compare based on their ID names, but based on their absolute filepaths.

Think this is missing proper handling for Libraries IDs themselves. These you do not want to compare based on their ID names, but based on their absolute filepaths.
ZedDB marked this conversation as resolved
@ -105,0 +290,4 @@
/* We don't care about embedded, loopback, or internal IDs. */
if (cb_data->cb_flag & (IDWALK_CB_EMBEDDED | IDWALK_CB_LOOPBACK | IDWALK_CB_INTERNAL)) {
return IDWALK_RET_NOP;

Same remarks as in copy code about return values.

Same remarks as in copy code about return values.
ZedDB marked this conversation as resolved
@ -105,0 +299,4 @@
/* Don't copy in actions here as we already handle these in "sequencer_paste_animation".
* We don't copy Collections (ID_GR) here either as we don't care about scene collections.
*/
return IDWALK_RET_NOP;

Same remarks as in copy code about return values (and need of special handling for collections).

Same remarks as in copy code about return values (and need of special handling for collections).
ZedDB marked this conversation as resolved
@ -117,0 +305,4 @@
ID *id_dst = nullptr;
int copy_method = -1;
ListBase *lb = which_libbase(bmain_dst, id_src_type);

Could use a comment noting that this is known sub-optimal processing, but simpler code and considered 'good enough' in practice for this specific use case?

(It is looping over all destination IDs for each source ID, so this is O(n^2) complexity)

Could use a comment noting that this is known sub-optimal processing, but simpler code and considered 'good enough' in practice for this specific use case? (It is looping over all destination IDs for each source ID, so this is `O(n^2)` complexity)
ZedDB marked this conversation as resolved
@ -117,0 +307,4 @@
ListBase *lb = which_libbase(bmain_dst, id_src_type);
LISTBASE_FOREACH (ID *, id_iter, lb) {
if (STREQ(id_src->name, id_iter->name)) {

this does not works propoerly for Library IDs, these need to be checked based on the absolute filepath, not the ID name.

this does not works propoerly for Library IDs, these need to be checked based on the absolute filepath, not the ID name.
ZedDB marked this conversation as resolved
@ -152,0 +328,4 @@
BKE_id_remapper_add(paste_data->id_remapper, id_src, id_dst);
break;
case ID_LIB_COPY:
paste_data->ids_to_copy.push_back(&id_src->lib->id);

This should only be done in case the id_dst and id_src->lib pointers are not nullptr! Also comment back to the check about nullptr destination ID in should_copy_id.

IMHO it would be much better to introduce a forth copy_method value here (ID_SKIP ?), would make the code overall way more readable.

And............ Isn't this code potentially adding the same Library ID many times to the list of IDs to copy?

This should only be done in case the `id_dst` and `id_src->lib` pointers are not `nullptr`! Also comment back to the check about `nullptr` destination ID in `should_copy_id`. IMHO it would be much better to introduce a forth `copy_method` value here (`ID_SKIP` ?), would make the code overall way more readable. And............ Isn't this code potentially adding the same Library ID many times to the list of IDs to copy?
ZedDB marked this conversation as resolved
@ -192,0 +411,4 @@
IDWALK_RECURSE);
/* Copy over all new ID data, save remapping for after we have moved over all the strips into
* bmain.

This comment should explain why this delayed remapping is needed (think it's roughly the same reason as in BKE_main_merge ?)

This comment should explain why this delayed remapping is needed (think it's roughly the same reason as in `BKE_main_merge` ?)
ZedDB marked this conversation as resolved
@ -192,0 +480,4 @@
}
}
BKE_libblock_remap_multiple(bmain_dst, paste_data.id_remapper, 0);

BKE_libblock_remap_multiple does not process ID.lib pointer by default, you need to give it the specific (shiny brand new) flag ID_REMAP_DO_LIBRARY_POINTERS.

Also, while working on the new BKE_main_merge, I noticed that library pointers should be remapped before moving IDs from source main the destination one, to ensure a proper sorting of IDs in destination Main. So code is using two remappers, one for all general IDs, and one dedicated to libraries.

This also helps you ensuring that you never remap a library to a nullptr.

`BKE_libblock_remap_multiple` does not process `ID.lib` pointer by default, you need to give it the specific (shiny brand new) flag `ID_REMAP_DO_LIBRARY_POINTERS`. Also, while working on the new `BKE_main_merge`, I noticed that library pointers should be remapped _before_ moving IDs from source main the destination one, to ensure a proper sorting of IDs in destination Main. So code is using two remappers, one for all general IDs, and one dedicated to libraries. This also helps you ensuring that you never remap a library to a nullptr.
ZedDB marked this conversation as resolved
Sebastian Parborg force-pushed copy_paste from 6f1cf1ad01 to bee2ea7465 2023-12-06 15:07:07 +01:00 Compare
Sebastian Parborg added 2 commits 2023-12-06 19:29:37 +01:00
Author
Member

I've updated the code to use BKE_main_merge.
It seems to work fine on the first paste into a blank file.

However if you paste a second time it will segfault as it doesn't seem to remap the pasted IDs correctly and thus the pasted strips still point to IDs that are freed in bmain_src that is freed at the end of BKE_main_merge

I tested this with a regular sound strip that doesn't use any linked libary data.

I guess we should figure out where the logic is wrong and also add a test for it?

I've updated the code to use `BKE_main_merge`. It seems to work fine on the first paste into a blank file. However if you paste a second time it will segfault as it doesn't seem to remap the pasted IDs correctly and thus the pasted strips still point to IDs that are freed in `bmain_src` that is freed at the end of `BKE_main_merge` I tested this with a regular sound strip that doesn't use any linked libary data. I guess we should figure out where the logic is wrong and also add a test for it?

Would suspect that the problem is that you are calling BKE_main_merge after moving strips to the destination scene. BKE_main_merge only remaps source IDs to be moved in destination main, but your source scene is already emptied of its strips at this points, so their pointers cannot be remapped.

Would suspect that the problem is that you are calling `BKE_main_merge` _after_ moving strips to the destination scene. `BKE_main_merge` only remaps source IDs to be moved in destination main, but your source scene is already emptied of its strips at this points, so their pointers cannot be remapped.
Sebastian Parborg added 1 commit 2023-12-07 11:33:49 +01:00
Author
Member

DOH!

Right you are! I had not arranged the logic to be in the correct order.
I think it should be correct now. However I'm getting an assert if I try to paste in a sound strip with a linked sound ID into the .blend file that the sound ID links to:

BLI_assert failed: source/blender/blenkernel/intern/main.cc:207, are_ids_from_different_mains_matching(), at '!(strcmp(lib_2->filepath_abs, bmain_2->filepath) == 0)'
DOH! Right you are! I had not arranged the logic to be in the correct order. I think it should be correct now. However I'm getting an assert if I try to paste in a sound strip with a linked sound ID into the .blend file that the sound ID links to: ``` BLI_assert failed: source/blender/blenkernel/intern/main.cc:207, are_ids_from_different_mains_matching(), at '!(strcmp(lib_2->filepath_abs, bmain_2->filepath) == 0)' ```
Sebastian Parborg added 1 commit 2023-12-07 12:17:27 +01:00
Sebastian Parborg added 1 commit 2023-12-07 12:21:25 +01:00
Author
Member

It seems like it is working as intended to me now.

It seems like it is working as intended to me now.
Bastien Montagne requested changes 2023-12-07 12:51:19 +01:00
Bastien Montagne left a comment
Owner

From logical PoV (reading the code ;) ), LGTM now.

Noted some picky minor improvements below.

Would recommend stress-testing this code a bit more though, especially targeting:

  • Linking data cases (trying situations where copied strips would bring several IDs from a same library, data from libraries of libraries, etc.)
  • Copying from full production scene, witch hundreds of other unrelated IDs (to check that the copying does filter them out properly, and that merging the read pastebuffer .blend does not bring in undesired data).
From logical PoV (reading the code ;) ), LGTM now. Noted some picky minor improvements below. Would recommend stress-testing this code a bit more though, especially targeting: - Linking data cases (trying situations where copied strips would bring several IDs from a same library, data from libraries of libraries, etc.) - Copying from full production scene, witch hundreds of other unrelated IDs (to check that the copying does filter them out properly, and that merging the read pastebuffer .blend does not bring in undesired data).
@ -183,0 +244,4 @@
}
Main *bmain = CTX_data_main(C);
Scene *scene = CTX_data_scene(C);

Would rather pass scene and bmain as parameters here, rather than have an obscure util function extract them (again) from the context.

Also:

  • bmain -> bmain_dst
  • scene -> scene_dst
  • act -> action_dst
Would rather pass `scene` and `bmain` as parameters here, rather than have an obscure util function extract them (again) from the context. Also: - `bmain` -> `bmain_dst` - `scene` -> `scene_dst` - `act` -> `action_dst`
ZedDB marked this conversation as resolved
@ -192,0 +329,4 @@
active_seq_name.assign(prev_active_seq->name);
}
/* Make sure we have all data IDs we need in bmain_dst. Remap the IDs if we already have them. */

also add a note that this has to happen before moving strips from source to destination scene.

also add a note that this has to happen before moving strips from source to destination scene.
ZedDB marked this conversation as resolved
@ -192,0 +336,4 @@
BKE_main_merge(bmain_dst, &bmain_src, merge_reports);
/* Paste animation.
* NOTE: Only fcurves are copied. Drivers and NLA action strips are not copied.

Drivers are also duplicated in paste_animation?

Drivers are also duplicated in `paste_animation`?
ZedDB marked this conversation as resolved
@ -192,0 +352,4 @@
scene_src, scene_dst, &nseqbase, &scene_src->ed->seqbase, 0, 0);
/* BKE_main_merge will copy the scene_src and its action into bmain_dst. Remove them as
* we merge the data from these these manually.

these these ;)

`these these` ;)
ZedDB marked this conversation as resolved
Sebastian Parborg added 1 commit 2023-12-07 13:06:09 +01:00
Bastien Montagne approved these changes 2023-12-07 14:33:28 +01:00
Sebastian Parborg force-pushed copy_paste from b9d59a90db to 4c21c64d79 2023-12-07 14:56:49 +01:00 Compare
Author
Member

@blender-bot build

@blender-bot build
Author
Member

@iss I've moved the files to editors as you suggested before.
Do you have any more feedback or does this look good to you as well?

@iss I've moved the files to `editors` as you suggested before. Do you have any more feedback or does this look good to you as well?
Richard Antalik approved these changes 2023-12-07 15:06:45 +01:00
Richard Antalik left a comment
Member

LGTM.

Now there is no point having SEQUENCER_OT_paste in sequencer_edit.cc file, but that is not blocking issue.

LGTM. Now there is no point having `SEQUENCER_OT_paste` in `sequencer_edit.cc` file, but that is not blocking issue.
Sebastian Parborg merged commit e7f0bb24f7 into main 2023-12-07 15:40:01 +01:00
Sebastian Parborg deleted branch copy_paste 2023-12-07 15:40:03 +01:00
Damien Picard reviewed 2023-12-11 14:25:27 +01:00
@ -0,0 +230,4 @@
}
/* We are all done! */
BKE_report(op->reports, RPT_INFO, "Copied the selected VSE strips to internal clipboard");
Member

“VSE” is mentioned nowhere in Blender’s UI, and nowhere in the manual except to describe options from an add-on.

I think it should be called “Sequencer” for consistency with the rest of the UI.

Same on line 294.

“VSE” is mentioned nowhere in Blender’s UI, and nowhere in the manual except to describe options from an add-on. I think it should be called “Sequencer” for consistency with the rest of the UI. Same on line 294.
First-time contributor

(Maybe a follow-up patch could be to add missing-source-indication to all strips needing it?)

(Maybe a follow-up patch could be to add missing-source-indication to all strips needing it?)
Author
Member

@pioverfour should it be "Video Sequencer" as that is what it is in the UI?

@pioverfour should it be "Video Sequencer" as that is what it is in the UI?
Member

@pioverfour should it be "Video Sequencer" as that is what it is in the UI?

Both are fine IMO, but it’s overwhelmingly just “sequencer.”

"Video Sequencer"
"Color space that the sequencer operates in"
"Safe areas used in 3D view and the sequencer"
"Final frame of the mask (used for sequencer)"
"First frame of the mask (used for sequencer)"
"Sequencer Color Space Settings"
"Settings of color space sequencer is working in"
"Sequencer Editors"
"Sequencer"
"Sequencer Preview"
"Render using the sequencer's OpenGL display"
"Sequencer effect type"
"Delete selected strips from the sequencer"
"Add an effect to the sequencer, most are applied on top of existing strips"
"Add an image or image sequence to the sequencer"
"Add a mask strip to the sequencer"
"Put the contents of a meta-strip back in the sequencer"
"Add a movie strip to the sequencer"
"Add a movieclip strip to the sequencer"
"Refresh Sequencer"
"Refresh the sequencer editor"
"Reload strips in the sequencer"
"Add a strip to the sequencer using a Blender scene as a source"
"Add a sound strip to the sequencer"
"Sequencer Swap Data"
"Swap 2 sequencer strips"
"View all the strips in the sequencer"
"Zoom the sequencer on the selected strips"
"Sequencer View Zoom Ratio"
"Change zoom ratio of sequencer preview"
"Sequencer Overlays"
"Sequencer Strips"
"Use metadata from the strips in the sequencer"
"Sequencer Preview Shading"
"Display method used in the sequencer view"
"Process the render (and composited) result through the video sequence editor pipeline, if sequencer strips exist"
"Use workbench render settings from the sequencer scene, instead of each individual scene used in the strip"
"Calculate modifiers in linear space instead of sequencer's space"
"Use the Scene's Sequencer timeline as input"
"Sequencer's active strip"
"Partial overlay on top of the sequencer with a frame offset"
"Use sequencer strip as mask input"
"Display the strip color tags in the sequencer"
"Sequencer Tool Settings"
"Display data belonging to the Video Sequencer"
"View mode to use for displaying sequencer output"
"Type of the Sequencer view (sequencer, preview or both)"
"Sequencer & Preview"
"SequencerCommon"
"Sequencer"
"Sequencer Tool: Tweak"
"Sequencer Tool: Tweak (fallback)"
"Sequencer Tool: Select Box"
"Sequencer Tool: Select Box (fallback)"
"Sequencer Tool: Blade"
"Sequencer Tool: Blade (fallback)"
"SequencerPreview"
"Sequencer Tool: Cursor"
"Sequencer Tool: Cursor (fallback)"
"Sequencer Tool: Move"
"Sequencer Tool: Move (fallback)"
"Sequencer Tool: Rotate"
"Sequencer Tool: Rotate (fallback)"
"Sequencer Tool: Scale"
"Sequencer Tool: Scale (fallback)"
"Sequencer Tool: Sample"
"Sequencer Tool: Sample (fallback)"
"Toggle Sequencer/Preview"
"Add a crossfade transition to the sequencer"
"Add an add effect strip to the sequencer"
"Add a subtract effect strip to the sequencer"
"Add an alpha over effect strip to the sequencer"
"Add an alpha under effect strip to the sequencer"
"Add a gamma cross transition to the sequencer"
"Add a multiply effect strip to the sequencer"
"Add an alpha over drop effect strip to the sequencer"
"Add a wipe transition to the sequencer"
"Add a glow effect strip to the sequencer"
"Add a transform effect strip to the sequencer"
"Add a color strip to the sequencer"
"Add a speed effect strip to the sequencer"
"Add a multicam selector effect strip to the sequencer"
"Add an adjustment layer effect strip to the sequencer"
"Add a gaussian blur effect strip to the sequencer"
"Add a text strip to the sequencer"
"Add a color mix effect strip to the sequencer"
"Border rendering is not supported by sequencer"
"Recursion detected in video sequencer. Strip %s at frame %d will not be rendered"

> @pioverfour should it be "Video Sequencer" as that is what it is in the UI? Both are fine IMO, but it’s overwhelmingly just “sequencer.” <details> "**Video Sequencer**" "Color space that the **sequencer** operates in" "Safe areas used in 3D view and the **sequencer**" "Final frame of the mask (used for **sequencer**)" "First frame of the mask (used for **sequencer**)" "**Sequencer** Color Space Settings" "Settings of color space **sequencer** is working in" "**Sequencer** Editors" "**Sequencer**" "**Sequencer** Preview" "Render using the **sequencer**'s OpenGL display" "**Sequencer** effect type" "Delete selected strips from the **sequencer**" "Add an effect to the **sequencer**, most are applied on top of existing strips" "Add an image or image sequence to the **sequencer**" "Add a mask strip to the **sequencer**" "Put the contents of a meta-strip back in the **sequencer**" "Add a movie strip to the **sequencer**" "Add a movieclip strip to the **sequencer**" "Refresh **Sequencer**" "Refresh the **sequencer** editor" "Reload strips in the **sequencer**" "Add a strip to the **sequencer** using a Blender scene as a source" "Add a sound strip to the **sequencer**" "**Sequencer** Swap Data" "Swap 2 **sequencer** strips" "View all the strips in the **sequencer**" "Zoom the **sequencer** on the selected strips" "**Sequencer** View Zoom Ratio" "Change zoom ratio of **sequencer** preview" "**Sequencer** Overlays" "**Sequencer** Strips" "Use metadata from the strips in the **sequencer**" "**Sequencer** Preview Shading" "Display method used in the **sequencer** view" "Process the render (and composited) result through the video sequence editor pipeline, if **sequencer** strips exist" "Use workbench render settings from the **sequencer** scene, instead of each individual scene used in the strip" "Calculate modifiers in linear space instead of **sequencer**'s space" "Use the Scene's **Sequencer** timeline as input" "**Sequencer**'s active strip" "Partial overlay on top of the **sequencer** with a frame offset" "Use **sequencer** strip as mask input" "Display the strip color tags in the **sequencer**" "**Sequencer** Tool Settings" "Display data belonging to the **Video Sequencer**" "View mode to use for displaying **sequencer** output" "Type of the **Sequencer** view (**sequencer**, preview or both)" "**Sequencer** & Preview" "**Sequencer**Common" "**Sequencer**" "**Sequencer** Tool: Tweak" "**Sequencer** Tool: Tweak (fallback)" "**Sequencer** Tool: Select Box" "**Sequencer** Tool: Select Box (fallback)" "**Sequencer** Tool: Blade" "**Sequencer** Tool: Blade (fallback)" "**Sequencer**Preview" "**Sequencer** Tool: Cursor" "**Sequencer** Tool: Cursor (fallback)" "**Sequencer** Tool: Move" "**Sequencer** Tool: Move (fallback)" "**Sequencer** Tool: Rotate" "**Sequencer** Tool: Rotate (fallback)" "**Sequencer** Tool: Scale" "**Sequencer** Tool: Scale (fallback)" "**Sequencer** Tool: Sample" "**Sequencer** Tool: Sample (fallback)" "Toggle **Sequencer**/Preview" "Add a crossfade transition to the **sequencer**" "Add an add effect strip to the **sequencer**" "Add a subtract effect strip to the **sequencer**" "Add an alpha over effect strip to the **sequencer**" "Add an alpha under effect strip to the **sequencer**" "Add a gamma cross transition to the **sequencer**" "Add a multiply effect strip to the **sequencer**" "Add an alpha over drop effect strip to the **sequencer**" "Add a wipe transition to the **sequencer**" "Add a glow effect strip to the **sequencer**" "Add a transform effect strip to the **sequencer**" "Add a color strip to the **sequencer**" "Add a speed effect strip to the **sequencer**" "Add a multicam selector effect strip to the **sequencer**" "Add an adjustment layer effect strip to the **sequencer**" "Add a gaussian blur effect strip to the **sequencer**" "Add a text strip to the **sequencer**" "Add a color mix effect strip to the **sequencer**" "Border rendering is not supported by **sequencer**" "Recursion detected in **video sequencer**. Strip %s at frame %d will not be rendered" </details>
Author
Member

I pushed a commit to change this to "Video Sequencer".
Thanks for pointing this out! :)

I pushed a commit to change this to "Video Sequencer". Thanks for pointing this out! :)

Hi, sorry but I just have to ask...

I'm translator for Blender UI into Spanish, but I'm having a hard time figuring out how to correctly translate "Main" (i.e. source Main, destination Main) in this context.
Could someone just give me some light about what "Main" refers to here?

The specific string reads:
"Merged %d IDs from '%s' Main into '%s' Main; %d IDs and %d Libraries already existed as part of the destination Main, and %d IDs missing from destination Main, were freed together with the source Main"

Thanks!!

Hi, sorry but I just have to ask... I'm translator for Blender UI into Spanish, but I'm having a hard time figuring out how to correctly translate "Main" (i.e. source Main, destination Main) in this context. Could someone just give me some light about what "Main" refers to here? The specific string reads: "Merged %d IDs from '%s' Main into '%s' Main; %d IDs and %d Libraries already existed as part of the destination Main, and %d IDs missing from destination Main, were freed together with the source Main" Thanks!!

Main is the name of the structure storing (almost) all data in Blender session. It can be seen as the runtime version of a blendfile. Sometimes we also call it main database.

`Main` is the name of the structure storing (almost) all data in Blender session. It can be seen as the runtime version of a blendfile. Sometimes we also call it `main database`.

Note that this report is more for advanced technical users and/or (pipelines) developers, so its translation is not critical - don't think it even shows up in the UI?

Note that this report is more for advanced technical users and/or (pipelines) developers, so its translation is not critical - don't think it even shows up in the UI?

Thanks for the clarification @mont29 !!

Thanks for the clarification @mont29 !!
First-time contributor

@mont29 Is all of this debug print necessary after this has been committed?
(imo, it seems to be unnecessary clutter if the function is working as intended)
image

@mont29 Is all of this debug print necessary after this has been committed? (imo, it seems to be unnecessary clutter if the function is working as intended) ![image](/attachments/ce4fd836-4c4c-4276-8c2f-c8648ad83dc9)

I believe this kind of info is important to keep easily available for technical artists and/or pipeline tools. This feature can generate unexpected results in some cases (from a basic user perspective), this type of info helps understanding what happens. This is similar handling to what we report when opening some blendfile or linked data.

In that sense, I think it still belongs to the 'report' category, and not the clog one.

I believe this kind of info is important to keep easily available for technical artists and/or pipeline tools. This feature can generate unexpected results in some cases (from a basic user perspective), this type of info helps understanding what happens. This is similar handling to what we report when opening some blendfile or linked data. In that sense, I think it still belongs to the 'report' category, and not the `clog` one.
First-time contributor

@mont29 Please consider limiting all of this, when you feel it is safe to do so. I'm developing Blender add-ons which extensively exposes the console window, because Blender lock the UI when running external processes, and using the console is the only way to communicate what is going on. And having copy operations within those processes adds 8 lines of print-out each time, which is completely unrelated to the function of the add-on and will clutter the experience and even confuse users.

@mont29 Please consider limiting all of this, when you feel it is safe to do so. I'm developing Blender add-ons which extensively exposes the console window, because Blender lock the UI when running external processes, and using the console is the only way to communicate what is going on. And having copy operations within those processes adds 8 lines of print-out each time, which is completely unrelated to the function of the add-on and will clutter the experience and even confuse users.
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
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 Assignees
9 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#114703
No description provided.