GPv3: Select similar #111410

Open
Lorenzo-Carpaneto wants to merge 17 commits from Lorenzo-Carpaneto/blender:gpv3-select-similar into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Contributor

Still WIP, but let's discuss problems and possible solutions. Even if we have discussed already this problem, I think better to reassess here.

For the templated function, is it good to leave it in the header file or better to use other strategy?
The set should be able to handle concurrency for better performances.
LAYER and MATERIAL could be moved into a select linked operator. We were talking about submenu but I am not completely clear of the idea.

Let me know your thoughts :)

Still WIP, but let's discuss problems and possible solutions. Even if we have discussed already this problem, I think better to reassess here. For the templated function, is it good to leave it in the header file or better to use other strategy? The set should be able to handle concurrency for better performances. LAYER and MATERIAL could be moved into a select linked operator. We were talking about submenu but I am not completely clear of the idea. Let me know your thoughts :)
Lorenzo-Carpaneto added 2 commits 2023-08-23 00:14:00 +02:00
6786da323a gpv3-select-similar
First working version. WIP: some things are to be worked on
c60f946e63 gpv3-select-similar
Slight refactor. Still WIP.
Iliya Katushenock added this to the Grease Pencil project 2023-08-23 00:20:38 +02:00
Matias Mendiola requested review from Falk David 2023-08-23 13:39:39 +02:00
Falk David changed title from WIP: gpv3-select-similar to WIP: GPv3: Select similar 2023-09-11 11:53:53 +02:00
Falk David requested changes 2023-09-14 12:02:08 +02:00
Falk David left a comment
Member

Since this is still WIP, I won't comment on specific code things. I have some general thoughts instead:

I think the case LAYER: in select_similar_exec shouldn't call the same functions as all the other option. Layers are not attributes in CurvesGeometry. All the other options select something based on an attribure. So making the LAYER option call a different function would remove complexity in select_similar_main and also remove the necessity for foreach_editable_drawing_in_layer_ex.

Since this is still WIP, I won't comment on specific code things. I have some general thoughts instead: I think the `case LAYER:` in `select_similar_exec` shouldn't call the same functions as all the other option. Layers are not attributes in `CurvesGeometry`. All the other options select something based on an attribure. So making the `LAYER` option call a different function would remove complexity in `select_similar_main` and also remove the necessity for `foreach_editable_drawing_in_layer_ex`.
Lorenzo-Carpaneto added 2 commits 2023-09-30 19:33:56 +02:00
415d5ae3eb gpv3-select-similar
Create new function for selecting similar layer
Make code more readable.
6280c5a80d gpv3-select-similar
Remove useless parameters from select_layer and setup a default param
function
Lorenzo-Carpaneto added 1 commit 2023-12-02 20:10:10 +01:00
Lorenzo-Carpaneto changed title from WIP: GPv3: Select similar to GPv3: Select similar 2023-12-02 20:11:17 +01:00
Lorenzo-Carpaneto requested review from Falk David 2023-12-02 21:48:39 +01:00
Falk David requested changes 2023-12-04 11:51:28 +01:00
Falk David left a comment
Member

There are some issues with this PR, I added some remarks, hopefully it's all relatively clear.

Also some general comments:

  • Please have a look at https://wiki.blender.org/wiki/Style_Guide/C_Cpp. There are some places that don't follow our style guide (e.g. variable names should be snake_case, use const when possible, etc.).
  • Generally, Blender doesn't use exceptions. I added some comments about this in the code.
