CurvesGeometry: Add initial vertex group support #106944

Merged
Falk David merged 24 commits from filedescriptor/blender:curves-deform-verts into main 2023-09-27 10:26:16 +02:00
Member

This PR adds vertex groups to CurvesGeometry as well as an attribute read/write accessor.

This is also in preparation for GPv3. Since the goal is to have full compatibility with the current grease pencil features, vertex groups need to be supported.

Grease Pencil allows filtering by vertex group for modifiers.To support this, it also makes sense to have read/write access for vertex groups in the attributes API.

In the future, vertex groups should be just another custom attribute on meshes/curves/grease pencil. There are some more issues to be solved before that can happen. This step gets us a bit closer since the vertex weight data is stored in CustomData.

This PR adds vertex groups to `CurvesGeometry` as well as an attribute read/write accessor. This is also in preparation for GPv3. Since the goal is to have full compatibility with the current grease pencil features, vertex groups need to be supported. Grease Pencil allows filtering by vertex group for modifiers.To support this, it also makes sense to have read/write access for vertex groups in the attributes API. In the future, vertex groups should be just another custom attribute on meshes/curves/grease pencil. There are some more issues to be solved before that can happen. This step gets us a bit closer since the vertex weight data is stored in `CustomData`.
Hans Goudey reviewed 2023-04-14 13:38:52 +02:00
@ -440,1 +440,4 @@
Span<MDeformVert> CurvesGeometry::deform_verts() const
{
const MDeformVert *dverts = (const MDeformVert *)CustomData_get_layer(&this->point_data,
Member

Use static_cast here and below

Use `static_cast` here and below
filedescriptor marked this conversation as resolved
Falk David force-pushed curves-deform-verts from b1328470d1 to a0aee11edd 2023-08-08 13:47:51 +02:00 Compare
Falk David requested review from Hans Goudey 2023-08-08 15:48:06 +02:00
Falk David requested review from Jacques Lucke 2023-08-08 15:48:14 +02:00
Falk David changed title from WIP: Add vertex groups to CurvesGeometry to Curves: Add initial vertex group support 2023-08-08 15:48:34 +02:00
Falk David force-pushed curves-deform-verts from eab28c4e09 to 9dbc164680 2023-09-11 14:08:28 +02:00 Compare
Hans Goudey requested changes 2023-09-11 18:33:17 +02:00
Hans Goudey left a comment
Member

Like I mentioned in chat, I'd rather not expose this in the UI or RNA API for Curves, since longer term we'd like to replace the storage of vertex groups, and it would be nice to not have to deprecate this version for the new curves type.

Like I mentioned in chat, I'd rather not expose this in the UI or RNA API for `Curves`, since longer term we'd like to replace the storage of vertex groups, and it would be nice to not have to deprecate this version for the new curves type.
@ -0,0 +21,4 @@
namespace blender::bke {
class VArrayImpl_For_VertexWeights final : public VMutableArrayImpl<float> {
Member

This entire implementation shouldn't be in a header. Since virtual arrays use virtual inheritance, none of it needs to be in a header actually.

This entire implementation shouldn't be in a header. Since virtual arrays use virtual inheritance, none of it needs to be in a header actually.
Author
Member

I'm not sure I understand. Should I move this entire code to a .cc file? Why are all other classes inheriting from VMutableArrayImpl defined in a header then?

I'm not sure I understand. Should I move this entire code to a `.cc` file? Why are all other classes inheriting from `VMutableArrayImpl` defined in a header then?
Member

Why are all other classes inheriting from VMutableArrayImpl defined in a header then?

They are templated to work on any type, this is not. You can move this to a cc file and just expose a function that returns a virtual array (VArray) for a vertex group.

> Why are all other classes inheriting from `VMutableArrayImpl` defined in a header then? They are templated to work on any type, this is not. You can move this to a cc file and just expose a function that returns a virtual array (`VArray`) for a vertex group.
filedescriptor marked this conversation as resolved
@ -60,6 +61,10 @@ CurvesGeometry::CurvesGeometry(const int point_num, const int curve_num)
CustomData_reset(&this->point_data);
CustomData_reset(&this->curve_data);
/* Make sure to clear this before using the attributes API. Otherwise the vertex group accessor
Member

Not sure this comment is really necessary, it's just the same thing as the CustomData_reset calls above.

Not sure this comment is really necessary, it's just the same thing as the `CustomData_reset` calls above.
filedescriptor marked this conversation as resolved
@ -144,2 +152,4 @@
std::swap(dst.curve_offsets, src.curve_offsets);
dst.vertex_group_active_index = src.vertex_group_active_index;
std::swap(dst.vertex_group_names.first, src.vertex_group_names.first);
Member

Can just keep it simpler by swapping everything.

std::swap(dst.vertex_group_names, src.vertex_group_names);
std::swap(dst.vertex_group_active_index, src.vertex_group_active_index);
Can just keep it simpler by swapping everything. ``` std::swap(dst.vertex_group_names, src.vertex_group_names); std::swap(dst.vertex_group_active_index, src.vertex_group_active_index); ```
filedescriptor marked this conversation as resolved
Author
Member

Hm I'm unsure how I can remove the Python RNA API for this 🤔

Hm I'm unsure how I can remove the Python RNA API for this 🤔
Member

I think undoing the change in BKE_id_supports_vertex_groups should do it.

I think undoing the change in `BKE_id_supports_vertex_groups` should do it.
Falk David force-pushed curves-deform-verts from 9dbc164680 to 84d6b5448b 2023-09-18 11:55:46 +02:00 Compare
Falk David requested review from Hans Goudey 2023-09-18 13:09:58 +02:00
Hans Goudey requested changes 2023-09-18 13:48:03 +02:00
Hans Goudey left a comment
Member

The PR description should be updated to give some context, describe the goal a bit more, and remove the screenshot.

The PR description should be updated to give some context, describe the goal a bit more, and remove the screenshot.
Falk David changed title from Curves: Add initial vertex group support to Curves: Add vertex group support 2023-09-18 13:50:02 +02:00
Falk David requested review from Hans Goudey 2023-09-18 13:59:45 +02:00
Hans Goudey requested changes 2023-09-25 18:43:11 +02:00
Hans Goudey left a comment
Member

Seems more of the changes can be reverted still, since we don't add support for the Curves object type in this commit.

Also, sorry for the nitpicking, but some comments about the PR description:

Grease Pencil allows filtering by vertex group for modifiers. This is why the attribute API needs to support vertex groups as well.

Those two things don't really feel connected to me, I guess it assumes that the modifiers would access vertex groups with the attribute API. "needs to" is just a bit strong wording then.

This step gets us a bit closer since the vertex weight data is stored in CustomData.

Not really sure how this gets us closer, vertex group data is stored in CustomData with or without this change.

Seems more of the changes can be reverted still, since we don't add support for the `Curves` object type in this commit. Also, sorry for the nitpicking, but some comments about the PR description: >Grease Pencil allows filtering by vertex group for modifiers. This is why the attribute API needs to support vertex groups as well. Those two things don't really feel connected to me, I guess it assumes that the modifiers would access vertex groups with the attribute API. "needs to" is just a bit strong wording then. >This step gets us a bit closer since the vertex weight data is stored in CustomData. Not really sure how this gets us closer, vertex group data is stored in `CustomData` with or without this change.
@ -293,2 +298,4 @@
float *r_weights);
namespace blender::bke {
VArray<float> varray_for_deform_verts(Span<MDeformVert> dverts, const int dvert_index);
Member
  • const int -> int
  • dvert_index -> defgroup_index/vgroup_index
- `const int` -> `int` - `dvert_index` -> `defgroup_index`/`vgroup_index`
filedescriptor marked this conversation as resolved
@ -461,0 +486,4 @@
if (dvert != nullptr) {
return {dvert, this->point_num};
}
return {(MDeformVert *)CustomData_add_layer(
Member

Casting style

Casting style
filedescriptor marked this conversation as resolved
@ -489,2 +494,4 @@
return &gpd->vertex_group_active_index;
}
case OB_CURVES: {
const Curves *curves_id = static_cast<const Curves *>(ob->data);
Member

Remove ID_CV and OB_CURVES from these, since we don't want to expose curves object vertex groups right now.

Remove `ID_CV` and `OB_CURVES` from these, since we don't want to expose curves object vertex groups right now.
filedescriptor marked this conversation as resolved
@ -334,0 +408,4 @@
for (MDeformVert &dvert : dverts.slice(range)) {
MDeformWeight *weight = BKE_defvert_find_index(&dvert, index);
BKE_defvert_remove_group(&dvert, weight);
for (MDeformWeight &weight : MutableSpan(dvert.dw, dvert.totweight)) {
Member

It would be nice to not duplicate this code between meshes and curves too, could that be shared in BKE_deform.h too?

It would be nice to not duplicate this code between meshes and curves too, could that be shared in `BKE_deform.h` too?
filedescriptor marked this conversation as resolved
@ -122,0 +123,4 @@
return lt->dvert;
}
case ID_CV: {
Curves *curves_id = reinterpret_cast<Curves *>(id);
Member

Same here, ID_CV and OB_CURVES don't need to be supported right now (at least in this commit)

Same here, `ID_CV` and `OB_CURVES` don't need to be supported right now (at least in this commit)
filedescriptor marked this conversation as resolved
Falk David force-pushed curves-deform-verts from f95bcdd709 to c58e8d58a2 2023-09-26 10:59:57 +02:00 Compare
Falk David requested review from Hans Goudey 2023-09-26 11:00:13 +02:00
Author
Member

Not really sure how this gets us closer, vertex group data is stored in CustomData with or without this change.

How so for Grease Pencil?

> Not really sure how this gets us closer, vertex group data is stored in CustomData with or without this change. How so for Grease Pencil?
Falk David added 1 commit 2023-09-26 15:03:08 +02:00
Falk David added 1 commit 2023-09-26 15:04:18 +02:00
Falk David added 1 commit 2023-09-26 15:14:04 +02:00
Hans Goudey reviewed 2023-09-26 15:28:54 +02:00
Hans Goudey left a comment
Member

Just one last comment I think. The commit message should mention there are no functional changes too.

Just one last comment I think. The commit message should mention there are no functional changes too.
@ -146,0 +151,4 @@
std::swap(dst.vertex_group_names.first, src.vertex_group_names.first);
std::swap(dst.vertex_group_names.last, src.vertex_group_names.last);
std::swap(dst.vertex_group_active_index, src.vertex_group_active_index);
src.vertex_group_active_index = 0;
Member

src.vertex_group_active_index = 0; That part is unnecessary I think

`src.vertex_group_active_index = 0;` That part is unnecessary I think
filedescriptor marked this conversation as resolved
Hans Goudey reviewed 2023-09-26 15:29:40 +02:00
@ -25,6 +25,8 @@
#include "BKE_attribute_math.hh"
#include "BKE_curves.h"
#include "DNA_meshdata_types.h"
Member

Does it work to forward declare MDeformVert? It would be nice not to include DNA_meshdata_types.h in the curves header.

Does it work to forward declare `MDeformVert`? It would be nice not to include `DNA_meshdata_types.h` in the curves header.
filedescriptor marked this conversation as resolved
Falk David changed title from Curves: Add vertex group support to CurvesGeometry: Add initial vertex group support 2023-09-26 16:10:20 +02:00
Falk David added 2 commits 2023-09-26 16:13:18 +02:00
Hans Goudey approved these changes 2023-09-26 16:44:59 +02:00
Jacques Lucke approved these changes 2023-09-26 22:29:19 +02:00
Falk David merged commit bc7034d9fd into main 2023-09-27 10:26:16 +02:00
Falk David deleted branch curves-deform-verts 2023-09-27 10:26:18 +02: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
3 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#106944
No description provided.