GPv3: Copy and Paste operators for copying strokes and points #114145

Merged
Falk David merged 13 commits from SietseB/blender:gpv3-copy-paste into main 2024-03-04 11:02:35 +01:00
Member

This PR implements the Copy and Paste operators for GPv3. The operators
are available in Edit Mode. The Copy operator copies the selected strokes/
points to a clipboard. The Paste operator pastes the strokes/points from
the clipboard to the active layer.

Keyboard shortcuts:

  • Ctrl-C for copy
  • Ctrl-V for paste
  • Ctrl-Shift-V for paste in the back (behind all existing strokes)
This PR implements the Copy and Paste operators for GPv3. The operators are available in Edit Mode. The Copy operator copies the selected strokes/ points to a clipboard. The Paste operator pastes the strokes/points from the clipboard to the active layer. Keyboard shortcuts: - `Ctrl`-`C` for copy - `Ctrl`-`V` for paste - `Ctrl`-`Shift`-`V` for paste in the back (behind all existing strokes)
Sietse Brouwer added 4 commits 2023-10-25 11:41:05 +02:00
Sietse Brouwer requested review from Falk David 2023-10-25 11:41:34 +02:00
Sietse Brouwer requested review from Hans Goudey 2023-10-25 11:41:34 +02:00
Sietse Brouwer added this to the Grease Pencil project 2023-10-25 11:41:44 +02:00
Iliya Katushenock reviewed 2023-10-25 12:38:19 +02:00
@ -855,2 +855,3 @@
const IndexMask &selection,
MutableAttributeAccessor dst_attributes)
MutableAttributeAccessor dst_attributes,
std::optional<IndexRange> dst_range)

Better to implement this function right in place where is you need this additional ranges.

Better to implement this function right in place where is you need this additional ranges.
Member

Agreed with Iliya here, seems better to reserve this function for when the mapping is complete, than to iterate over all attributes for every dst_range.

In general, this API is definitely not complete, we're sort of halfway through figuring out the right way to structure attribute interpolation between geometries.