There are some issues with this PR, I added some remarks, hopefully it's all relatively clear. Also some general comments: * Please have a look at https://wiki.blender.org/wiki/Style_Guide/C_Cpp. There are some places that don't follow our style guide (e.g. variable names should be `snake_case`, use `const` when possible, etc.). * Generally, Blender doesn't use exceptions. I added some comments about this in the code.
@ -28,2 +28,4 @@
namespace blender::ed::greasepencil {
/* WARNING: don't change existing ones without modifying rearrange func accordingly */
typedef enum eSelectSimilar {
Member

This should be an enum class SelectSimilarType.

This should be an `enum class SelectSimilarType`.
@ -286,1 +304,4 @@
static int select_similar_exec(bContext *C, wmOperator *op)
{
const int type = RNA_enum_get(op->ptr, "type");
Member

With the change above: const SelectSimilarType type = SelectSimilarType(RNA_enum_get(op->ptr, "type"));

With the change above: `const SelectSimilarType type = SelectSimilarType(RNA_enum_get(op->ptr, "type"));`
@ -287,0 +313,4 @@
switch (type) {
case LAYER:
ed::curves::select_similar_layer(
Member

These functions shouldn't be defined in ED_curves.hh (especially select_similar_layer) since the functions are specific to grease pencil. If in the future, some of the functionality is used by the Curves object, we can still move some of them there.
I would suggest to keep it simple and define all of them as static functions above this function (select_similar_exec). That also means we don't have to have code in the header file.

These functions shouldn't be defined in `ED_curves.hh` (especially `select_similar_layer`) since the functions are specific to grease pencil. If in the future, some of the functionality is used by the `Curves` object, we can still move some of them there. I would suggest to keep it simple and define all of them as static functions above this function (`select_similar_exec`). That also means we don't have to have code in the header file.
@ -287,0 +333,4 @@
grease_pencil, scene, selection_domain, type, threshold, "opacity");
break;
default:
throw std::invalid_argument("Undefined behavior for eSelectSimilar_Mode " + type);
Member

We don't use exceptions in Blender. There wouldn't be anything catching this. Either we handle the error directly, or we just assert. In this case no default case is necessary, because you're handling all the possible . If in the future we add a new case, then the compiler will complain about a missing case.

We don't use exceptions in Blender. There wouldn't be anything catching this. Either we handle the error directly, or we just assert. In this case no default case is necessary, because you're handling all the possible . If in the future we add a new case, then the compiler will complain about a missing case.
@ -287,0 +348,4 @@
{
ot->name = "Select Similar";
ot->idname = "GREASE_PENCIL_OT_select_similar";
ot->description = "Select all strokes with similar characteristics";
Member

This should probably say points instead of strokes since we're only allowing this operator to run in point selection mode.

This should probably say `points` instead of `strokes` since we're only allowing this operator to run in point selection mode.
@ -213,0 +227,4 @@
else if constexpr (std::is_same<T, std::string>::value) {
return "default";
}
throw std::invalid_argument("Undefined behavior for distance function for the used type");
Member

return T(0);

`return T(0);`
@ -213,0 +278,4 @@
return currentlySelectedValues;
}
template<typename T> static float distance(T first, T second)
Member

This should only handle the case for ColorGeometry4f and otherwise always use math::distance.

This should only handle the case for `ColorGeometry4f` and otherwise always use `math::distance`.
@ -213,0 +300,4 @@
VArray<T> attributes = *curves.attributes().lookup_or_default<T>(
attribute_id, ATTR_DOMAIN_POINT, default_for_lookup<T>());
const OffsetIndices points_by_curve = curves.points_by_curve();
bke::GSpanAttributeWriter selection = ensure_selection_attribute(
Member

Use const VArray<bool> point_selection = *curves.attributes().lookup_or_default<bool>(".selection", ATTR_DOMAIN_POINT, true); instead.

Use `const VArray<bool> point_selection = *curves.attributes().lookup_or_default<bool>(".selection", ATTR_DOMAIN_POINT, true);` instead.
Author
Contributor

I don't get how would this work... I'll publish my changes in a bit, so we can return on this point :)

I don't get how would this work... I'll publish my changes in a bit, so we can return on this point :)
@ -213,0 +305,4 @@
MutableSpan<bool> selection_typed = selection.span.typed<bool>();
// for now sequential impl, grain_size == 1
threading::parallel_for(curves.curves_range(), 1, [&](const IndexRange range) {
Member

This should be a loop over each editable point using editable_points.foreach_index([&](const int64_t point_i) { ... });

This should be a loop over each editable point using `editable_points.foreach_index([&](const int64_t point_i) { ... });`
@ -213,0 +313,4 @@
continue;
}
for (const int index : points.index_range()) {
Member

This should use IndexMask::from_predicate to build a mask using the distance condition and then index_mask::masked_fill to fill the selection attribute with true for the mask.

This should use `IndexMask::from_predicate` to build a mask using the distance condition and then `index_mask::masked_fill` to fill the selection attribute with `true` for the mask.
@ -213,0 +326,4 @@
selection.finish();
}
static void select_similar_layer(GreasePencil &grease_pencil,
Member

This function seems overly complicated. There is no need to store the layers in a set and loop twice.
Here is a simpler version (needs a paramter Object &object to account for locked materials).

const Array<MutableDrawingInfo> drawings = retrieve_editable_drawings(*scene, grease_pencil);
threading::parallel_for_each(drawings, [&](const MutableDrawingInfo &info) {
  IndexMaskMemory memory;
  const IndexMask editable_elements = retrieve_editable_elements(object, info.drawing, selection_domain, memory);
  if (editable_elements.is_empty()) {
     return;
  }
  const IndexMask selected_strokes = ed::curves::retrieve_selected_curves(curves, memory);
  ed::curves::select_all(
        info.drawing.strokes_for_write(), selectable_elements, selection_domain, SEL_SELECT);
});
This function seems overly complicated. There is no need to store the layers in a set and loop twice. Here is a simpler version (needs a paramter `Object &object` to account for locked materials). ``` const Array<MutableDrawingInfo> drawings = retrieve_editable_drawings(*scene, grease_pencil); threading::parallel_for_each(drawings, [&](const MutableDrawingInfo &info) { IndexMaskMemory memory; const IndexMask editable_elements = retrieve_editable_elements(object, info.drawing, selection_domain, memory); if (editable_elements.is_empty()) { return; } const IndexMask selected_strokes = ed::curves::retrieve_selected_curves(curves, memory); ed::curves::select_all( info.drawing.strokes_for_write(), selectable_elements, selection_domain, SEL_SELECT); }); ```
Author
Contributor

I have changed the code as suggested but to me it's not clear how this implementation works. Could you provide a more in detail explanation?

I have changed the code as suggested but to me it's not clear how this implementation works. Could you provide a more in detail explanation?
@ -213,0 +371,4 @@
std::string attribute_id)
{
using namespace blender::ed::greasepencil;
blender::Vector<blender::Set<T>> currentlySelectedValuesPerDrawing;
Member

This should be a blender::Array since we can retrieve the editable drawings before and initialize the array with drawings.size().

This should be a `blender::Array` since we can retrieve the editable drawings before and initialize the array with `drawings.size()`.
@ -213,0 +383,4 @@
Set<T> currentlySelectedValues = join_sets<T>(currentlySelectedValuesPerDrawing);
threading::parallel_for_each(drawings, [&](const MutableDrawingInfo &info) {
blender::ed::curves::select_with_similar_attribute<T>(info.drawing.strokes_for_write(),
Member

This should also receive an index mask for the points that are editable. See ed::greaspencil::retrieve_editable_points.

This should also receive an index mask for the points that are editable. See `ed::greaspencil::retrieve_editable_points`.
@ -549,6 +549,11 @@ typedef struct GreasePencil {
const blender::Map<int, int> &frame_number_destinations,
const blender::Map<int, GreasePencilFrame> &duplicate_frames);
void foreach_editable_drawing_in_layer_ex(
Member

This function is not needed. Use ed::greasepencil::retrieve_editable_drawings.

This function is not needed. Use `ed::greasepencil::retrieve_editable_drawings`.
Member

@Lorenzo-Carpaneto Are you planning on addressing the review? Otherwise we'll just take over at some point.

@Lorenzo-Carpaneto Are you planning on addressing the review? Otherwise we'll just take over at some point.
Author
Contributor

Hello @filedescriptor, I wanted to address the review and I tried contacting you in blender chat for further questions but I did not receive answers. I pinged you on those chats, so if you d like, I can move forward with this pr

Hello @filedescriptor, I wanted to address the review and I tried contacting you in blender chat for further questions but I did not receive answers. I pinged you on those chats, so if you d like, I can move forward with this pr
Lorenzo-Carpaneto added 12 commits 2024-04-14 20:10:48 +02:00
61ae1c5bc2 Merge branch 'main' into gpv3-select-similar
# Conflicts:
#	source/blender/blenkernel/intern/grease_pencil.cc
#	source/blender/editors/include/ED_curves.hh
634626c843 gpv3-select-similar
Solve merge issues
904f938f3f gpv3-select-similar
Use enum class instead of enum
c06363d1df gpv3-select-similar
Move static methods to other file and namespace.
7524ab79f4 gpv3-select-similar
Use math::distance if not ColorGeometry4f. Refactored some methods, leaving out unused params
7aaba681f8 gpv3-select-similar
Iterate over editable points
a7efe3a405 gpv3-select-similar
Using index masks to select things
cf4e550bc4 gpv3-select-similar
Use suggested implementation for select_similar_layer
05254155d8 gpv3-select-similar
Remove unused function
4dcd2ecb03 gpv3-select-similar
Use array instead of Vector
a0b7fde895 gpv3-select-similar
Move functions that don't belong in ED_curves.hh into grease_pencil_select.cc
Lorenzo-Carpaneto requested review from Falk David 2024-04-15 21:26:54 +02:00
Merge conflict checking is in progress. Try again in few moments.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u gpv3-select-similar:Lorenzo-Carpaneto-gpv3-select-similar
git checkout Lorenzo-Carpaneto-gpv3-select-similar
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
2 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#111410
No description provided.