blender
/
blender
134
398
Fork 580
Code Issues 6.3k Pull Requests 756 Projects 19 Wiki Activity

GPv3: Basic vertex group operators #117476

Merged
Lukas Tönne merged 24 commits from LukasTonne/blender:gp3-vertex-group-operators into main 2024-01-31 17:46:07 +01:00
Conversation 20 Commits 24 Files Changed 10 +329 -17
Lukas Tönne commented 2024-01-24 13:13:36 +01:00
Member
Copy Link

Adds vertex groups and basic operator support to the GreasePencil data block.

Vertex groups in the GreasePencil ID are used as the source of truth for vertex groups names and ordering in the UI. Individual drawings also have vertex group lists, but they should not be modified directly by users or the API. The main purpose of storing vertex group names in in a drawing's CurveGeometry is to make it self-contained, so that vertex weights can be associated with names without requiring the GreasePencil parent data.

Vertex group operators are implemented generically for some ID types. Grease Pencil needs its own handling in these operators. After manipulating vertex_group_names the validate_drawing_vertex_groups utility function should be called to ensure that drawings only contain a true subset of the GreasePencil data block.

Operators for assigning/removing/selecting/deselecting vertices are also implemented here. To avoid putting grease pencil logic into the generic object_deform.cc file a number of utility functions have been added in BKE_grease_pencil_vgroup.hh.

Fixes #117337

Adds vertex groups and basic operator support to the `GreasePencil` data block. Vertex groups in the `GreasePencil` ID are used as the source of truth for vertex groups names and ordering in the UI. Individual drawings also have vertex group lists, but they should not be modified directly by users or the API. The main purpose of storing vertex group names in in a drawing's `CurveGeometry` is to make it self-contained, so that vertex weights can be associated with names without requiring the `GreasePencil` parent data. Vertex group operators are implemented generically for some ID types. Grease Pencil needs its own handling in these operators. After manipulating `vertex_group_names` the `validate_drawing_vertex_groups` utility function should be called to ensure that drawings only contain a true subset of the `GreasePencil` data block. Operators for assigning/removing/selecting/deselecting vertices are also implemented here. To avoid putting grease pencil logic into the generic `object_deform.cc` file a number of utility functions have been added in `BKE_grease_pencil_vgroup.hh`. Fixes #117337
Lukas Tönne added 4 commits 2024-01-24 13:13:46 +01:00
Falk David added this to the Grease Pencil project 2024-01-24 14:04:22 +01:00
Brecht Van Lommel commented 2024-01-24 21:07:37 +01:00
Owner
Copy Link

The design choice here also affects how to deal with attributes across layers more generally. Since I imagine when you are doing vertex color painting, you also want to select from a single list that aggregates attributes from all layers.

I can't immediately think of a better alternative to this mechanism, it seems ok.

I would expect this to be purely a runtime cache, not saved to the .blend file. And rather than syncing after modifying drawings, it would clear the cache and generate it when needed?

The design choice here also affects how to deal with attributes across layers more generally. Since I imagine when you are doing vertex color painting, you also want to select from a single list that aggregates attributes from all layers. I can't immediately think of a better alternative to this mechanism, it seems ok. I would expect this to be purely a runtime cache, not saved to the .blend file. And rather than syncing after modifying drawings, it would clear the cache and generate it when needed?
Lukas Tönne commented 2024-01-25 00:49:42 +01:00
Author
Member
Copy Link

I would expect this to be purely a runtime cache, not saved to the .blend file. And rather than syncing after modifying drawings, it would clear the cache and generate it when needed?

Reason for storing the list in DNA is that it also encodes the global order of vertex groups. Otherwise adding or removing vertices from groups can change the order of vertex groups:

  1. Two drawings with groups D1=(a, b) and D2=(a, b)
  2. Vertex group list constructed as (a, b)
  3. Remove all "a" weights from D1: D1=(b), D2=(a, b)
  4. Now vertex group order is (b, a)

In addition to the stable order it's also useful to have a single consistent lock_weight flag for vertex groups (this maintains relative weights in a group during edits).

