GPv3: Duplicate operator (edit mode) #115389
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
4 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#115389
Loading…
Reference in New Issue
No description provided.
Delete Branch "casey-bianco-davis/blender:GPv3-Duplicate"
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?
Adds duplicate operator fallowing the legacy system.
Note: Most of the exec code is modified from
remove_points_and_split
resolves: #113640
Thanks for working on this :)
Did a first review pass and added some comments.
@ -714,7 +715,6 @@ static void GREASE_PENCIL_OT_delete(wmOperatorType *ot)
ot->description = "Delete selected strokes or points";
/* Callbacks. */
ot->invoke = WM_menu_invoke;
Looks like this change shouldn't be in this PR.
@ -664,6 +664,7 @@ static bke::CurvesGeometry remove_points_and_split(const bke::CurvesGeometry &cu
/* Transfer point attributes. */
gather_attributes(src_attributes, ATTR_DOMAIN_POINT, {}, {}, dst_to_src_point, dst_attributes);
dst_curves.update_curve_types();
Looks like this change shouldn't be in this PR.
@ -1451,0 +1460,4 @@
Array<bool> points_to_duplicate(curves.points_num());
mask.to_bools(points_to_duplicate.as_mutable_span());
const int num_points_to_add = points_to_duplicate.as_span().count(true);
Use
const int num_points_to_add = mask.size();
to avoid the linear search.@ -1451,0 +1463,4 @@
const int num_points_to_add = points_to_duplicate.as_span().count(true);
int curr_dst_point_id = 0;
Array<int> dst_to_src_point(curves.points_num() + num_points_to_add);
We can replace the first
curves.curves_range()
loop by using some helper functions here. This avoids callingappend
a bunch of times ;)@ -1451,0 +1484,4 @@
}
/* Add the duplicated curves and points. */
for (const int curve_i : curves.curves_range()) {
With the changes above, make sure to initialize
curr_dst_point_id
correctly:int curr_dst_point_id = curves.points_num();
@ -1451,0 +1503,4 @@
const bool is_curve_self_joined = is_last_segment_selected && ranges_to_duplicate.size() != 1;
const bool is_cyclic = ranges_to_duplicate.size() == 1 && is_last_segment_selected;
IndexRange range_ids = ranges_to_duplicate.index_range();
Use
const
.@ -1451,0 +1564,4 @@
Object *object = CTX_data_active_object(C);
GreasePencil &grease_pencil = *static_cast<GreasePencil *>(object->data);
bool changed = false;
I'll have to clean this up in other areas as well, but this should use
std::atomic<bool>
for correctness.We're writing from potentially multiple threads to the same local variable. This might only sometimes lead to actual issues, but for clarity, we should use
atomic
.@ -1451,0 +1577,4 @@
bke::CurvesGeometry &curves = info.drawing.strokes_for_write();
curves = duplicate_points(curves, points);
info.drawing.tag_topology_changed();
changed = true;
Use
changed.store(true, std::memory_order_relaxed);
.GEO_join_geometries.hh
?Ok, second pass. Replacing some more loops.
Otherwise this looks good to me :)
@ -1451,0 +1505,4 @@
const IndexRange range = ranges_to_duplicate[range_i];
int count = range.size();
for (const int src_point : range.shift(points.first())) {
We can replace this loop with
fill_index_range
as well.@ -1451,0 +1511,4 @@
/* Join the first range to the end of the last range. */
if (is_curve_self_joined && range_i == range_ids.last()) {
const IndexRange first_range = ranges_to_duplicate[range_ids.first()];
Move this below
const IndexRange range_ids
ouside of the loop.@ -1451,0 +1512,4 @@
/* Join the first range to the end of the last range. */
if (is_curve_self_joined && range_i == range_ids.last()) {
const IndexRange first_range = ranges_to_duplicate[range_ids.first()];
for (const int src_point : first_range.shift(points.first())) {
Same idea as above.
Also:
curr_dst_point_id
should now probably be something likecurr_dst_point_start
.@ -1451,0 +1581,4 @@
WM_event_add_notifier(C, NC_GEOM | ND_DATA, &grease_pencil);
}
return OPERATOR_FINISHED;
}
Missing blank line here.
It does seem simpler and less work to rely on the join geometry code combined with the existing "copy_selected_points" function. But doing it this way is theoretically faster anyway, and makes dealing with cyclic-ness simpler as well.
It seems a bit more natural to resize existing curves though, instead of generating new curves.
@ -1451,0 +1464,4 @@
/* Add all of the orignal curves and points. */
Array<int> dst_to_src_point(curves.points_num() + num_points_to_add);
array_utils::fill_index_range<int>(
When we can use a direct copy, best not to rely on an index map and just copy directly instead. That means we can't use
gather_attributes
directly, but that's fine, better to functions that actually correspond to the needs of the algorithm.To clarify, the
Array<int>
index map should only be the size of the newly added points. The rest of the values should be copied with a simplecopy_from
(array_utils::copy
). Since the index map isn't necessary for the original points.And that's why I mentioned it would be simpler to use
resize
on the existing curves-- you wouldn't have to manually copy the other data. And it's probably simpler to optimize lots of repeated small duplications in the future.Nice, looks good. Just a few comments now.
@ -1451,0 +1537,4 @@
if (meta_data.domain == ATTR_DOMAIN_CURVE && id.name() == "cyclic") {
return true;
}
const bke::GAttributeReader src = src_attributes.lookup(id, meta_data.domain);
src
anddst
are the same attribute, andsrc_attributes
anddst_attributes
are the same. It should be simpler to uselookup_for_write_span
here too.@ -1451,0 +1544,4 @@
return true;
}
if (meta_data.domain == ATTR_DOMAIN_CURVE) {
A switch case with a
default
containingBLI_assert_unreachable()
should be a nicer replacement for theif (!(meta_data.domain == ATTR_DOMAIN_CURVE || meta_data.domain == ATTR_DOMAIN_POINT)) {
check at the top of this loop IMO.@ -1451,0 +1561,4 @@
array_utils::copy(dst_cyclic.as_span(), curves.cyclic_for_write().drop_front(old_curves_num));
/* Deselect the original curves. */
bke::GSpanAttributeWriter selection = ed::curves::ensure_selection_attribute(
I think the semantics are a bit wrong here, this shouldn't add the selection attribute, it should be independent of how the
mask
argument was created. Also looks like this assumes the domain.How about this?
Because selection attribute is true by default. and selecting everything deletes the attribute. creating the attribute when it does not existed makes it so that selecting everything and duplicating will work correctly.
Good point, a more efficient way to address that goal is probably deleting the attribute before interpolating attributes, then adding it back at the end and filling the end true. That we don't waste time copying over the values.
I don't think that deleting the attribute would be more efficient because it would have to be filled with default values.
I think it would be more efficient to skip transferring the attribute and then just fill with true and false.
@ -1451,0 +1602,4 @@
/* Identifiers. */
ot->name = "Duplicate";
ot->idname = "GREASE_PENCIL_OT_duplicate";
ot->description = "Duplicate the selected strokes";
Isn't it really duplicating the selected points?
@ -1451,0 +1539,4 @@
if (id.name() == "cyclic") {
return true;
}
This is nitpicky, but all the blank lines inside this switch seem unnecessary to me. The brackets already give that visual separation.
@ -1451,0 +1560,4 @@
BLI_assert_unreachable();
return true;
break;
};
Unnecessary ; after bracket. Also,
break
is unnecessary after return@ -1451,0 +1568,4 @@
return true;
});
array_utils::copy(dst_cyclic.as_span(), curves.cyclic_for_write().drop_front(old_curves_num));
Add this at the end:
@ -1451,0 +1533,4 @@
if (!attribute) {
return true;
}
if (id.name() == ".selection") {
Instead of skipping the selection here, you can call
attributes.remove(".selection");
right before thecurves.resize
call. That will avoid the need to copy the values to the reallocated array. Sorry I wasn't clearer before.