GPv3: Thickness modifier #117631

YimingWu · 2024-01-29T15:18:30+01:00

YimingWu commented

2024-01-29 15:18:30 +01:00

Thickness modifier ported to Grease Pencil v3.

Note: Uniform thickness range and UI step changed to better match new thickness of blender unit.

Thickness modifier ported to Grease Pencil v3. Note: Uniform thickness range and UI step changed to better match new thickness of blender unit. ![image](/attachments/2e9c9bfa-d869-4bec-a529-c3833390a201)

image.png

47 KiB

YimingWu added 2 commits 2024-01-29 15:18:44 +01:00

4cad100cf4 init

fe5f733c0f thickness modifier is functional now

YimingWu requested review from Falk David 2024-01-29 15:19:03 +01:00

YimingWu added the

labels 2024-01-29 15:19:10 +01:00

YimingWu added this to the Grease Pencil project 2024-01-29 15:19:15 +01:00

YimingWu added 1 commit 2024-01-29 15:23:11 +01:00

54c2c6470a Defaults

YimingWu added 1 commit 2024-01-29 15:25:31 +01:00

9ea276fc31 Fix uniform thickness

Falk David requested changes 2024-01-29 15:34:48 +01:00

Falk David left a comment

Some cleanup comments

source/blender/makesrna/intern/rna_modifier.cc Outdated

						
				@ -8132,0 +8174,4 @@

				  RNA_def_property_ui_text(prop, "Weighted", "Use weight to modulate effect");

				  RNA_def_property_update(prop, 0, "rna_Modifier_update");

				  prop = RNA_def_property(srna, "use_normalized_thickness", PROP_BOOLEAN, PROP_NONE);

Falk David commented

2024-01-29 15:21:51 +01:00

Should be named use_uniform_thickness.

Should be named `use_uniform_thickness`.

ChengduLittleA marked this conversation as resolved

source/blender/modifiers/intern/MOD_grease_pencil_thickness.cc Outdated

						
				@ -0,0 +110,4 @@

				  const VArray<float> vgroup_weights =

				      attributes

				          .lookup_or_default<float>(mmd.influence.vertex_group_name, bke::AttrDomain::Point, 1.0f)

				          .varray;

Falk David commented

2024-01-29 15:23:10 +01:00

Instead of using .varray you can do = *attributes.lookup_or_default<float>(...);

Instead of using `.varray` you can do ` = *attributes.lookup_or_default<float>(...);`

ChengduLittleA marked this conversation as resolved

source/blender/modifiers/intern/MOD_grease_pencil_thickness.cc Outdated

						
				@ -0,0 +111,4 @@

				      attributes

				          .lookup_or_default<float>(mmd.influence.vertex_group_name, bke::AttrDomain::Point, 1.0f)

				          .varray;

				  const bool is_normalized = (mmd.flag & MOD_GREASE_PENCIL_THICK_NORMALIZE);

Falk David commented

2024-01-29 15:23:53 +01:00

Use (...) != 0;

Use ` (...) != 0;`

ChengduLittleA marked this conversation as resolved

source/blender/modifiers/intern/MOD_grease_pencil_thickness.cc Outdated

						
				@ -0,0 +112,4 @@

				          .lookup_or_default<float>(mmd.influence.vertex_group_name, bke::AttrDomain::Point, 1.0f)

				          .varray;

				  const bool is_normalized = (mmd.flag & MOD_GREASE_PENCIL_THICK_NORMALIZE);

				  bool is_inverted = ((mmd.flag & MOD_GREASE_PENCIL_THICK_WEIGHT_FACTOR) == 0) &&

Falk David commented

2024-01-29 15:23:34 +01:00

Use const

Use `const`

ChengduLittleA marked this conversation as resolved

