GPv3: Duplicate operator (edit mode) #115389

Merged
Falk David merged 23 commits from casey-bianco-davis/blender:GPv3-Duplicate into main 2023-12-01 10:14:23 +01:00

Adds duplicate operator fallowing the legacy system.

Note: Most of the exec code is modified from remove_points_and_split

resolves: #113640

Adds duplicate operator fallowing the legacy system. Note: Most of the exec code is modified from `remove_points_and_split` resolves: #113640
casey-bianco-davis added 1 commit 2023-11-25 02:41:20 +01:00
Pratik Borhade requested review from Falk David 2023-11-25 06:09:28 +01:00
Pratik Borhade requested review from Hans Goudey 2023-11-25 06:09:29 +01:00
Pratik Borhade added this to the Grease Pencil project 2023-11-25 06:09:36 +01:00
Pratik Borhade added the
Module
Grease Pencil
label 2023-11-25 06:09:41 +01:00
Falk David requested changes 2023-11-27 11:58:27 +01:00
Falk David left a comment
Member

Thanks for working on this :)

Did a first review pass and added some comments.

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;
Member

Looks like this change shouldn't be in this PR.

Looks like this change shouldn't be in this PR.
casey-bianco-davis marked this conversation as resolved
@ -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();
Member

Looks like this change shouldn't be in this PR.

Looks like this change shouldn't be in this PR.
casey-bianco-davis marked this conversation as resolved
@ -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);
Member

Use const int num_points_to_add = mask.size(); to avoid the linear search.

Use `const int num_points_to_add = mask.size();` to avoid the linear search.
casey-bianco-davis marked this conversation as resolved
@ -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);
Member

We can replace the first curves.curves_range() loop by using some helper functions here. This avoids calling append a bunch of times ;)

Array<int> dst_to_src_point(curves.points_num() + num_points_to_add);
array_utils::fill_index_range<int>(dst_to_src_point.as_mutable_span().drop_back(num_points_to_add));

Vector<int> dst_curve_counts(curves.curves_num());
offset_indices::copy_group_sizes(points_by_curve, curves.curves_range(), dst_curve_counts.as_mutable_span());

Vector<int> dst_to_src_curve(curves.curves_num());
array_utils::fill_index_range<int>(dst_to_src_curve.as_mutable_span());

