GPv3: Vertex Group functions should make use of the VArray API #117753

New Issue

Falk David · 2024-02-02T13:41:57+01:00

Falk David commented

2024-02-02 13:41:57 +01:00

The functions in grease_pencil_vertex_groups.cc should make use of VArrayImpl_For_VertexWeights (see BKE_deform.hh).

bke::varray_for_deform_verts for reading
bke::varray_for_mutable_deform_verts for writing
Use bke::remove_defgroup_index to remove a group by its index.

The functions in `grease_pencil_vertex_groups.cc` should make use of `VArrayImpl_For_VertexWeights` (see `BKE_deform.hh`). * `bke::varray_for_deform_verts` for reading * `bke::varray_for_mutable_deform_verts` for writing * Use `bke::remove_defgroup_index` to remove a group by its index.

Falk David added the

Type

To Do

label 2024-02-02 13:41:58 +01:00

Falk David added this to the Grease Pencil project 2024-02-02 13:42:00 +01:00

Falk David commented

2024-02-02 13:53:52 +01:00

For assign_to_vertex_group here are my changes (since assign_to_vertex_group is dealing with selection, the code should probably be in the editors file, rather than in the kernel):

diff --git a/source/blender/blenkernel/intern/grease_pencil_vertex_groups.cc b/source/blender/blenkernel/intern/grease_pencil_vertex_groups.cc
index 236a65bb879..46b2d9f94a7 100644
--- a/source/blender/blenkernel/intern/grease_pencil_vertex_groups.cc
+++ b/source/blender/blenkernel/intern/grease_pencil_vertex_groups.cc
@@ -20,6 +20,8 @@
 
 #include "BLT_translation.h"
 
+#include "ED_curves.hh"
+
 namespace blender::bke::greasepencil {
 
 /* ------------------------------------------------------------------- */
@@ -55,6 +57,19 @@ void validate_drawing_vertex_groups(GreasePencil &grease_pencil)
   }
 }
 