source/blender/modifiers/intern/MOD_grease_pencil_thickness.cc Outdated

						
				@ -0,0 +115,4 @@

				  bool is_inverted = ((mmd.flag & MOD_GREASE_PENCIL_THICK_WEIGHT_FACTOR) == 0) &&

				                     ((mmd.influence.flag & GREASE_PENCIL_INFLUENCE_INVERT_VERTEX_GROUP) != 0);

				  for (const int curve : curves.curves_range()) {

Falk David commented

2024-01-29 15:34:18 +01:00

We're only writing to the radii, so this should be a threading::parallel_for loop over the curves range.

We're only writing to the radii, so this should be a `threading::parallel_for` loop over the curves range.

ChengduLittleA marked this conversation as resolved

source/blender/modifiers/intern/MOD_grease_pencil_thickness.cc Outdated

						
				@ -0,0 +118,4 @@

				  for (const int curve : curves.curves_range()) {

				    for (const int local_point : points_by_curve[curve].index_range()) {

				      const int point = local_point + points_by_curve[curve].first();

				      float weight = vgroup_weights[point];

Falk David commented

2024-01-29 15:30:00 +01:00

Use const

Use `const`

ChengduLittleA marked this conversation as resolved

source/blender/modifiers/intern/MOD_grease_pencil_thickness.cc Outdated

						
				@ -0,0 +125,4 @@

				      if ((!is_normalized) && (mmd.flag & MOD_GREASE_PENCIL_THICK_WEIGHT_FACTOR)) {

				        radii[point] *= (is_inverted ? 1.0f - weight : weight);

				        if (radii[point] < 0.0f) {

Falk David commented

2024-01-29 15:30:26 +01:00

Use math::clamp instead of the if here

Use `math::clamp` instead of the if here

ChengduLittleA marked this conversation as resolved

source/blender/modifiers/intern/MOD_grease_pencil_thickness.cc Outdated

						
				@ -0,0 +131,4 @@

				        continue;

				      }

				      float curvef = 1.0f;

Falk David commented

2024-01-29 15:31:43 +01:00

You can use a lambda here to make this variable const. Maybe a better name would be influence too.

const float influence = [&]() {
   if (mmd.influence.flag & GREASE_PENCIL_INFLUENCE_USE_CUSTOM_CURVE &&
          (mmd.influence.custom_curve))
   {
      /* Normalize value to evaluate curve. */
      const float value = float(local_point) / (points_by_curve[curve].size() - 1);
      return BKE_curvemapping_evaluateF(mmd.influence.custom_curve, 0, value);
   }
   return 1.0f;
}();

You can use a lambda here to make this variable `const`. Maybe a better name would be `influence` too. ``` const float influence = [&]() { if (mmd.influence.flag & GREASE_PENCIL_INFLUENCE_USE_CUSTOM_CURVE && (mmd.influence.custom_curve)) { /* Normalize value to evaluate curve. */ const float value = float(local_point) / (points_by_curve[curve].size() - 1); return BKE_curvemapping_evaluateF(mmd.influence.custom_curve, 0, value); } return 1.0f; }(); ```

ChengduLittleA marked this conversation as resolved

source/blender/modifiers/intern/MOD_grease_pencil_thickness.cc Outdated

						
				@ -0,0 +140,4 @@

				        curvef = BKE_curvemapping_evaluateF(mmd.influence.custom_curve, 0, value);

				      }

				      float target;

Falk David commented

2024-01-29 15:29:46 +01:00

Same as above

const float target = [&]() {
   if (is_normalized) {
      return mmd.thickness * influence;
   }
   weight *= curvef; // why is this needed?
   return radii[point] * mmd.thickness_fac;
}();

Same as above ``` const float target = [&]() { if (is_normalized) { return mmd.thickness * influence; } weight *= curvef; // why is this needed? return radii[point] * mmd.thickness_fac; }(); ```

ChengduLittleA marked this conversation as resolved

source/blender/modifiers/intern/MOD_grease_pencil_thickness.cc Outdated

						
				@ -0,0 +150,4 @@

				        weight *= curvef;

				      }

				      float radius = math::interpolate(radii[point], target, weight * curvef);

Falk David commented

2024-01-29 15:24:53 +01:00

Use const

Use `const`

ChengduLittleA marked this conversation as resolved

source/blender/modifiers/intern/MOD_grease_pencil_thickness.cc Outdated

						
				@ -0,0 +151,4 @@

				      }

				      float radius = math::interpolate(radii[point], target, weight * curvef);

				      if (radius < 0.0f) {

Falk David commented

2024-01-29 15:24:45 +01:00

Use math::clamp instead of the if here

Use `math::clamp` instead of the if here

ChengduLittleA marked this conversation as resolved

YimingWu added 3 commits 2024-01-30 04:21:45 +01:00

b3de4bb9be cleanups

09ca80bf89 Merge branch 'main' into gp3-mod-thickness

4af5e6a5e3 cleanup

Pratik Borhade reviewed 2024-01-30 09:51:11 +01:00

source/blender/makesrna/intern/rna_modifier.cc Outdated

						
				@ -8230,0 +8244,4 @@

				  rna_def_modifier_grease_pencil_layer_filter(srna);

				  rna_def_modifier_grease_pencil_material_filter(

				      srna, "rna_GreasePencilSmoothModifier_material_filter_set");

Pratik Borhade commented

2024-01-30 09:49:54 +01:00

rna_GreasePencilThickModifier_material_filter_set and also need to include the macro for expansion: RNA_MOD_GREASE_PENCIL_MATERIAL_FILTER_SET

`rna_GreasePencilThickModifier_material_filter_set` and also need to include the macro for expansion: `RNA_MOD_GREASE_PENCIL_MATERIAL_FILTER_SET`

👍 1

ChengduLittleA marked this conversation as resolved

source/blender/makesrna/intern/rna_modifier.cc Outdated

						
				@ -8230,0 +8246,4 @@

				  rna_def_modifier_grease_pencil_material_filter(

				      srna, "rna_GreasePencilSmoothModifier_material_filter_set");

				  rna_def_modifier_grease_pencil_vertex_group(

				      srna, "rna_GreasePencilSmoothModifier_vertex_group_name_set");

Pratik Borhade commented

2024-01-30 09:50:41 +01:00

same as above :)

ChengduLittleA marked this conversation as resolved

Falk David requested changes 2024-01-30 10:34:18 +01:00

Falk David left a comment

Some cleanup comments

source/blender/modifiers/intern/MOD_grease_pencil_thickness.cc Outdated

						
				@ -0,0 +114,4 @@

				                           ((mmd.influence.flag & GREASE_PENCIL_INFLUENCE_INVERT_VERTEX_GROUP) !=

				                            0);

				  threading::parallel_for(curves.curves_range(), 512, [&](const IndexRange curves_range) {

Falk David commented

2024-01-30 10:32:39 +01:00

curves_range -> range

`curves_range` -> `range`

ChengduLittleA marked this conversation as resolved

source/blender/modifiers/intern/MOD_grease_pencil_thickness.cc Outdated

						
				@ -0,0 +116,4 @@

				  threading::parallel_for(curves.curves_range(), 512, [&](const IndexRange curves_range) {

				    for (const int curve : curves_range) {

				      for (const int local_point : points_by_curve[curve].index_range()) {

Falk David commented

2024-01-30 10:31:40 +01:00

Since you're looking up the points_by_curve[curve] multiple times, better put it in a variable above the inner loop.

const OffsetIndices<int> points = points_by_curve[curve];

Since you're looking up the `points_by_curve[curve]` multiple times, better put it in a variable above the inner loop. ``` const OffsetIndices<int> points = points_by_curve[curve]; ```

ChengduLittleA marked this conversation as resolved

YimingWu added 1 commit 2024-01-30 11:10:03 +01:00