> I would expect this to be purely a runtime cache, not saved to the .blend file. And rather than syncing after modifying drawings, it would clear the cache and generate it when needed? Reason for storing the list in DNA is that it also encodes the global order of vertex groups. Otherwise adding or removing vertices from groups can change the order of vertex groups: 1. Two drawings with groups D1=(a, b) and D2=(a, b) 2. Vertex group list constructed as (a, b) 3. Remove all "a" weights from D1: D1=(b), D2=(a, b) 4. Now vertex group order is (b, a) In addition to the stable order it's also useful to have a single consistent `lock_weight` flag for vertex groups (this maintains relative weights in a group during edits).
Sietse Brouwer commented 2024-01-25 00:57:04 +01:00
Member
Copy Link

Sorry, we were typing at the same moment. Didn't see your post until I posted mine...

Perhaps premature questions, since this is a first WIP, but:

  • The vertex groups must be propagated when new Drawings are created. I don't see anything for that yet in the PR, but I expect some mechanism for that will be added to GreasePencil::add_empty_drawings, add_duplicate_drawings etc., right?
  • How will the order of vertex groups in the 'global' list be ensured?

It's not entirely clear to me if there can be an 'unsynced' state. Is it possible that at any moment a Drawing can have a different list of vertex groups than the other Drawings in the scene? When the vertex_group_names are always in sync, you could skip the iteration over all drawings in sync_vertex_groups_from_layers.

Sorry, we were typing at the same moment. Didn't see your post until I posted mine... Perhaps premature questions, since this is a first WIP, but: - The vertex groups must be propagated when new `Drawing`s are created. I don't see anything for that yet in the PR, but I expect some mechanism for that will be added to `GreasePencil::add_empty_drawings`, `add_duplicate_drawings` etc., right? - How will the order of vertex groups in the 'global' list be ensured? It's not entirely clear to me if there can be an 'unsynced' state. Is it possible that at any moment a `Drawing` can have a different list of vertex groups than the other `Drawing`s in the scene? When the `vertex_group_names` are always in sync, you could skip the iteration over all drawings in `sync_vertex_groups_from_layers`.
Falk David commented 2024-01-25 10:55:48 +01:00
Member
Copy Link

@SietseB We want to move away from the idea that "every drawing always has the same vertex groups" which is similar to the idea that "all drawings are one geometry in grease pencil".

We want to treat vertex groups as attributes. And yes, different drawings can have different attributes. This might seem like an issue, because "what happens if I want to access a vertex group on a drawing that doesn't have it?!" and the awnser is simple: it returns a virtual array with a default value for all the elements (e.g. see VArray::ForSingle). When writing to a vertex group of a drawing, then it will actually create it.

So we can think of this global list on the grease pencil as the list that tells us:

  1. which vertex groups are availible
  2. what is the order of the vertex groups (something needs to decide on the order, so we can show it consistently in the UI)
@SietseB We want to move away from the idea that "every drawing always has the same vertex groups" which is similar to the idea that "all drawings are one geometry in grease pencil". We want to treat vertex groups as attributes. And yes, different drawings can have different attributes. This might seem like an issue, because "what happens if I want to access a vertex group on a drawing that doesn't have it?!" and the awnser is simple: it returns a virtual array with a default value for all the elements (e.g. see `VArray::ForSingle`). When writing to a vertex group of a drawing, then it will actually create it. So we can think of this global list on the grease pencil as the list that tells us: 1. which vertex groups are availible 2. what is the order of the vertex groups (something needs to decide on the order, so we can show it consistently in the UI)
Lukas Tönne commented 2024-01-25 11:03:21 +01:00
Author
Member
Copy Link

The vertex groups must be propagated when new Drawings are created.

Yes in those cases the groups list also needs to be synced. Although new drawings probably don't have vertex groups by default and adding groups is separate operation. Vertex groups in drawings don't have to match the groups list in GreasePencil, they can be a subset and don't even have to follow the same order.

How will the order of vertex groups in the 'global' list be ensured?

By writing to blend files, and by keeping the order unchanged, unless users actively change it using vertex group operators.

It's not entirely clear to me if there can be an 'unsynced' state. Is it possible that at any moment a Drawing can have a different list of vertex groups than the other Drawings in the scene?

By "sync" i will mean copying the combined set of vertex groups from drawings to the vertex groups in GreasePencil. The individual drawings can have different subsets of vertex groups of the GreasePencil list, they can have all of the groups or none or just a few. The GreasePencil list functions like a registry for the Drawing groups.