Vector<bool> dst_cyclic(curves.curves_num());
src_cyclic.materialize(dst_cyclic.as_mutable_span());
We can replace the first `curves.curves_range()` loop by using some helper functions here. This avoids calling `append` a bunch of times ;) ``` Array<int> dst_to_src_point(curves.points_num() + num_points_to_add); array_utils::fill_index_range<int>(dst_to_src_point.as_mutable_span().drop_back(num_points_to_add)); Vector<int> dst_curve_counts(curves.curves_num()); offset_indices::copy_group_sizes(points_by_curve, curves.curves_range(), dst_curve_counts.as_mutable_span()); Vector<int> dst_to_src_curve(curves.curves_num()); array_utils::fill_index_range<int>(dst_to_src_curve.as_mutable_span()); Vector<bool> dst_cyclic(curves.curves_num()); src_cyclic.materialize(dst_cyclic.as_mutable_span()); ```
casey-bianco-davis marked this conversation as resolved
@ -1451,0 +1484,4 @@
}
/* Add the duplicated curves and points. */
for (const int curve_i : curves.curves_range()) {
Member

With the changes above, make sure to initialize curr_dst_point_id correctly: int curr_dst_point_id = curves.points_num();

With the changes above, make sure to initialize `curr_dst_point_id` correctly: `int curr_dst_point_id = curves.points_num();`
casey-bianco-davis marked this conversation as resolved
@ -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();
Member

Use const.

Use `const`.
casey-bianco-davis marked this conversation as resolved
@ -1451,0 +1564,4 @@
Object *object = CTX_data_active_object(C);
GreasePencil &grease_pencil = *static_cast<GreasePencil *>(object->data);
bool changed = false;
Member

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.

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`.
casey-bianco-davis marked this conversation as resolved
@ -1451,0 +1577,4 @@
bke::CurvesGeometry &curves = info.drawing.strokes_for_write();
curves = duplicate_points(curves, points);
info.drawing.tag_topology_changed();
changed = true;
Member

Use changed.store(true, std::memory_order_relaxed);.

Use `changed.store(true, std::memory_order_relaxed);`.
casey-bianco-davis marked this conversation as resolved

GEO_join_geometries.hh?

`GEO_join_geometries.hh`?
casey-bianco-davis added 5 commits 2023-11-28 02:46:46 +01:00
Falk David requested changes 2023-11-28 11:37:01 +01:00
Falk David left a comment
Member

Ok, second pass. Replacing some more loops.
Otherwise this looks good to me :)

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())) {
Member

We can replace this loop with fill_index_range as well.

array_utils::fill_index_range<int>(
    dst_to_src_point.as_mutable_span().slice(curr_dst_point_id, count),
    range.start() + points.first());
curr_dst_point_id += range.size();
We can replace this loop with `fill_index_range` as well. ``` array_utils::fill_index_range<int>( dst_to_src_point.as_mutable_span().slice(curr_dst_point_id, count), range.start() + points.first()); curr_dst_point_id += range.size(); ```
casey-bianco-davis marked this conversation as resolved
@ -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()];
Member

Move this below const IndexRange range_ids ouside of the loop.

Move this below `const IndexRange range_ids` ouside of the loop.
casey-bianco-davis marked this conversation as resolved
@ -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())) {
Member

Same idea as above.

array_utils::fill_index_range<int>(
    dst_to_src_point.as_mutable_span().slice(curr_dst_point_id, first_range.size()),
    first_range.start() + points.first());
curr_dst_point_id += first_range.size();

Also: curr_dst_point_id should now probably be something like curr_dst_point_start.

Same idea as above. ``` array_utils::fill_index_range<int>( dst_to_src_point.as_mutable_span().slice(curr_dst_point_id, first_range.size()), first_range.start() + points.first()); curr_dst_point_id += first_range.size(); ``` Also: `curr_dst_point_id` should now probably be something like `curr_dst_point_start`.
casey-bianco-davis marked this conversation as resolved
@ -1451,0 +1581,4 @@
WM_event_add_notifier(C, NC_GEOM | ND_DATA, &grease_pencil);
}
return OPERATOR_FINISHED;
}
Member

Missing blank line here.

Missing blank line here.
casey-bianco-davis marked this conversation as resolved
Hans Goudey reviewed 2023-11-28 17:04:30 +01:00
Hans Goudey left a comment
Member

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.

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>(
Member

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.

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.
Member

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 simple copy_from (array_utils::copy). Since the index map isn't necessary for the original points.

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 simple `copy_from` (`array_utils::copy`). Since the index map isn't necessary for the original points.
Member

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.

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.
casey-bianco-davis marked this conversation as resolved
casey-bianco-davis added 5 commits 2023-11-29 02:13:54 +01:00
casey-bianco-davis added 1 commit 2023-11-30 06:13:48 +01:00
Falk David approved these changes 2023-11-30 10:47:37 +01:00
Hans Goudey requested changes 2023-11-30 15:32:20 +01:00
Hans Goudey left a comment
Member

Nice, looks good. Just a few comments now.

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);
Member

src and dst are the same attribute, and src_attributes and dst_attributes are the same. It should be simpler to use lookup_for_write_span here too.

`src` and `dst` are the same attribute, and `src_attributes` and `dst_attributes` are the same. It should be simpler to use `lookup_for_write_span` here too.
casey-bianco-davis marked this conversation as resolved
@ -1451,0 +1544,4 @@
return true;
}
if (meta_data.domain == ATTR_DOMAIN_CURVE) {
Member

A switch case with a default containing BLI_assert_unreachable() should be a nicer replacement for the if (!(meta_data.domain == ATTR_DOMAIN_CURVE || meta_data.domain == ATTR_DOMAIN_POINT)) { check at the top of this loop IMO.

A switch case with a `default` containing `BLI_assert_unreachable()` should be a nicer replacement for the `if (!(meta_data.domain == ATTR_DOMAIN_CURVE || meta_data.domain == ATTR_DOMAIN_POINT)) {` check at the top of this loop IMO.
casey-bianco-davis marked this conversation as resolved
@ -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(
Member

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?

if (bke::GSpanAttributeWriter selection = attributes.lookup_for_write_span(".selection")) {
  if (selection.domain == ATTR_DOMAIN_POINT) {
    curves::fill_selection_false(selection.span, IndexMask(old_points_num));
  }
  else if (selection.domain == ATTR_DOMAIN_CURVE) {
    curves::fill_selection_false(selection.span, IndexMask(old_curves_num));
  }
  selection.finish();
}
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? ```cpp if (bke::GSpanAttributeWriter selection = attributes.lookup_for_write_span(".selection")) { if (selection.domain == ATTR_DOMAIN_POINT) { curves::fill_selection_false(selection.span, IndexMask(old_points_num)); } else if (selection.domain == ATTR_DOMAIN_CURVE) { curves::fill_selection_false(selection.span, IndexMask(old_curves_num)); } selection.finish(); } ```
Author
Member

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.

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.
Member

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.

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.
Author
Member

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.

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.
casey-bianco-davis marked this conversation as resolved
@ -1451,0 +1602,4 @@
/* Identifiers. */
ot->name = "Duplicate";
ot->idname = "GREASE_PENCIL_OT_duplicate";
ot->description = "Duplicate the selected strokes";
Member

Isn't it really duplicating the selected points?

Isn't it really duplicating the selected points?
casey-bianco-davis marked this conversation as resolved
casey-bianco-davis added 5 commits 2023-12-01 02:39:52 +01:00
casey-bianco-davis added 1 commit 2023-12-01 02:50:43 +01:00
Hans Goudey requested changes 2023-12-01 03:09:16 +01:00
@ -1451,0 +1539,4 @@
if (id.name() == "cyclic") {
return true;
}
Member

This is nitpicky, but all the blank lines inside this switch seem unnecessary to me. The brackets already give that visual separation.

This is nitpicky, but all the blank lines inside this switch seem unnecessary to me. The brackets already give that visual separation.
casey-bianco-davis marked this conversation as resolved
@ -1451,0 +1560,4 @@
BLI_assert_unreachable();
return true;
break;
};
Member

Unnecessary ; after bracket. Also, break is unnecessary after return

Unnecessary ; after bracket. Also, `break` is unnecessary after return
casey-bianco-davis marked this conversation as resolved
@ -1451,0 +1568,4 @@
return true;
});
array_utils::copy(dst_cyclic.as_span(), curves.cyclic_for_write().drop_front(old_curves_num));
Member

Add this at the end:

  this->update_curve_types();
  this->tag_topology_changed();
Add this at the end: ``` this->update_curve_types(); this->tag_topology_changed(); ```
casey-bianco-davis marked this conversation as resolved
casey-bianco-davis added 4 commits 2023-12-01 03:30:16 +01:00
Hans Goudey reviewed 2023-12-01 03:38:15 +01:00
@ -1451,0 +1533,4 @@
if (!attribute) {
return true;
}
if (id.name() == ".selection") {
Member

Instead of skipping the selection here, you can call attributes.remove(".selection"); right before the curves.resize call. That will avoid the need to copy the values to the reallocated array. Sorry I wasn't clearer before.

Instead of skipping the selection here, you can call `attributes.remove(".selection");` right before the `curves.resize` call. That will avoid the need to copy the values to the reallocated array. Sorry I wasn't clearer before.
casey-bianco-davis marked this conversation as resolved
casey-bianco-davis added 1 commit 2023-12-01 04:24:59 +01:00
casey-bianco-davis requested review from Hans Goudey 2023-12-01 04:25:32 +01:00
Hans Goudey approved these changes 2023-12-01 04:46:58 +01:00
Falk David merged commit fb275bc040 into main 2023-12-01 10:14:23 +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
4 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#115389
No description provided.