Agreed with Iliya here, seems better to reserve this function for when the mapping is complete, than to iterate over all attributes for every dst_range. In general, this API is definitely not complete, we're sort of halfway through figuring out the right way to structure attribute interpolation between geometries.
SietseB marked this conversation as resolved
@ -951,0 +955,4 @@
/** \name Copy and Paste Operator
* \{ */
static bool append_geometry_to_layer(Vector<bke::CurvesGeometry> &sources,

Const argument should be first.

Const argument should be first.
SietseB marked this conversation as resolved
Iliya Katushenock reviewed 2023-10-25 12:52:03 +02:00
Iliya Katushenock left a comment
Member

Hm, it seems that copy-paste operator does not use static clipboard buffer and implies copy-past only inside single object?

Hm, it seems that copy-paste operator does not use static clipboard buffer and implies copy-past only inside single object?
@ -951,0 +958,4 @@
static bool append_geometry_to_layer(Vector<bke::CurvesGeometry> &sources,
bke::CurvesGeometry &dest,
const eAttrDomain selection_domain)
{

Whole this function can be done with using GEO_join_geometries.hh.

Whole this function can be done with using `GEO_join_geometries.hh`.
Member

I also think it's important to reuse the code in the join geometry code. Maybe parts of that need to be extracted.

Maybe that's not possible, but I'd really hope it is. Joining geometries is surprisingly non-trivial, especially handling the extreme cases in a performant way.

I also think it's important to reuse the code in the join geometry code. Maybe parts of that need to be extracted. Maybe that's not possible, but I'd really hope it is. Joining geometries is surprisingly non-trivial, especially handling the extreme cases in a performant way.
SietseB marked this conversation as resolved
@ -951,0 +962,4 @@
/* For safety, remove source geometries with zero points. */
sources.remove_if([](const bke::CurvesGeometry source) { return source.points_num() == 0; });
const int source_num = sources.size();
if (source_num == 0) {

is_empty()

`is_empty()`
SietseB marked this conversation as resolved
Hans Goudey requested changes 2024-01-08 20:19:18 +01:00
Dismissed
Hans Goudey left a comment
Member

Inline comments are just seconding what Iliya said in a couple places and adding more detail.

Inline comments are just seconding what Iliya said in a couple places and adding more detail.
Hans Goudey requested changes 2024-01-08 20:24:20 +01:00
Dismissed
@ -951,0 +960,4 @@
const eAttrDomain selection_domain)
{
/* For safety, remove source geometries with zero points. */
sources.remove_if([](const bke::CurvesGeometry source) { return source.points_num() == 0; });
Member

Careful here, without the & this causes a copy of CurvesGeometry

Careful here, without the `&` this causes a copy of `CurvesGeometry`
SietseB marked this conversation as resolved
Falk David requested changes 2024-02-06 12:29:13 +01:00
Dismissed
Falk David left a comment
Member

Finally getting to this. Just a few comments, generally this approach looks right (using a buffer in the object-data runtime).

Finally getting to this. Just a few comments, generally this approach looks right (using a buffer in the object-data runtime).
@ -951,0 +1125,4 @@
bke::CurvesGeometry &layer = drawing->strokes_for_write();
/* Append the geometry in the paste buffer to the active layer. */
const bool success = append_geometry_to_layer(
Member

Like Hans and Ilya suggested, this part can be implemented with join_geometries.
There is an example in source/blender/modifiers/intern/MOD_grease_pencil_mirror.cc

Like Hans and Ilya suggested, this part can be implemented with `join_geometries`. There is an example in `source/blender/modifiers/intern/MOD_grease_pencil_mirror.cc`
SietseB marked this conversation as resolved
@ -951,0 +1150,4 @@
int32_t num_copied = 0;
/* Copy all selected strokes/points on all editable layers. */
grease_pencil.foreach_editable_drawing(
Member

This needs to be updated to the newer API now.

This needs to be updated to the newer API now.
SietseB marked this conversation as resolved
Sietse Brouwer added 4 commits 2024-02-28 23:11:42 +01:00
Sietse Brouwer added 1 commit 2024-02-28 23:15:22 +01:00
Author
Member

An almost full rewrite, because the reviews took a different turn than the community task I started with (#113248: GPv3: Copy/Paste Strokes).

And I was wrong – I thought in GPv2 you could only copy/paste within an object, but that isn't true. So I rewrote the PR to use a static clipboard, enabling copying between objects. I added the part for matching materials between objects, like it is in GPv2.

An almost full rewrite, because the reviews took a different turn than the community task I started with ([#113248: GPv3: Copy/Paste Strokes](https://projects.blender.org/blender/blender/issues/113248)). And I was wrong – I thought in GPv2 you could only copy/paste _within_ an object, but that isn't true. So I rewrote the PR to use a static clipboard, enabling copying between objects. I added the part for matching materials between objects, like it is in GPv2.
Sietse Brouwer added 1 commit 2024-02-28 23:47:16 +01:00
Falk David requested changes 2024-02-29 11:16:01 +01:00
Dismissed
Falk David left a comment
Member

Thanks for updating, left some comments.

Thanks for updating, left some comments.
@ -2181,0 +2184,4 @@
* \{ */
/* Global clipboard for Grease Pencil curves. */
struct GreasePencilClipboard {
Member

Since this is already in the ed::greasepencil namespace, I think just calling it Clipboard is good.

Since this is already in the `ed::greasepencil` namespace, I think just calling it `Clipboard` is good.
SietseB marked this conversation as resolved
@ -2181,0 +2189,4 @@
/* We store the material names of the copied curves, so we can match those when pasting the
* clipboard into another object. */
Vector<std::pair<std::string, int>> materials;
int materials_num;
Member

This seems redundant to materials.size().

This seems redundant to `materials.size()`.
Author
Member

I've renamed it. It's for efficiently remapping the material indices from the source object to the paste target. When the source has 6 materials, you need a remap array of 6 positions. The materials vector can be smaller, it's only containing the materials used by the curves on the clipboard.

I've renamed it. It's for efficiently remapping the material indices from the source object to the paste target. When the source has 6 materials, you need a remap array of 6 positions. The `materials` vector can be smaller, it's only containing the materials used by the curves on the clipboard.
SietseB marked this conversation as resolved
@ -2181,0 +2249,4 @@
clipboard_material_indices.span.varray().materialize(original_clipboard_materials);
/* Get a list of all materials in the scene. */
Map<std::string, Material *> scene_materials;
Member

@mont29 Could you take a look at this? I'm not too certain about this remapping code for materials.

@mont29 Could you take a look at this? I'm not too certain about this remapping code for materials.

Indeed, same as for the storage info itself, better to use ID's UID as keys here.

Indeed, same as for the storage info itself, better to use ID's UID as keys here.
SietseB marked this conversation as resolved
@ -2181,0 +2323,4 @@
GreasePencilClipboard &grease_pencil_clipboard = get_grease_pencil_clipboard();
bool anything_copied = false;
int32_t num_copied = 0;
Member

This can be just int.

This can be just `int`.
SietseB marked this conversation as resolved
@ -2181,0 +2339,4 @@
}
/* Get a copy of the selected geometry on this layer. */
const AnonymousAttributePropagationInfo propagation_info{};
Member

You can just pass {} directly to the function that needs a AnonymousAttributePropagationInfo

You can just pass `{}` directly to the function that needs a `AnonymousAttributePropagationInfo`
SietseB marked this conversation as resolved
@ -330,6 +333,43 @@ void create_keyframe_edit_data_selected_frames_list(KeyframeEditData *ked,
}
}
bool ensure_active_keyframe(const bContext *C, const wmOperator *op, GreasePencil &grease_pencil)
Member

The context is only needed for the scene and the tool_settings (which are part of the scene). So just pass the scene into this function.

The context is only needed for the `scene` and the `tool_settings` (which are part of the scene). So just pass the `scene` into this function.
SietseB marked this conversation as resolved
@ -333,0 +340,4 @@
bke::greasepencil::Layer &active_layer = *grease_pencil.get_active_layer();
if (!active_layer.has_drawing_at(current_frame) && !blender::animrig::is_autokey_on(scene)) {
BKE_report(op->reports, RPT_ERROR, "No Grease Pencil frame to draw on");
Member

I wouldn't expect this function to report something. Better to put this in grease_pencil_paste_strokes_exec where this function is called (when ensure_active_keyframe returns false). We don't have a good way of returning specific errors yet (see https://devtalk.blender.org/t/developer-discussion-handling-errors/33054 for a discussion), but here there seems to only be one failiure case, so a bool is fine.

I wouldn't expect this function to report something. Better to put this in `grease_pencil_paste_strokes_exec` where this function is called (when `ensure_active_keyframe` returns `false`). We don't have a good way of returning specific errors yet (see https://devtalk.blender.org/t/developer-discussion-handling-errors/33054 for a discussion), but here there seems to only be one failiure case, so a `bool` is fine.
SietseB marked this conversation as resolved
@ -160,0 +163,4 @@
* Autokey is on (taking Additive drawing setting into account).
* Returns false when no keyframe could be found or created.
*/
bool ensure_active_keyframe(const bContext *C, const wmOperator *op, GreasePencil &grease_pencil);
Member

Since this seems to have been extracted from grease_pencil_brush_stroke_invoke, I would like to see that function getting used there so we don't duplicate the code.

Since this seems to have been extracted from `grease_pencil_brush_stroke_invoke`, I would like to see that function getting used there so we don't duplicate the code.
SietseB marked this conversation as resolved
Falk David requested review from Hans Goudey 2024-02-29 11:24:24 +01:00
Bastien Montagne reviewed 2024-02-29 11:37:01 +01:00
@ -2181,0 +2188,4 @@
bke::CurvesGeometry curves;
/* We store the material names of the copied curves, so we can match those when pasting the
* clipboard into another object. */
Vector<std::pair<std::string, int>> materials;

Using an ID name is only key is not going to work in linked data context (since you can have a local and several linked materials with exactly the same name).

Since this is a runtime paste buffer, I'd suggest to use ID.uid instead.

Using an ID name is only key is not going to work in linked data context (since you can have a local and several linked materials with exactly the same name). Since this is a runtime paste buffer, I'd suggest to use `ID.uid` instead.
SietseB marked this conversation as resolved
@ -2181,0 +2260,4 @@
/* Check if the material name exists in the scene. */
int target_index;
std::string material_name = grease_pencil_clipboard.materials[i].first;
if (!scene_materials.contains(material_name)) {

Would be more efficient to do a single lookup here, something like that:

    Material *material = scene_materials.lookup_default(material_name, nullptr);
    if (!material) {
      ...
      continue;
    }

    target_index = BKE_grease_pencil_object_material_index_get_by_name(object, material_name.c_str());
    ...
Would be more efficient to do a single lookup here, something like that: ```Cpp Material *material = scene_materials.lookup_default(material_name, nullptr); if (!material) { ... continue; } target_index = BKE_grease_pencil_object_material_index_get_by_name(object, material_name.c_str()); ... ```
SietseB marked this conversation as resolved
Sietse Brouwer added 1 commit 2024-02-29 18:03:39 +01:00
Hans Goudey reviewed 2024-02-29 18:27:18 +01:00
@ -4615,0 +4615,4 @@
# Copy/paste
("grease_pencil.copy", {"type": 'C', "value": 'PRESS', "ctrl": True}, None),
("grease_pencil.paste", {"type": 'V', "value": 'PRESS', "ctrl": True}, None),
("grease_pencil.paste", {"type": 'V', "value": 'PRESS', "shift": True, "ctrl": True},
Member

Trailing whitespace here

Trailing whitespace here
SietseB marked this conversation as resolved
@ -2181,0 +2273,4 @@
}
/* Remap the material indices of the clipboard curves to the target object material indices. */
for (const int i : clipboard_material_indices.span.index_range()) {
Member

What do you think about remapping the material indices in the final joined geometry? The current method seems a bit hacky, and it's also less efficient, and always creates the material_index attribute even if neither the clipboard or the layer's curves already had it.

What do you think about remapping the material indices in the final joined geometry? The current method seems a bit hacky, and it's also less efficient, and always creates the `material_index` attribute even if neither the clipboard or the layer's curves already had it.
Author
Member

Perhaps I was too conservative, but I treated join_geometries as a black box, i.e. not knowing what the index range of the pasted curves is after the join.
When I have a CurvesGeometry A with m curves and B with n curves and I join them, is it always guaranteed that the B curves have an index range of m to m + n - 1 after the join? Or are there corner cases?
When it is guaranteed, we could do the remapping after the join.

Perhaps I was too conservative, but I treated `join_geometries` as a black box, i.e. not knowing what the index range of the pasted curves is after the join. When I have a CurvesGeometry A with `m` curves and B with `n` curves and I join them, is it always guaranteed that the B curves have an index range of `m` to `m + n - 1` after the join? Or are there corner cases? When it is guaranteed, we could do the remapping after the join.
Member

Yeah, the index range is in order of the input geometries. It's the simplest way to implement the joining, and the behavior is exposed in the join geometry node so it won't change at this point. So it's fine to rely on it. I appreciate you being conservative about that though.

Yeah, the index range is in order of the input geometries. It's the simplest way to implement the joining, and the behavior is exposed in the join geometry node so it won't change at this point. So it's fine to rely on it. I appreciate you being conservative about that though.
SietseB marked this conversation as resolved
@ -2181,0 +2304,4 @@
static int grease_pencil_copy_strokes_exec(bContext *C, wmOperator *op)
{
using namespace blender::bke;
Member

Personally I find these using namespace blender::bke; not so helpful. I find it helps to see when a function comes from elsewhere or blenkernel

Personally I find these `using namespace blender::bke;` not so helpful. I find it helps to see when a function comes from elsewhere or blenkernel
SietseB marked this conversation as resolved
Sietse Brouwer added 1 commit 2024-02-29 21:04:55 +01:00
Sietse Brouwer added 1 commit 2024-02-29 23:57:43 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
1c011b5514
Remap the clipboard material indices after pasting
Hans Goudey reviewed 2024-03-01 00:05:57 +01:00
@ -2181,0 +2283,4 @@
/* Remap the material indices of the pasted curves to the target object material indices. */
bke::MutableAttributeAccessor attributes =
target_drawing->strokes_for_write().attributes_for_write();
bke::SpanAttributeWriter<int> material_indices = attributes.lookup_or_add_for_write_span<int>(
Member

Is it necessary to always add the attribute? Would be nice to avoid that if possible, accumulating the attributes unnecessarily will make performance worse elsewhere too

Is it necessary to always add the attribute? Would be nice to avoid that if possible, accumulating the attributes unnecessarily will make performance worse elsewhere too
Author
Member

The material_index attribute is so enormously common on GP curves, I would say it's practically always there.
I could add an if, something like:

if (clipboard.materials.size() > 1 ||
    clipboard_material_remap[clipboard.materials[0].second] != 0)
{
  bke::SpanAttributeWriter<int> material_indices = attributes.lookup_or_add_for_write_span<int>(
      "material_index", bke::AttrDomain::Curve);
  ...
}

But I wouldn't recommend it. It's obscuring the code and an optimization for a black swan.

The `material_index` attribute is so enormously common on GP curves, I would say it's practically always there. I could add an `if`, something like: ```C if (clipboard.materials.size() > 1 || clipboard_material_remap[clipboard.materials[0].second] != 0) { bke::SpanAttributeWriter<int> material_indices = attributes.lookup_or_add_for_write_span<int>( "material_index", bke::AttrDomain::Curve); ... } ``` But I wouldn't recommend it. It's obscuring the code and an optimization for a black swan.
Member

Okay, that's fine, it can always be done later.

Okay, that's fine, it can always be done later.
SietseB marked this conversation as resolved
Hans Goudey approved these changes 2024-03-01 21:31:02 +01:00
Falk David approved these changes 2024-03-04 10:30:54 +01:00
Member

@blender-bot build

@blender-bot build
Falk David merged commit ad8180c54c into main 2024-03-04 11:02:35 +01:00
Sietse Brouwer deleted branch gpv3-copy-paste 2024-03-04 22:32:10 +01:00
Sign in to join this conversation.
No reviewers
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
Interest: X11
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 project
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#114145
No description provided.