> The vertex groups must be propagated when new Drawings are created. Yes in those cases the groups list also needs to be synced. Although new drawings probably don't have vertex groups by default and adding groups is separate operation. Vertex groups in drawings don't have to match the groups list in `GreasePencil`, they can be a subset and don't even have to follow the same order. > How will the order of vertex groups in the 'global' list be ensured? By writing to blend files, and by keeping the order unchanged, unless users actively change it using vertex group operators. > It's not entirely clear to me if there can be an 'unsynced' state. Is it possible that at any moment a Drawing can have a different list of vertex groups than the other Drawings in the scene? By "sync" i will mean copying the combined set of vertex groups from drawings to the vertex groups in `GreasePencil`. The individual drawings can have different subsets of vertex groups of the `GreasePencil` list, they can have all of the groups or none or just a few. The `GreasePencil` list functions like a registry for the `Drawing` groups.
hamza.SMA commented 2024-01-25 11:30:42 +01:00
First-time contributor
Copy Link

I want to mention a behavior that can be hard to notice about vertex group , to be aware of it , when we duplicate / copy / interpolate points that are in a vertex group , all those generated point will be automatically assigned to this vertex group, hope i'm to be helpful here.

I want to mention a behavior that can be hard to notice about vertex group , to be aware of it , when we duplicate / copy / interpolate points that are in a vertex group , all those generated point will be automatically assigned to this vertex group, hope i'm to be helpful here.
Lukas Tönne commented 2024-01-25 11:51:25 +01:00
Author
Member
Copy Link

I want to mention a behavior that can be hard to notice about vertex group , to be aware of it , when we duplicate / copy / interpolate points that are in a vertex group , all those generated point will be automatically assigned to this vertex group, hope i'm to be helpful here.

This should work just fine. When points are copied or interpolated within a drawing the vertex groups already exist there. The process is already covered by CurvesGeometry functions and does not need to involve the GreasePencil data block.

> I want to mention a behavior that can be hard to notice about vertex group , to be aware of it , when we duplicate / copy / interpolate points that are in a vertex group , all those generated point will be automatically assigned to this vertex group, hope i'm to be helpful here. This should work just fine. When points are copied or interpolated _within a drawing_ the vertex groups already exist there. The process is already covered by `CurvesGeometry` functions and does not need to involve the `GreasePencil` data block.
👍 1
Brecht Van Lommel commented 2024-01-25 11:57:14 +01:00
Owner
Copy Link

@SietseB We want to move away from the idea that "every drawing always has the same vertex groups" which is similar to the idea that "all drawings are one geometry in grease pencil".

We want to treat vertex groups as attributes. And yes, different drawings can have different attributes. This might seem like an issue, because "what happens if I want to access a vertex group on a drawing that doesn't have it?!" and the awnser is simple: it returns a virtual array with a default value for all the elements (e.g. see VArray::ForSingle). When writing to a vertex group of a drawing, then it will actually create it.

This is confusing to me. In the first paragraph you say that drawings do not share vertex groups,. But in the second you seem to say that it works as if they are shared by all drawings, at least from a user point of view.

Can it be clarified how this will work on a user level? Ignoring for a moment the implementation.

  • In the properties editor, will there be a single vertex groups list panel for the entire datablock, with add and remove functionality? Will there be a list per drawing too, of is any potential distinction here hidden?
  • In geometry nodes, will it work as if all drawings share the same vertex groups, or is the distinction visible here? For example if I have a node that reads from a vertex group, in the properties editor it shows as existing for the whole datablock, could it report an error if it does not exist for a particular drawing?
  • If attributes perhaps will work similarly, will it be possible to have attributes with the same name but different types on different drawings?
> @SietseB We want to move away from the idea that "every drawing always has the same vertex groups" which is similar to the idea that "all drawings are one geometry in grease pencil". > > We want to treat vertex groups as attributes. And yes, different drawings can have different attributes. This might seem like an issue, because "what happens if I want to access a vertex group on a drawing that doesn't have it?!" and the awnser is simple: it returns a virtual array with a default value for all the elements (e.g. see `VArray::ForSingle`). When writing to a vertex group of a drawing, then it will actually create it. This is confusing to me. In the first paragraph you say that drawings do not share vertex groups,. But in the second you seem to say that it works as if they are shared by all drawings, at least from a user point of view. Can it be clarified how this will work on a user level? Ignoring for a moment the implementation. * In the properties editor, will there be a single vertex groups list panel for the entire datablock, with add and remove functionality? Will there be a list per drawing too, of is any potential distinction here hidden? * In geometry nodes, will it work as if all drawings share the same vertex groups, or is the distinction visible here? For example if I have a node that reads from a vertex group, in the properties editor it shows as existing for the whole datablock, could it report an error if it does not exist for a particular drawing? * If attributes perhaps will work similarly, will it be possible to have attributes with the same name but different types on different drawings?
Falk David commented 2024-01-25 12:34:52 +01:00
Member
Copy Link