+int ensure_vertex_group(const StringRef name, ListBase &vertex_group_names)
+{
+  int def_nr = BLI_findstringindex(&vertex_group_names, name.data(), offsetof(bDeformGroup, name));
+  if (def_nr < 0) {
+    bDeformGroup *defgroup = MEM_cnew<bDeformGroup>(__func__);
+    STRNCPY(defgroup->name, name.data());
+    BLI_addtail(&vertex_group_names, defgroup);
+    def_nr = BLI_listbase_count(&vertex_group_names) - 1;
+    BLI_assert(def_nr >= 0);
+  }
+  return def_nr;
+}
+
 void assign_to_vertex_group(GreasePencil &grease_pencil, const StringRef name, const float weight)
 {
   for (GreasePencilDrawingBase *base : grease_pencil.drawings()) {
@@ -69,29 +84,20 @@ void assign_to_vertex_group(GreasePencil &grease_pencil, const StringRef name, c
     const VArray<bool> selection = *attributes.lookup_or_default<bool>(
         ".selection", bke::AttrDomain::Point, true);
 
-    /* Look for existing group, otherwise lazy-initialize if any vertex is selected. */
-    int def_nr = BLI_findstringindex(
-        &vertex_group_names, name.data(), offsetof(bDeformGroup, name));
-
-    const MutableSpan<MDeformVert> dverts = curves.deform_verts_for_write();
-    for (const int i : dverts.index_range()) {
-      if (selection[i]) {
-        /* Lazily add the vertex group if any vertex is selected. */
-        if (def_nr < 0) {
-          bDeformGroup *defgroup = MEM_cnew<bDeformGroup>(__func__);
-          STRNCPY(defgroup->name, name.data());
-
-          BLI_addtail(&vertex_group_names, defgroup);
-          def_nr = BLI_listbase_count(&vertex_group_names) - 1;
-          BLI_assert(def_nr >= 0);
-        }
-
-        MDeformWeight *dw = BKE_defvert_ensure_index(&dverts[i], def_nr);
-        if (dw) {
-          dw->weight = weight;
+    if (!ed::curves::has_anything_selected(curves)) {
+      continue;
+    }
+    
+    int def_nr = ensure_vertex_group(vertex_group_names, name);
+    VMutableArray<float> dverts = bke::varray_for_mutable_deform_verts(
+        curves.deform_verts_for_write(), def_nr);
+    threading::parallel_for(curves.points_range(), 1024, [&](const IndexRange range) {
+      for (const int point_i : range) {
+        if (selection[point_i]) {
+          dverts[point_i] = weight;
         }
       }
-    }
+    });
   }
 }

For `assign_to_vertex_group` here are my changes (since `assign_to_vertex_group` is dealing with selection, the code should probably be in the editors file, rather than in the kernel): ```diff diff --git a/source/blender/blenkernel/intern/grease_pencil_vertex_groups.cc b/source/blender/blenkernel/intern/grease_pencil_vertex_groups.cc index 236a65bb879..46b2d9f94a7 100644 --- a/source/blender/blenkernel/intern/grease_pencil_vertex_groups.cc +++ b/source/blender/blenkernel/intern/grease_pencil_vertex_groups.cc @@ -20,6 +20,8 @@ #include "BLT_translation.h" +#include "ED_curves.hh" + namespace blender::bke::greasepencil { /* ------------------------------------------------------------------- */ @@ -55,6 +57,19 @@ void validate_drawing_vertex_groups(GreasePencil &grease_pencil) } } +int ensure_vertex_group(const StringRef name, ListBase &vertex_group_names) +{ + int def_nr = BLI_findstringindex(&vertex_group_names, name.data(), offsetof(bDeformGroup, name)); + if (def_nr < 0) { + bDeformGroup *defgroup = MEM_cnew<bDeformGroup>(__func__); + STRNCPY(defgroup->name, name.data()); + BLI_addtail(&vertex_group_names, defgroup); + def_nr = BLI_listbase_count(&vertex_group_names) - 1; + BLI_assert(def_nr >= 0); + } + return def_nr; +} + void assign_to_vertex_group(GreasePencil &grease_pencil, const StringRef name, const float weight) { for (GreasePencilDrawingBase *base : grease_pencil.drawings()) { @@ -69,29 +84,20 @@ void assign_to_vertex_group(GreasePencil &grease_pencil, const StringRef name, c const VArray<bool> selection = *attributes.lookup_or_default<bool>( ".selection", bke::AttrDomain::Point, true); - /* Look for existing group, otherwise lazy-initialize if any vertex is selected. */ - int def_nr = BLI_findstringindex( - &vertex_group_names, name.data(), offsetof(bDeformGroup, name)); - - const MutableSpan<MDeformVert> dverts = curves.deform_verts_for_write(); - for (const int i : dverts.index_range()) { - if (selection[i]) { - /* Lazily add the vertex group if any vertex is selected. */ - if (def_nr < 0) { - bDeformGroup *defgroup = MEM_cnew<bDeformGroup>(__func__); - STRNCPY(defgroup->name, name.data()); - - BLI_addtail(&vertex_group_names, defgroup); - def_nr = BLI_listbase_count(&vertex_group_names) - 1; - BLI_assert(def_nr >= 0); - } - - MDeformWeight *dw = BKE_defvert_ensure_index(&dverts[i], def_nr); - if (dw) { - dw->weight = weight; + if (!ed::curves::has_anything_selected(curves)) { + continue; + } + + int def_nr = ensure_vertex_group(vertex_group_names, name); + VMutableArray<float> dverts = bke::varray_for_mutable_deform_verts( + curves.deform_verts_for_write(), def_nr); + threading::parallel_for(curves.points_range(), 1024, [&](const IndexRange range) { + for (const int point_i : range) { + if (selection[point_i]) { + dverts[point_i] = weight; } } - } + }); } } ```

Hans Goudey commented

2024-02-02 15:44:10 +01:00

For that matter, it might be even nicer to use the attribute API here. Once the vertex group is created, it can be accessed as a float attribute on the point domain.

Falk David commented

2024-02-02 15:56:35 +01:00

@HooglyBoogly True! Would be even better.

Lukas and I noticed that if the user creates a vertex group that has the same name as some attribute, that it would first find the attribute.
I guess the only solution to that would be to make sure that when a vertex group is created, that it doesn't have the same name as any other attribute. Not sure how this would work if the attribute was added afterwards though...

@HooglyBoogly True! Would be even better. Lukas and I noticed that if the user creates a vertex group that has the same name as some attribute, that it would first find the attribute. I guess the only solution to that would be to make sure that when a vertex group is created, that it doesn't have the same name as any other attribute. Not sure how this would work if the attribute was added afterwards though...

Pratik Borhade commented

2024-02-05 11:33:31 +01:00

Oh there is task already, didn't know that. Perhaps unrelated, I tried some changes (for simplification) during weekend while understanding the new vgroup structure). How about using Indexmask selection instead of looping all points in threading::parellel_for?:

Code Changes

diff --git a/source/blender/blenkernel/intern/grease_pencil_vertex_groups.cc b/source/blender/blenkernel/intern/grease_pencil_vertex_groups.cc
index 236a65bb879..101c3fa8a84 100644
--- a/source/blender/blenkernel/intern/grease_pencil_vertex_groups.cc
+++ b/source/blender/blenkernel/intern/grease_pencil_vertex_groups.cc
@@ -13,6 +13,7 @@
 #include "BLI_string.h"
 #include "BLI_string_utils.hh"

+#include "../editors\include\ED_curves.hh"
 #include "BKE_curves.hh"
 #include "BKE_deform.hh"
 #include "BKE_grease_pencil.hh"
@@ -66,32 +67,34 @@ void assign_to_vertex_group(GreasePencil &grease_pencil, const StringRef name, c
     ListBase &vertex_group_names = curves.vertex_group_names;

     const bke::AttributeAccessor attributes = curves.attributes();
-    const VArray<bool> selection = *attributes.lookup_or_default<bool>(
-        ".selection", bke::AttrDomain::Point, true);
+
+    IndexMaskMemory memory;
+    IndexMask selection = ed::curves::retrieve_selected_points(curves, memory);
+    if (selection.is_empty()) {
+      continue;
+    }

     /* Look for existing group, otherwise lazy-initialize if any vertex is selected. */
     int def_nr = BLI_findstringindex(
         &vertex_group_names, name.data(), offsetof(bDeformGroup, name));

     const MutableSpan<MDeformVert> dverts = curves.deform_verts_for_write();
-    for (const int i : dverts.index_range()) {
-      if (selection[i]) {
-        /* Lazily add the vertex group if any vertex is selected. */
-        if (def_nr < 0) {
-          bDeformGroup *defgroup = MEM_cnew<bDeformGroup>(__func__);
-          STRNCPY(defgroup->name, name.data());
-
-          BLI_addtail(&vertex_group_names, defgroup);
-          def_nr = BLI_listbase_count(&vertex_group_names) - 1;
-          BLI_assert(def_nr >= 0);
-        }

-        MDeformWeight *dw = BKE_defvert_ensure_index(&dverts[i], def_nr);
-        if (dw) {
-          dw->weight = weight;
-        }
+    selection.foreach_index([&](int point_i) {
+      if (def_nr < 0) {
+        bDeformGroup *defgroup = MEM_cnew<bDeformGroup>(__func__);
+        STRNCPY(defgroup->name, name.data());
+
+        BLI_addtail(&vertex_group_names, defgroup);
+        def_nr = BLI_listbase_count(&vertex_group_names) - 1;
+        BLI_assert(def_nr >= 0);
       }
-    }
+
+      MDeformWeight *dw = BKE_defvert_ensure_index(&dverts[point_i], def_nr);
+      if (dw) {
+        dw->weight = weight;
+      }
+    });
   }
 }

Oh there is task already, didn't know that. Perhaps unrelated, I tried some changes (for simplification) during weekend while understanding the new vgroup structure). How about using `Indexmask selection` instead of looping all points in `threading::parellel_for`?: <details> <summary> Code Changes </summary> ```Diff diff --git a/source/blender/blenkernel/intern/grease_pencil_vertex_groups.cc b/source/blender/blenkernel/intern/grease_pencil_vertex_groups.cc index 236a65bb879..101c3fa8a84 100644 --- a/source/blender/blenkernel/intern/grease_pencil_vertex_groups.cc +++ b/source/blender/blenkernel/intern/grease_pencil_vertex_groups.cc @@ -13,6 +13,7 @@ #include "BLI_string.h" #include "BLI_string_utils.hh" +#include "../editors\include\ED_curves.hh" #include "BKE_curves.hh" #include "BKE_deform.hh" #include "BKE_grease_pencil.hh" @@ -66,32 +67,34 @@ void assign_to_vertex_group(GreasePencil &grease_pencil, const StringRef name, c ListBase &vertex_group_names = curves.vertex_group_names; const bke::AttributeAccessor attributes = curves.attributes(); - const VArray<bool> selection = *attributes.lookup_or_default<bool>( - ".selection", bke::AttrDomain::Point, true); + + IndexMaskMemory memory; + IndexMask selection = ed::curves::retrieve_selected_points(curves, memory); + if (selection.is_empty()) { + continue; + } /* Look for existing group, otherwise lazy-initialize if any vertex is selected. */ int def_nr = BLI_findstringindex( &vertex_group_names, name.data(), offsetof(bDeformGroup, name)); const MutableSpan<MDeformVert> dverts = curves.deform_verts_for_write(); - for (const int i : dverts.index_range()) { - if (selection[i]) { - /* Lazily add the vertex group if any vertex is selected. */ - if (def_nr < 0) { - bDeformGroup *defgroup = MEM_cnew<bDeformGroup>(__func__); - STRNCPY(defgroup->name, name.data()); - - BLI_addtail(&vertex_group_names, defgroup); - def_nr = BLI_listbase_count(&vertex_group_names) - 1; - BLI_assert(def_nr >= 0); - } - MDeformWeight *dw = BKE_defvert_ensure_index(&dverts[i], def_nr); - if (dw) { - dw->weight = weight; - } + selection.foreach_index([&](int point_i) { + if (def_nr < 0) { + bDeformGroup *defgroup = MEM_cnew<bDeformGroup>(__func__); + STRNCPY(defgroup->name, name.data()); + + BLI_addtail(&vertex_group_names, defgroup); + def_nr = BLI_listbase_count(&vertex_group_names) - 1; + BLI_assert(def_nr >= 0); } - } + + MDeformWeight *dw = BKE_defvert_ensure_index(&dverts[point_i], def_nr); + if (dw) { + dw->weight = weight; + } + }); } } ``` </details>

Falk David commented

2024-02-05 11:51:07 +01:00

@PratikPB2123 Generally, code in blenkernel shouldn't call code in the editors. This is why I suggested to move this logic into a static function that's called by assign_to_vertex_group. And this static function would e.g. ensure that the vertex group exists, which can be a kernel function.

@PratikPB2123 Generally, code in `blenkernel` shouldn't call code in the editors. This is why I suggested to move this logic into a static function that's called by `assign_to_vertex_group`. And this static function would e.g. ensure that the vertex group exists, which can be a kernel function.

Sietse Brouwer commented

2024-02-15 11:01:36 +01:00

And I think that it's better assign_to_vertex_group will use retrieve_visible_drawings() instead of assigning to all drawings. I checked, that's the behaviour in GPv2 too.

And I think that it's better `assign_to_vertex_group` will use `retrieve_visible_drawings()` instead of assigning to _all_ drawings. I checked, that's the behaviour in GPv2 too.

👍 1

Falk David referenced this issue

2024-02-15 15:09:27 +01:00

GPv3: Vertex Weight Angle modifier #117846

Falk David commented

2024-02-15 15:26:18 +01:00

@HooglyBoogly I'm wondering if we can make all of this easier by adding a try_create function to CurvesVertexGroupsAttributeProvider. That should essentially replace the ensure_vertex_group no?

@HooglyBoogly I'm wondering if we can make all of this easier by adding a `try_create` function to `CurvesVertexGroupsAttributeProvider`. That should essentially replace the `ensure_vertex_group` no?

Hans Goudey commented

2024-02-15 15:36:38 +01:00

We can't do that, since float attributes should be created by default in geometry nodes (and elsewhere in the attribute API) instead of vertex groups. Vertex groups always have to be created explicitly outside of the attribute API, for now anyway.

👍 1

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

GPv3: Vertex Group functions should make use of the VArray API #117753

GPv3: Vertex Group functions should make use of the `VArray` API #117753