GPv3: Vertex Weight Angle modifier #117846

Merged
Falk David merged 13 commits from ChengduLittleA/blender:gp3-mod-wangle into main 2024-02-16 12:30:52 +01:00
Member

Vertex Weight Angle modifier is migrated to Grease Pencil v3.

图片

Vertex Weight Angle modifier is migrated to Grease Pencil v3. ![图片](/attachments/53e640c3-38f7-4424-99b0-7b0d4db78472)
YimingWu added the
Interest
Grease Pencil
Module
Grease Pencil
labels 2024-02-05 13:54:53 +01:00
YimingWu added 1 commit 2024-02-05 13:55:07 +01:00
Falk David requested changes 2024-02-05 14:04:06 +01:00
Falk David left a comment
Member

First pass

First pass
@ -0,0 +92,4 @@
modifier::greasepencil::read_influence_data(reader, &mmd->influence);
}
static void deform_drawing(const ModifierData &md,
Member

deform_drawing -> write_weights_for_drawing ?

`deform_drawing` -> `write_weights_for_drawing` ?
ChengduLittleA marked this conversation as resolved
@ -0,0 +109,4 @@
}
bke::MutableAttributeAccessor attributes = curves.attributes_for_write();
bke::SpanAttributeWriter<float> target = attributes.lookup_or_add_for_write_span<float>(mmd.target_vgname,bke::AttrDomain::Point);
Member

Maybe dst_weights ?

Maybe `dst_weights` ?
ChengduLittleA marked this conversation as resolved
@ -0,0 +117,4 @@
mmd.influence.vertex_group_name, bke::AttrDomain::Point, 1.0f);
/* Use default Z up. */
float3 vec_axis(0.0f, 0.0f, 1.0f);
Member

const float3 z_up(0.0f, 0.0f, 1.0f);