@brecht Right, we want vertex groups to be stored per drawing, but expose them to the user at the object-data level. Let me explain the reasoning first:

  • We don't want to break the workflow when working with with vertex groups compared to GPv2.
  • We want to make sure vertex groups are moving towards attributes (because that's where they are headed for other objects as well).

Now to awnser your questions:

  • Yes, there will be a single list of vertex groups at the object-data level. We want to make sure it stays the same compared to GPv2. We had discussions about "per drawing" settings, but I don't think we should make that part of the GPv3 project. There are many design questions that we would have to figure out and there is not enough time.
  • No, in geometry nodes, vertex groups are just another attribute on a drawing. The "Named Attribute" node has an "Exists" output that would be false, if the vertex group doesn't exist on a drawing.
  • I guess this is not only a question for grease pencil. We can already have this situation with e.g. meshes. From a quick test, it looks like if I join two geometries with attributes with the same name but different types, geometry nodes will combine them into one attribute (by converting one type into the other for all elements, using some heuristic)
@brecht Right, we want vertex groups to be stored per drawing, but expose them to the user at the object-data level. Let me explain the reasoning first: * We don't want to break the workflow when working with with vertex groups compared to GPv2. * We want to make sure vertex groups are moving towards attributes (because that's where they are headed for other objects as well). Now to awnser your questions: * Yes, there will be a single list of vertex groups at the object-data level. We want to make sure it stays the same compared to GPv2. We had discussions about "per drawing" settings, but I don't think we should make that part of the GPv3 project. There are many design questions that we would have to figure out and there is not enough time. * No, in geometry nodes, vertex groups are just another attribute on a drawing. The "Named Attribute" node has an "Exists" output that would be false, if the vertex group doesn't exist on a drawing. * I guess this is not only a question for grease pencil. We can already have this situation with e.g. meshes. From a quick test, it looks like if I join two geometries with attributes with the same name but different types, geometry nodes will combine them into one attribute (by converting one type into the other for all elements, using some heuristic)
Sietse Brouwer commented 2024-01-25 14:55:46 +01:00
Member
Copy Link

Thanks for all the answers.

So, for my own understanding: there are two sources of truth. The vertex groups in Drawings are the source of truth for the existence of the groups. And the GreasePencil list is the source of truth for the (UI) order of the groups.

