Sequencer file based copy paste #114703
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
9 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#114703
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "ZedDB/blender:copy_paste"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This 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.
@blender-bot package
Package build started. Download here when ready.
067d53e50c
to204c7e7e5b
@blender-bot package
Package build started. Download here when ready.
Thats a very neat feature being cooked here. While it is WIP, I gave it whirl anyway. Some observations:
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.
Agreed.
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.
Agreed.
204c7e7e5b
to5480812338
WIP: Sequencer file based copy pasteto Sequencer file based copy pasteI'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.
@blender-bot package
Package build started. Download here when ready.
From a functional design point of view:
All relative paths are remapped.
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.
@ -23,0 +32,4 @@
#include "BKE_appdir.h"
#include "BKE_blender_copybuffer.h"
#include "BKE_blendfile.h"
#include "BKE_context.h"
BKE_context.hh
@ -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
.@ -192,0 +293,4 @@
}
}
int stripts_to_paste = 0;
num_stripts_to_paste
@ -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.
68b686f0e0
to910f3ac379
Addressed all the review comments.
I had to rebase as
BKE_context.hh
was a recent change that wasn't in my branch.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.
@ -164,0 +206,4 @@
/* Paste Operator Helper functions
*/
struct Main_pair {
ReuseIDsData
as it is used as a data container for thepaste_strips_data_ids_reuse_or_add
, orMainPair
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.
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.
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).
paste_strips_data_ids_reuse_or_add
):IDRemapper
data) and 'to be duplicated' IDs as part of the suggestedSEQPasteData
structure.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).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 aX_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
andsequencer_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?
@ -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
@ -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
@ -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
@ -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
@ -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.@ -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
@ -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.)?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 withBKE_id_delete
afterwards?@ -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 toED_id_action_ensure
.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?
@ -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 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 astd::pair
), but was changed because of the feedback here.@ -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
@ -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.@ -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
@ -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@ -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?
@ -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.
@ -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?
@ -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
@ -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);
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 neededitors/include
.There is virtually no low level VSE data manipulation here, so the code could be in
sequencer_edit.cc
or own file inspace_sequencer
folder.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.
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 likeED_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.@blender-bot build
We are getting there, but think this still needs a bit more work. Mainly:
@ -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).
@ -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.This is the only function that this name is used? There are no other places to rename this variable in.
@ -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
@ -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.@ -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
?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 tonullptr
in an IDRemapper data, and then callBKE_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.
@ -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).
What is the solution?
I understand the issue. But I don't see any concrete solution proposed here?
@ -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@ -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 aRPT_ERROR
, since this situation is not expected to happen.bcb52d097e
to9aa05b2c0b
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?
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:
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: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.
@ -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 returnIDWALK_RET_NOP
, since you still do want to process the embedded ID pointers.IDWALK_CB_LOOPBACK
andIDWALK_CB_INTERNAL
should returnIDWALK_RET_STOP_RECURSION
, to avoid going deeper into these (iirc this is implemented behavior for at leastIDWALK_CB_LOOPBACK
anyway, but still better to be logical and consistent here)@ -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)
@ -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.@ -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.
@ -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).@ -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.
@ -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.
@ -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).
@ -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)@ -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.
@ -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
andid_src->lib
pointers are notnullptr
! Also comment back to the check aboutnullptr
destination ID inshould_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?
@ -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
?)@ -192,0 +480,4 @@
}
}
BKE_libblock_remap_multiple(bmain_dst, paste_data.id_remapper, 0);
BKE_libblock_remap_multiple
does not processID.lib
pointer by default, you need to give it the specific (shiny brand new) flagID_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.
6f1cf1ad01
tobee2ea7465
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 ofBKE_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.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:
It seems like it is working as intended to me now.
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:
@ -183,0 +244,4 @@
}
Main *bmain = CTX_data_main(C);
Scene *scene = CTX_data_scene(C);
Would rather pass
scene
andbmain
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
@ -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.
@ -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
?@ -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
;)b9d59a90db
to4c21c64d79
@blender-bot build
@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?
LGTM.
Now there is no point having
SEQUENCER_OT_paste
insequencer_edit.cc
file, but that is not blocking issue.@ -0,0 +230,4 @@
}
/* We are all done! */
BKE_report(op->reports, RPT_INFO, "Copied the selected VSE strips to internal clipboard");
“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.
(Maybe a follow-up patch could be to add missing-source-indication to all strips needing it?)
@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"
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!!
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 itmain 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?
Thanks for the clarification @mont29 !!
@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)
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.@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.