GPv3: Copy and Paste operators for copying strokes and points #114145
No reviewers
Labels
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#114145
Loading…
Reference in New Issue
No description provided.
Delete Branch "SietseB/blender:gpv3-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 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 copyCtrl
-V
for pasteCtrl
-Shift
-V
for paste in the back (behind all existing strokes)@ -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.
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.
@ -951,0 +955,4 @@
/** \name Copy and Paste Operator
* \{ */
static bool append_geometry_to_layer(Vector<bke::CurvesGeometry> &sources,
Const argument should be first.
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
.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.
@ -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()
Inline comments are just seconding what Iliya said in a couple places and adding more detail.
@ -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; });
Careful here, without the
&
this causes a copy ofCurvesGeometry
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(
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
@ -951,0 +1150,4 @@
int32_t num_copied = 0;
/* Copy all selected strokes/points on all editable layers. */
grease_pencil.foreach_editable_drawing(
This needs to be updated to the newer API now.
casey-bianco-davis referenced this pull request2024-02-06 21:49:19 +01:00
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.
Thanks for updating, left some comments.
@ -2181,0 +2184,4 @@
* \{ */
/* Global clipboard for Grease Pencil curves. */
struct GreasePencilClipboard {
Since this is already in the
ed::greasepencil
namespace, I think just calling itClipboard
is good.@ -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;
This seems redundant to
materials.size()
.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.@ -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;
@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.
@ -2181,0 +2323,4 @@
GreasePencilClipboard &grease_pencil_clipboard = get_grease_pencil_clipboard();
bool anything_copied = false;
int32_t num_copied = 0;
This can be just
int
.@ -2181,0 +2339,4 @@
}
/* Get a copy of the selected geometry on this layer. */
const AnonymousAttributePropagationInfo propagation_info{};
You can just pass
{}
directly to the function that needs aAnonymousAttributePropagationInfo
@ -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)
The context is only needed for the
scene
and thetool_settings
(which are part of the scene). So just pass thescene
into this function.@ -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");
I wouldn't expect this function to report something. Better to put this in
grease_pencil_paste_strokes_exec
where this function is called (whenensure_active_keyframe
returnsfalse
). 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 abool
is fine.@ -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);
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.@ -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.@ -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:
@ -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},
Trailing whitespace here
@ -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()) {
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.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 withn
curves and I join them, is it always guaranteed that the B curves have an index range ofm
tom + n - 1
after the join? Or are there corner cases?When it is guaranteed, we could do the remapping after the join.
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.
@ -2181,0 +2304,4 @@
static int grease_pencil_copy_strokes_exec(bContext *C, wmOperator *op)
{
using namespace blender::bke;
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@ -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>(
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
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:But I wouldn't recommend it. It's obscuring the code and an optimization for a black swan.
Okay, that's fine, it can always be done later.
@blender-bot build