What about this edge case: I have one keyframe (Drawing), I create a vertex group, I create a second keyframe (which doesn't get the vertex group added by default, as I understand it) and I delete the first keyframe. Now my vertex group is gone. From a UX perspective I would say this is undesirable, but is this the price we have to pay for this design?

Thanks for all the answers. So, for my own understanding: there are two sources of truth. The vertex groups in `Drawing`s are the source of truth for the _existence_ of the groups. And the `GreasePencil` list is the source of truth for the (UI) _order_ of the groups. What about this edge case: I have one keyframe (`Drawing`), I create a vertex group, I create a second keyframe (which doesn't get the vertex group added by default, as I understand it) and I delete the first keyframe. Now my vertex group is gone. From a UX perspective I would say this is undesirable, but is this the price we have to pay for this design?
Falk David commented 2024-01-25 15:10:41 +01:00
Member
Copy Link

@SietseB We don't need to remove the vertex group name from the list. We could keep it if we wanted to. Wouldn't hurt and I guess would be consistent with the other object types.

@SietseB We don't need to remove the vertex group name from the list. We could keep it if we wanted to. Wouldn't hurt and I guess would be consistent with the other object types.
Sietse Brouwer commented 2024-01-25 15:38:25 +01:00
Member
Copy Link

No, we don't need to, but as soon as sync_vertex_groups_from_layers() is called somewhere, the vertex group will be gone. So that's pretty tricky...

No, we don't need to, but as soon as `sync_vertex_groups_from_layers()` is called somewhere, the vertex group will be gone. So that's pretty tricky...
Sietse Brouwer reviewed 2024-01-25 15:39:13 +01:00
Sietse Brouwer left a comment
Member
Copy Link

A few things I noticed when scanning the code.

A few things I noticed when scanning the code.
source/blender/blenkernel/intern/grease_pencil.cc Outdated
@ -2420,0 +2509,4 @@
const bDeformGroup &defgroup,
blender::StringRef name)
{
for (const GreasePencilDrawingBase *base : grease_pencil.drawings()) {
Sietse Brouwer commented 2024-01-25 15:14:53 +01:00
Member
Copy Link

You could iterate over grease_pencil.vertex_group_names here? That's less data than all the drawings in a scene.

You could iterate over `grease_pencil.vertex_group_names` here? That's less data than all the drawings in a scene.
LukasTonne marked this conversation as resolved
source/blender/blenkernel/intern/grease_pencil.cc Outdated
@ -2420,0 +2567,4 @@
BLI_addtail(&this->vertex_group_names, defgroup);
for (GreasePencilDrawingBase *base : this->drawings()) {
Sietse Brouwer commented 2024-01-25 15:30:46 +01:00
Member
Copy Link

It seems a bit random to me that all existing drawings get the vertex group added here, but newly inserted keyframes/drawings after this don't (as I understood from the conversation). There is no real reasoning behind it then, which Drawing contains the vertex group and which not.

It seems a bit random to me that all existing drawings get the vertex group added here, but newly inserted keyframes/drawings after this don't (as I understood from the conversation). There is no real reasoning behind it then, which `Drawing` contains the vertex group and which not.
LukasTonne marked this conversation as resolved
source/blender/blenkernel/intern/grease_pencil.cc Outdated
@ -2420,0 +2581,4 @@
BLI_addtail(&curves.vertex_group_names, drawing_defgroup);
}
this->sync_vertex_groups_from_layers();
Sietse Brouwer commented 2024-01-25 15:32:03 +01:00
Member
Copy Link

I would say this call is redundant.

I would say this call is redundant.
LukasTonne marked this conversation as resolved
Falk David commented 2024-01-25 15:53:10 +01:00
Member
Copy Link

@SietseB What I meant was that sync_vertex_groups_from_layers doesn't need to remove vertex groups that are unused.

@SietseB What I meant was that `sync_vertex_groups_from_layers` doesn't need to remove vertex groups that are unused.
Brecht Van Lommel commented 2024-01-25 16:20:55 +01:00
Owner
Copy Link

I think users managing vertex groups and attributes per drawing is something we should not add even if we had the time. I don't think the UI and conceptual complexity is worth it. So I would avoid complicating the design for a use case that is not going to happen.

Still in practical terms, the implementation might end up similar anyway. If you want to avoid memory usage for attributes that only have default values on some drawings, then with current data structures indeed you would need to not have the custom data layer at all on them.

For vertex groups there is not as much reason since it doesn't save memory to have it per drawing. My guess would be that storing the list of vertex group names only on the datablock would give the simplest implementation, but I might be wrong about that.

I think users managing vertex groups and attributes per drawing is something we should not add even if we had the time. I don't think the UI and conceptual complexity is worth it. So I would avoid complicating the design for a use case that is not going to happen. Still in practical terms, the implementation might end up similar anyway. If you want to avoid memory usage for attributes that only have default values on some drawings, then with current data structures indeed you would need to not have the custom data layer at all on them. For vertex groups there is not as much reason since it doesn't save memory to have it per drawing. My guess would be that storing the list of vertex group names only on the datablock would give the simplest implementation, but I might be wrong about that.
Sietse Brouwer commented 2024-01-25 17:51:23 +01:00
Member
Copy Link

@filedescriptor Ah, right. So when the GreasePencil list can contain unused vertex groups, then that list is basically the only source of truth.
You can omit the whole sync_vertex_groups_from_layers mechanism then. And only adding a vertex group as an attribute to the Drawing when there are actually stroke points part of that group (e.g. by weight painting or through a node in GN).

@filedescriptor Ah, right. So when the `GreasePencil` list can contain unused vertex groups, then that list is basically the only source of truth. You can omit the whole `sync_vertex_groups_from_layers` mechanism then. And only adding a vertex group as an attribute to the `Drawing` when there are actually stroke points part of that group (e.g. by weight painting or through a node in GN).
Lukas Tönne commented 2024-01-30 12:07:27 +01:00
Author
Member
Copy Link

Thanks for all the comments, i think we've arrived at a feasible plan:

  • Vertex group names list in GreasePencil is the main source of truth for which vertex groups exist and what their order is.
  • Vertex group lists in drawings should be subsets of the GreasePencil list.
    Drawings have their own vertex groups to be self-contained: we can relate vertex groups to e.g. armatures without having to refer to the GreasePencil data block.
  • API and UI exist only for the GreasePencil data block.
    Changing vertex groups applies to the GreasePencil list first, then drawings are updated to match (remove invalid groups).
  • Adding vertex groups to drawings directly must be considered volatile: The group can be removed any time if it doesn't exist in the GreasePencil data.
    If necessary we can add a "sync" function to update GreasePencil from its drawings, but i consider that not recommended.
    Note: Using the attributes API of drawings does not allow adding new vertex groups, only changing existing groups. This is "safe" wrt. the proposed design.
Thanks for all the comments, i think we've arrived at a feasible plan: - Vertex group names list in `GreasePencil` is the main source of truth for which vertex groups exist and what their order is. - Vertex group lists in drawings should be subsets of the `GreasePencil` list. Drawings have their own vertex groups to be self-contained: we can relate vertex groups to e.g. armatures without having to refer to the `GreasePencil` data block. - API and UI exist **only** for the `GreasePencil` data block. Changing vertex groups applies to the `GreasePencil` list first, then drawings are updated to match (remove invalid groups). - Adding vertex groups to drawings directly must be considered volatile: The group can be removed any time if it doesn't exist in the `GreasePencil` data. If necessary we can add a "sync" function to update `GreasePencil` from its drawings, but i consider that not recommended. Note: Using the `attributes` API of drawings does not allow adding new vertex groups, only changing existing groups. This is "safe" wrt. the proposed design.
👍 1
Lukas Tönne added 1 commit 2024-01-30 13:02:03 +01:00
217247a295 Make GreasePencil vertex_group_names the main source of truth. 
Drawing vertex groups should get validated after operators to ensure
they form a proper subset of the GreasePencil vertex groups.
Lukas Tönne added 1 commit 2024-01-30 14:58:18 +01:00
Lukas Tönne added 1 commit 2024-01-30 15:03:41 +01:00
Lukas Tönne added 1 commit 2024-01-30 16:57:48 +01:00
Lukas Tönne added 1 commit 2024-01-30 17:21:22 +01:00
Lukas Tönne added 1 commit 2024-01-30 17:40:34 +01:00
Lukas Tönne added 1 commit 2024-01-30 18:42:25 +01:00
Lukas Tönne changed title from WIP: GPv3: Support for vertex group operators in GreasePencil data blocks to WIP: GPv3: Basic vertex group operators 2024-01-31 11:15:54 +01:00
Lukas Tönne added a new dependency 2024-01-31 11:16:38 +01:00
#117689 GPv3: Vertex Group Operators
Lukas Tönne removed a dependency 2024-01-31 11:16:46 +01:00
#117689 GPv3: Vertex Group Operators
Lukas Tönne added a new dependency 2024-01-31 11:16:59 +01:00
#117689 GPv3: Vertex Group Operators
Lukas Tönne force-pushed gp3-vertex-group-operators from 066ebd70af to 0bff7079f2 2024-01-31 11:23:34 +01:00 Compare
Lukas Tönne added 1 commit 2024-01-31 11:27:11 +01:00
Lukas Tönne added 2 commits 2024-01-31 11:35:31 +01:00
Lukas Tönne added 1 commit 2024-01-31 11:36:37 +01:00
Lukas Tönne changed title from WIP: GPv3: Basic vertex group operators to GPv3: Basic vertex group operators 2024-01-31 11:44:57 +01:00
Falk David requested review from Falk David 2024-01-31 11:54:39 +01:00
Falk David requested changes 2024-01-31 12:07:01 +01:00
Falk David left a comment
Member
Copy Link

Did a first pass on this.

Did a first pass on this.
source/blender/blenkernel/intern/grease_pencil_vgroup.cc Outdated
@ -0,0 +20,4 @@
#include "BLT_translation.h"
#include <iostream>
Falk David commented 2024-01-31 11:55:57 +01:00
Member
Copy Link

This header should probably be removed.

This header should probably be removed.
LukasTonne marked this conversation as resolved
source/blender/blenkernel/intern/grease_pencil_vgroup.cc Outdated
@ -0,0 +30,4 @@
void validate_drawing_vertex_groups(GreasePencil &grease_pencil)
{
Set<const char *> valid_names;
Falk David commented 2024-01-31 12:05:32 +01:00
Member
Copy Link

Will this not just compare the pointers to check for equality? I feel like this should be a Set<std::string>

Will this not just compare the pointers to check for equality? I feel like this should be a `Set<std::string>`
Lukas Tönne commented 2024-01-31 13:31:44 +01:00
Author
Member
Copy Link

Yes, should be Set<std::string>. I think the idea was to avoid string copies, but that's not very important for operators.

Yes, should be `Set<std::string>`. I think the idea was to avoid string copies, but that's not very important for operators.
LukasTonne marked this conversation as resolved
source/blender/blenkernel/intern/grease_pencil_vgroup.cc Outdated
@ -0,0 +70,4 @@
/* 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));
auto ensure_group_in_drawing = [&]() {
Falk David commented 2024-01-31 12:00:07 +01:00
Member
Copy Link

I think this lambda is not too helpful. Seems better to just put the code in the loop.

I think this lambda is not too helpful. Seems better to just put the code in the loop.
LukasTonne marked this conversation as resolved
source/blender/blenkernel/intern/grease_pencil_vgroup.cc Outdated
@ -0,0 +85,4 @@
};
const bke::AttributeAccessor attributes = curves.attributes();
const VArray<bool> select_vert = *attributes.lookup_or_default<bool>(
Falk David commented 2024-01-31 11:58:57 +01:00
Member
Copy Link

select_vert -> selection

`select_vert` -> `selection`
LukasTonne marked this conversation as resolved
source/blender/blenkernel/intern/grease_pencil_vgroup.cc Outdated
@ -0,0 +103,4 @@
}
/** Remove selected vertices from the vertex group. */
bool remove_from_vertex_group(GreasePencil &grease_pencil, StringRef name, bool use_selection)
Falk David commented 2024-01-31 12:06:35 +01:00
Member
Copy Link

const StringRef name and const bool use_selection

`const StringRef name` and `const bool use_selection`
LukasTonne marked this conversation as resolved
Lukas Tönne added 5 commits 2024-01-31 13:36:57 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
5c9130dd02 Const function parameters.
Lukas Tönne commented 2024-01-31 13:37:37 +01:00
Author
Member
Copy Link

@blender-bot build

@blender-bot build
Lukas Tönne added 1 commit 2024-01-31 13:48:14 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
8c50831ec7 Cleanup: formatting.
Lukas Tönne commented 2024-01-31 13:48:29 +01:00
Author
Member
Copy Link

@blender-bot build

@blender-bot build
Falk David approved these changes 2024-01-31 14:59:31 +01:00
Falk David left a comment
Member
Copy Link

The code looks good to me.
Personally, I would name the files grease_pencil_vertex_groups even if it's not consistent with the other ones.

The code looks good to me. Personally, I would name the files `grease_pencil_vertex_groups` even if it's not consistent with the other ones.
Lukas Tönne added 1 commit 2024-01-31 16:48:44 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
a1798cdab1 Merge branch 'main' into gp3-vertex-group-operators
Lukas Tönne commented 2024-01-31 16:50:24 +01:00
Author
Member
Copy Link

@blender-bot build

@blender-bot build
Lukas Tönne added 1 commit 2024-01-31 16:53:22 +01:00
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
90b59d4739 Renamed file to BKE_grease_pencil_vertex_groups.hh.
Lukas Tönne commented 2024-01-31 16:53:33 +01:00
Author
Member
Copy Link

@blender-bot build

@blender-bot build
Lukas Tönne merged commit 7e7165b085 into main 2024-01-31 17:46:07 +01:00
Lukas Tönne deleted branch gp3-vertex-group-operators 2024-01-31 17:46:09 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
Falk David
Labels
Clear labels   
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
This issue affects/is about backward or forward 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
Issues relating to Wayland windowing support.

  
Interest
Workbench

  
Interest: X11

Issues relating to Xorg/X11 support.

  
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
Issues relating to security: https://wiki.blender.org/wiki/Process/Vulnerability_Reports

  
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 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
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Grease Pencil
Assignees
Clear assignees
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Blocks
#117689 GPv3: Vertex Group Operators
blender/blender
Reference: blender/blender#117476
No description provided.
Delete Branch "LukasTonne/blender:gp3-vertex-group-operators"

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?