`const float3 z_up(0.0f, 0.0f, 1.0f);`
ChengduLittleA marked this conversation as resolved
@ -0,0 +136,4 @@
const Span<float3> positions = curves.positions();
strokes.foreach_index(GrainSize(512), [&](const int stroke) {
IndexRange points = points_by_curve[stroke];
Member

Use const

Use `const`
ChengduLittleA marked this conversation as resolved
@ -0,0 +142,4 @@
return;
}
for (const int point : points.drop_front(1)) {
float3 p1 = math::transform_point(obmat3x3, positions[point]);
Member

All these vectors can be const

All these vectors can be `const`
ChengduLittleA marked this conversation as resolved
@ -0,0 +146,4 @@
float3 p2 = math::transform_point(obmat3x3, positions[point - 1]);
float3 vec = p2 - p1;
const float angle = angle_on_axis_v3v3_v3(vec_ref, vec, axis);
float weight = target.span[point] = 1.0f - sin(angle);
Member

Hm this looks strange, is it suppoesd to be just const float weight =1.0f - math::sin(angle); ?

Hm this looks strange, is it suppoesd to be just `const float weight =1.0f - math::sin(angle);` ?
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-02-05 14:31:41 +01:00
Hans Goudey requested changes 2024-02-05 15:24:56 +01:00
@ -119,0 +120,4 @@
"GREASE_PENCIL_VERTEX_WEIGHT_ANGLE",
ICON_MOD_VERTEX_WEIGHT,
"Vertex Weight Angle",
"Set the vertex group weights based on the angle of the stroke"},
Member

Better to just keep the old description: Generate vertex weights base on stroke angle

Better to just keep the old description: `Generate vertex weights base on stroke angle`
ChengduLittleA marked this conversation as resolved
@ -0,0 +96,4 @@
const Object &ob,
bke::greasepencil::Drawing &drawing)
{
auto &mmd = reinterpret_cast<const GPWeightAngleModifierData &>(md);
Member

const auto

`const auto`
ChengduLittleA marked this conversation as resolved
@ -0,0 +114,4 @@
if (dst_weights.span.is_empty()) {
return;
}
const VArray<float> input_weights = *attributes.lookup_or_default<float>(
Member

I think this isn't right when the name is set but the vertex group has no data. There's a function added for this case instead: get_influence_vertex_weights

I think this isn't right when the name is set but the vertex group has no data. There's a function added for this case instead: `get_influence_vertex_weights`
ChengduLittleA marked this conversation as resolved
@ -0,0 +124,4 @@
float3 vec_ref;
/* Apply modifier rotation (sub 90 degrees for Y axis due Z-Up vector). */
const float rot_angle = mmd.angle - ((mmd.axis == 1) ? M_PI_2 : 0.0f);
rotate_normalized_v3_v3v3fl(vec_ref, z_up, axis, rot_angle);
Member

You can use the AxisAngle type and to_quaternion here I think

You can use the `AxisAngle` type and `to_quaternion` here I think
Author
Member

Let me try fiddle with it...

Let me try fiddle with it...
Author
Member

I don't think I understood the math here to convert this into new vector api 😅 Can't get it to work correctly

I don't think I understood the math here to convert this into new vector api 😅 Can't get it to work correctly
ChengduLittleA marked this conversation as resolved
@ -0,0 +159,4 @@
dst_weights.span[point] = math::clamp(dst_weights.span[point], mmd.min_weight, 1.0f);
}
/* First point has the same weight as the second one. */
dst_weights.span[0] = dst_weights.span[1];
Member

Don't you meant the first point of the curve, rather than the first point in all curves?

Don't you meant the first point of the curve, rather than the first point in all curves?
ChengduLittleA marked this conversation as resolved
@ -0,0 +169,4 @@
const ModifierEvalContext *ctx,
bke::GeometrySet *geometry_set)
{
GPWeightAngleModifierData *mmd = reinterpret_cast<GPWeightAngleModifierData *>(md);
Member

const

`const`
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-02-06 03:38:09 +01:00
Falk David added this to the Grease Pencil project 2024-02-08 14:53:10 +01:00
Falk David approved these changes 2024-02-08 15:24:40 +01:00
Falk David left a comment
Member

Looks good.

Looks good.
Hans Goudey reviewed 2024-02-08 15:43:13 +01:00
@ -0,0 +109,4 @@
}
bke::MutableAttributeAccessor attributes = curves.attributes_for_write();
bke::SpanAttributeWriter<float> dst_weights = attributes.lookup_or_add_for_write_span<float>(
Member

Is the output meant to be a vertex group? Seems it probably should be to have compatibility with the old modifier. In that case, vertex groups can't be created with the attribute API

Is the output meant to be a vertex group? Seems it probably should be to have compatibility with the old modifier. In that case, vertex groups can't be created with the attribute API
Member

The modifier isn't supposed to create the vertex group, only write to it.

The modifier isn't supposed to create the vertex group, only write to it.
Member

So I guess we just need to check if the vertex group is present, otherwise return.

So I guess we just need to check if the vertex group is present, otherwise return.
Member

In that case it's misleading to use lookup_or_add_for_write_span (specifically the "or_add" part).

In that case it's misleading to use `lookup_or_add_for_write_span` (specifically the "or_add" part).
Member

Right, should be lookup_for_write_span

Right, should be `lookup_for_write_span`
Author
Member

Apparently lookup_for_write_span is not effective when no points has been assigned to the target vertex group (And it will naturally happen when you just prepare some empty vertex groups for passing stuff inside modifier). It will crash if I skip the dst_weights.span.is_empty() check.

Apparently `lookup_for_write_span` is not effective when no points has been assigned to the target vertex group (And it will naturally happen when you just prepare some empty vertex groups for passing stuff inside modifier). It will crash if I skip the `dst_weights.span.is_empty()` check.
Member

I think the crash should be fixed then, because semantically that seems like the right way to do it.

I think the crash should be fixed then, because semantically that seems like the right way to do it.
Author
Member

I'm not sure how to fix this in this case... I think we need to make sure the result of lookup_for_write_span is still writeable when is_empty() ? But that would mean lookup_or_add_for_write_span. I think it won't change any original data since it's only doing things in the modifier geometry set.

To better make sure that we stick to the "don't create vertex group in the modifier" thing, I guess there can be a check for "whether the vertex group exists", and if the vertex group does exist while no points have been assigned to it, should dst_weights.span.is_empty() return true?

I'm not sure how to fix this in this case... I think we need to make sure the result of `lookup_for_write_span` is still writeable when `is_empty()` ? But that would mean `lookup_or_add_for_write_span`. I think it won't change any original data since it's only doing things in the modifier geometry set. To better make sure that we stick to the "don't create vertex group in the modifier" thing, I guess there can be a check for "whether the vertex group exists", and if the vertex group does exist while no points have been assigned to it, should `dst_weights.span.is_empty()` return true?
Member

Right, I think it should be:

  • Check if the name exists in the grease_penci.vertex_group_names. If it doesn't, return
  • If it does, ensure that the vertex groups is added to the drawing (see #117753 (comment)).
  • Write to the weights with lookup_for_write_span
Right, I think it should be: * Check if the name exists in the `grease_penci.vertex_group_names`. If it doesn't, return * If it does, ensure that the vertex groups is added to the drawing (see https://projects.blender.org/blender/blender/issues/117753#issuecomment-1114422). * Write to the weights with `lookup_for_write_span`
Author
Member

Now it seems to be working.

Now it seems to be working.
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-02-15 13:23:42 +01:00
YimingWu added 2 commits 2024-02-15 14:42:54 +01:00
Hans Goudey requested changes 2024-02-15 15:14:27 +01:00
Hans Goudey left a comment
Member
RNA_boolean_get: GPWeightAngleModifier.open_influence_panel not found.
``` RNA_boolean_get: GPWeightAngleModifier.open_influence_panel not found. ```
YimingWu added 1 commit 2024-02-15 15:33:21 +01:00
Author
Member

The RNA error was likely broken by the merge, fixed.

The RNA error was likely broken by the merge, fixed.
Hans Goudey requested changes 2024-02-15 15:35:34 +01:00
@ -0,0 +93,4 @@
modifier::greasepencil::read_influence_data(reader, &mmd->influence);
}
static int ensure_vertex_group(const StringRef name, ListBase &vertex_group_names)
Member

StringRef.data() can't be compared with C strings because it isn't necessarily null terminated. In that case you have to use StringRefNull and .c_str().

`StringRef.data()` can't be compared with C strings because it isn't necessarily null terminated. In that case you have to use `StringRefNull` and `.c_str()`.
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-02-15 15:47:49 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 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
7f6069f050
Use StringRefNull
Hans Goudey requested changes 2024-02-15 16:01:32 +01:00
Hans Goudey left a comment
Member

I have some final comments but I don't think this will need another round of review from me.

I have some final comments but I don't think this will need another round of review from me.
@ -0,0 +29,4 @@
#include "UI_interface.hh"
#include "UI_resources.hh"
#include "GEO_smooth_curves.hh"
Member

Unused include

Unused include
ChengduLittleA marked this conversation as resolved
@ -0,0 +99,4 @@
&vertex_group_names, name.c_str(), offsetof(bDeformGroup, name));
if (def_nr < 0) {
bDeformGroup *defgroup = MEM_cnew<bDeformGroup>(__func__);
STRNCPY(defgroup->name, name.data());
Member

Call .c_str() instead of data to make the intentions ("I need the null character") clear

Call `.c_str()` instead of data to make the intentions ("I need the null character") clear
ChengduLittleA marked this conversation as resolved
@ -0,0 +134,4 @@
}
/* Make sure that the target vertex group is added to this drawing so we can write to it. */
ensure_vertex_group(mmd.target_vgname, drawing.strokes_for_write().vertex_group_names);
Member

drawing.strokes_for_write().vertex_group_names -> curves.vertex_group_names

`drawing.strokes_for_write().vertex_group_names` -> `curves.vertex_group_names`
ChengduLittleA marked this conversation as resolved
@ -0,0 +154,4 @@
const float rot_angle = mmd.angle - ((mmd.axis == 1) ? M_PI_2 : 0.0f);
rotate_normalized_v3_v3v3fl(vec_ref, z_up, axis, rot_angle);
const float3x3 obmat3x3(float4x4(ob.object_to_world()));
Member

object_to_world already returns the 4x4 matrix type, so that can be removed

`object_to_world` already returns the 4x4 matrix type, so that can be removed
ChengduLittleA marked this conversation as resolved
Member

@blender-bot build

@blender-bot build
Sietse Brouwer reviewed 2024-02-15 16:11:08 +01:00
@ -0,0 +93,4 @@
modifier::greasepencil::read_influence_data(reader, &mmd->influence);
}
static int ensure_vertex_group(const StringRefNull name, ListBase &vertex_group_names)
Member

This will be needed in other places too, so IMHO it's better to have this in blenkernel/internel/grease_pencil_vertex_groups.cc.

This will be needed in other places too, so IMHO it's better to have this in `blenkernel/internel/grease_pencil_vertex_groups.cc`.
Member

Yes I'm working on a refactor that will add this.

Yes I'm working on a refactor that will add this.
Author
Member

Hummm should I do anything about it?

Hummm should I do anything about it?
Member

Because there are more places than just this one, for this PR to stay local to the modifier, you don't need to do anything. I'll replace the static function with a kernel one after.

Because there are more places than just this one, for this PR to stay local to the modifier, you don't need to do anything. I'll replace the static function with a kernel one after.
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-02-15 16:43:14 +01:00
YimingWu added 1 commit 2024-02-15 16:45:39 +01:00
Hans Goudey approved these changes 2024-02-15 16:53:40 +01:00
Falk David requested changes 2024-02-15 16:53:58 +01:00
Falk David left a comment
Member

Some final cleanup comments from me

Some final cleanup comments from me
@ -8650,6 +8658,7 @@ static void rna_def_modifier_grease_pencil_thickness(BlenderRNA *brna)
}
static void rna_def_modifier_grease_pencil_lattice(BlenderRNA *brna)
Member

Seems like this empty line slipped in

Seems like this empty line slipped in
ChengduLittleA marked this conversation as resolved
@ -0,0 +264,4 @@
} // namespace blender
ModifierTypeInfo modifierType_GPWeightAngle = {
Member

The name can be GreasePencilWeightAngle everywhere now. Lukas fixed that

The name can be `GreasePencilWeightAngle` everywhere now. Lukas fixed that
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-02-16 02:44:01 +01:00
YimingWu added 1 commit 2024-02-16 07:55:02 +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-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
b40924c1f2
Merge branch 'main' into gp3-mod-wangle
Falk David approved these changes 2024-02-16 10:18:34 +01:00
Member

@blender-bot build

@blender-bot build
YimingWu added 1 commit 2024-02-16 12:24:12 +01:00
Falk David merged commit ace5c9af2a into main 2024-02-16 12:30:52 +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#117846
No description provided.