GPv3: Length modifier #117124

Merged
Falk David merged 45 commits from ChengduLittleA/blender:gp3-length-modifier into main 2024-02-15 16:38:05 +01:00
Member

Length modifier ported to Grease Pencil v3

All features now work as expected

图片

Length modifier ported to Grease Pencil v3 All features now work as expected ![图片](/attachments/a5041089-b083-422c-b3dc-1029eef5a38c)
102 KiB
YimingWu added the
Interest
Grease Pencil
Module
Grease Pencil
labels 2024-01-15 09:53:26 +01:00
YimingWu added 3 commits 2024-01-15 09:53:36 +01:00
YimingWu added this to the Grease Pencil project 2024-01-15 09:54:19 +01:00
YimingWu added 1 commit 2024-01-15 13:46:24 +01:00
YimingWu added 1 commit 2024-01-15 14:51:50 +01:00
YimingWu reviewed 2024-01-15 14:52:50 +01:00
@ -0,0 +56,4 @@
do_starts[curve]=do_start;
do_ends[curve]=do_end;
}
OffsetIndices<int> dst_point_offsets = offset_indices::accumulate_counts_to_offsets(dst_point_counts.as_mutable_span());
Author
Member

So here it seems to have some problems.

So here it seems to have some problems.
ChengduLittleA marked this conversation as resolved
Hans Goudey reviewed 2024-01-15 14:59:49 +01:00
@ -0,0 +42,4 @@
Array<int> dst_point_counts(src_curves_num + 1);
/* Count how many points we need. */
for(const int curve : do_curves.index_range()){
Member

Looks like do_curves isn't necessary. You can copy the existing point counts with copy_group_sizes (using the full curves mask). Then iterate through the selection mask, increasing the point count for those curves as necessary

Looks like `do_curves` isn't necessary. You can copy the existing point counts with `copy_group_sizes` (using the full curves mask). Then iterate through the selection mask, increasing the point count for those curves as necessary
ChengduLittleA marked this conversation as resolved
@ -0,0 +65,4 @@
const int point_count = points_by_curve[curve].size();
int local_front = 0;
MutableSpan<int> new_points = dst_to_src_point.as_mutable_span().slice(
dst_point_counts[curve], dst_point_counts[curve+1]-dst_point_counts[curve]);
Member

.slice(dst_point_offsets[curve])

`.slice(dst_point_offsets[curve])`
ChengduLittleA marked this conversation as resolved
@ -0,0 +86,4 @@
Array<int> dst_to_src_curve(src_curves_num);
dst_to_src_curve.fill(1);
dst_to_src_curve[0] = 0;
offset_indices::accumulate_counts_to_offsets(dst_to_src_curve.as_mutable_span());
Member

Instead of copying the attributes like this, it's better to use copy_only_curve_domain to create the new curves. That will use implicit sharing to not do any copies for curve domain attributes. Then you can resize those curves to contain the correct amount of points, after using their offsets array instead of dst_point_counts

Instead of copying the attributes like this, it's better to use `copy_only_curve_domain` to create the new curves. That will use implicit sharing to not do any copies for curve domain attributes. Then you can resize those curves to contain the correct amount of points, after using their offsets array instead of `dst_point_counts`
ChengduLittleA marked this conversation as resolved
@ -0,0 +105,4 @@
src_attributes, bke::AttrDomain::Point, {}, {}, dst_to_src_point, dst_attributes);
dst_curves.update_curve_types();
dst_curves.remove_attributes_based_on_types();
Member

You didn't change the types here, so no need for remove_attributes_based_on_types

You didn't change the types here, so no need for `remove_attributes_based_on_types`
ChengduLittleA marked this conversation as resolved
YimingWu added 2 commits 2024-01-16 02:54:44 +01:00
YimingWu added 1 commit 2024-01-16 06:53:50 +01:00
YimingWu added 1 commit 2024-01-16 07:44:49 +01:00
YimingWu added 2 commits 2024-01-16 13:25:00 +01:00
YimingWu added 1 commit 2024-01-16 14:49:15 +01:00
YimingWu added 1 commit 2024-01-16 14:51:04 +01:00
YimingWu added 2 commits 2024-01-18 07:45:04 +01:00
YimingWu added 1 commit 2024-01-18 08:26:23 +01:00
YimingWu added 1 commit 2024-01-18 13:27:15 +01:00
YimingWu added 1 commit 2024-01-18 13:28:14 +01:00
YimingWu added 1 commit 2024-01-22 06:48:20 +01:00
YimingWu added 1 commit 2024-01-22 07:28:17 +01:00
YimingWu changed title from WIP: GPv3: Length modifier to GPv3: Length modifier 2024-01-22 07:30:34 +01:00
YimingWu added 1 commit 2024-01-22 07:30:36 +01:00
YimingWu added 1 commit 2024-01-22 07:36:13 +01:00
Iliya Katushenock reviewed 2024-01-22 10:19:52 +01:00
@ -0,0 +187,4 @@
selection,
use_starts,
use_ends,
std::move(VArray<float>::ForSingle(mmd.overshoot_fac, cnum)),

Move is not required if arguments is taken by reference. Even more, result of function (constructor in this case) already is r-value. Value of such r-value argument will be there while function is evaluated.

Move is not required if arguments is taken by reference. Even more, result of function (constructor in this case) already is r-value. Value of such r-value argument will be there while function is evaluated.
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-01-22 16:44:49 +01:00
Hans Goudey requested changes 2024-01-23 19:16:01 +01:00
@ -0,0 +170,4 @@
positions[new_points[curve][index2]],
fmodf(overshoot_point_param, 1.0f));
result -= positions[new_points[curve].last()];
if (UNLIKELY(is_zero_v3(result))) {
Member

Use C++ math vector API

Use C++ math vector API
ChengduLittleA marked this conversation as resolved
@ -0,0 +190,4 @@
* strokes. */
const float overshoot_parameter = used_percent_length * (orig_totpoints - 2);
int overshoot_pointcount = ceil(overshoot_parameter);
CLAMP(overshoot_pointcount, 1, orig_totpoints - 2);
Member

std::clamp

`std::clamp`
ChengduLittleA marked this conversation as resolved
@ -0,0 +193,4 @@
CLAMP(overshoot_pointcount, 1, orig_totpoints - 2);
/* Do for both sides without code duplication. */
float3 no, vec1, vec2, total_angle;
Member

Declare variables where their values are assigned

Declare variables where their values are assigned
ChengduLittleA marked this conversation as resolved
@ -0,0 +200,4 @@
}
const int start_i = k == 0 ? first_old_index :
last_old_index; // first_old_index, last_old_index
Member

Comment style

Comment style
ChengduLittleA marked this conversation as resolved
@ -0,0 +207,4 @@
positions[new_points[curve][start_i]];
total_angle = float3({0, 0, 0});
float segment_length = normalize_v3(
vec1); /* Not using `math::normalize_and_get_length` for simplicity. */
Member

Put comments on separate lines so they don't cause weird wrapping like this. Also, just use normalize_and_get_length, mixing two APIs isn't simpler.

Put comments on separate lines so they don't cause weird wrapping like this. Also, just use `normalize_and_get_length`, mixing two APIs isn't simpler.
ChengduLittleA marked this conversation as resolved
@ -0,0 +273,4 @@
const float prev_length = normalize_v3_length(vec1, step_length);
/* Build rotation matrix here to get best performance. */
float rot[3][3];
Member

Use the C++ matrix and rotaiton API here, that will give us less work to do in the future

Use the C++ matrix and rotaiton API here, that will give us less work to do in the future
Author
Member

Not sure the equivalent of quat_to_mat3 and axis_angle_to_quat in math::.

Tried following, doesn't seem to work:

        math::AxisAngleBase axis_base(total_angle,angle_step);
        math::Quaternion q = math::to_quaternion(axis_base);
        math::EulerXYZ euler = math::to_euler(q);
        float3x3 rot = math::to_gimbal_axis(ebase); //<----- this won't work
Not sure the equivalent of `quat_to_mat3` and `axis_angle_to_quat` in `math::`. Tried following, doesn't seem to work: ``` math::AxisAngleBase axis_base(total_angle,angle_step); math::Quaternion q = math::to_quaternion(axis_base); math::EulerXYZ euler = math::to_euler(q); float3x3 rot = math::to_gimbal_axis(ebase); //<----- this won't work ```
ChengduLittleA marked this conversation as resolved
@ -2610,0 +2612,4 @@
typedef struct GreasePencilLengthModifierData {
ModifierData modifier;
GreasePencilModifierInfluenceData influence;
/** Flags. */
Member

This comment isn't helpful. Best to save comments for when they explain something non-obvious.

This comment isn't helpful. Best to save comments for when they explain something non-obvious.
ChengduLittleA marked this conversation as resolved
@ -2610,0 +2624,4 @@
int seed;
/** How many frames before recalculate randoms. */
int step;
/** Modifier mode. */
Member

Reference the enum type instead, like /** #EnumType. */

Reference the enum type instead, like `/** #EnumType. */`
ChengduLittleA marked this conversation as resolved
@ -0,0 +95,4 @@
static Array<float> noise_table(int len, int offset, int seed)
{
Array<float> table(len);
for (int i = 0; i < len; i++) {
Member

for (const int i : table.index_range())

`for (const int i : table.index_range())`
ChengduLittleA marked this conversation as resolved
@ -0,0 +103,4 @@
static float table_sample(Array<float> &table, float x)
{
return math::interpolate(table[int(ceilf(x))], table[int(floor(x))], fractf(x));
Member
  • Use C++ math functions
  • Array<float> & -> MutableSpan<float>
- Use C++ math functions - `Array<float> &` -> `MutableSpan<float>`
ChengduLittleA marked this conversation as resolved
@ -0,0 +107,4 @@
}
static void deform_drawing(ModifierData &md,
Depsgraph *depsgraph,
Member

Use references and const

Use references and const
ChengduLittleA marked this conversation as resolved
@ -0,0 +122,4 @@
const IndexMask selection = modifier::greasepencil::get_filtered_stroke_mask(
&ob, curves, mmd.influence, memory);
const int cnum = curves.curves_num();
Member

cnum -> curves_num

`cnum` -> `curves_num`
ChengduLittleA marked this conversation as resolved
@ -0,0 +127,4 @@
/* Variable for tagging shrinking when values are adjusted after random. */
bool needs_additional_shrinking = false;
VArray<float> use_starts = VArray<float>::ForSingle(mmd.start_fac, cnum);
Member

Wherever the old modifier didn't support varying values per curve, better to just use a single value here rather than VArray::ForSingle. Using virtual arrays adds overhead and complicates code. If we want to add support for varying values later, that can be done. For now things should be as simple as possible.

Wherever the old modifier didn't support varying values per curve, better to just use a single value here rather than `VArray::ForSingle`. Using virtual arrays adds overhead and complicates code. If we want to add support for varying values later, that can be done. For now things should be as simple as possible.
Author
Member

The old modifier does support varying values per curve, since it had options for randomized length. Using an array here so we can call stretch_curves once with different values. use_starts will be referring to modified_starts array when this happens.

The old modifier does support varying values per curve, since it had options for randomized length. Using an array here so we can call `stretch_curves` once with different values. `use_starts` will be referring to `modified_starts` array when this happens.
Member

I'm confused, in the old modifier all these were just a single value for the whole modifier:

const float dist
const float overshoot_fac
const short mode
const bool follow_curvature
const int extra_point_count
const float segment_influence
const float max_angle
const bool invert_curvature

I only see overshoot_fac varying per stroke in the old modifier.

My point is, you shouldn't need to use VArray::ForSingle unless there's another code path that calls the function with a non-single varray, that just adds overhead for no reason.

I'm confused, in the old modifier all these were just a single value for the whole modifier: ``` const float dist const float overshoot_fac const short mode const bool follow_curvature const int extra_point_count const float segment_influence const float max_angle const bool invert_curvature ``` I only see `overshoot_fac` varying per stroke in the old modifier. My point is, you shouldn't need to use `VArray::ForSingle` unless there's another code path that calls the function with a non-single varray, that just adds overhead for no reason.
Author
Member

The old modifier call stretch per stroke, and each stroke will have different distance and segment count (since there's also randomize feature).

I think for the purpose of this modifier, arguments other than start/end lengths can be single value (since it accepts point density directly), but maybe it would be useful if this were to become a node some time later so people can pass an array and have a bunch of strokes modified differently.

The old modifier call `stretch` per stroke, and each stroke will have different distance and segment count (since there's also randomize feature). I think for the purpose of this modifier, arguments other than start/end lengths can be single value (since it accepts point density directly), but maybe it would be useful if this were to become a node some time later so people can pass an array and have a bunch of strokes modified differently.
Member

In that case, per-curve variation could be easily implemented in the future if it was necessary. The code committed in this PR shouldn't have more complexity and arbitrary performance cost than necessary though. Anyway, I doubt any of these modifiers will become nodes. Geometry nodes is design in a very different way than the GP modifiers.

In that case, per-curve variation could be easily implemented in the future if it was necessary. The code committed in this PR shouldn't have more complexity and arbitrary performance cost than necessary though. Anyway, I doubt any of these modifiers will become nodes. Geometry nodes is design in a very different way than the GP modifiers.
ChengduLittleA marked this conversation as resolved
YimingWu added 2 commits 2024-01-24 03:28:17 +01:00
Author
Member

@filedescriptor Do we need to add an option to "remove 1 point strokes" since trim_curves will always leave the last point, which is different than the old length modifier where if you trimmed longer than the stroke, the entire stroke will be erased.

@filedescriptor Do we need to add an option to "remove 1 point strokes" since `trim_curves` will always leave the last point, which is different than the old length modifier where if you trimmed longer than the stroke, the entire stroke will be erased.
YimingWu added 1 commit 2024-01-24 03:36:50 +01:00
YimingWu added 1 commit 2024-01-24 04:59:10 +01:00
YimingWu added 1 commit 2024-01-25 04:32:23 +01:00
Member

@ChengduLittleA I suspect that you can call remove_curves for the curves that get fully trimmed away.

@ChengduLittleA I suspect that you can call `remove_curves` for the curves that get fully trimmed away.
YimingWu added 1 commit 2024-01-29 04:58:28 +01:00
Falk David requested changes 2024-01-29 12:00:07 +01:00
Falk David left a comment
Member

Some cleanup comments

Some cleanup comments
@ -1129,12 +1129,12 @@ static int add_material_from_template(Main &bmain, Object &ob, const ColorTempla
return index;
}
static bke::CurvesGeometry create_drawing_data(const Span<float3> positions,
Member

These changes should be reverted.

These changes should be reverted.
ChengduLittleA marked this conversation as resolved
@ -1135,3 +1132,1 @@
const Span<int> offsets,
const Span<int> materials,
const float4x4 &matrix)
bke::CurvesGeometry create_drawing_data(const Span<float3> positions,
Member

Looks like these changes aren't needed.

Looks like these changes aren't needed.
ChengduLittleA marked this conversation as resolved
@ -198,6 +198,13 @@ IndexMask retrieve_editable_and_selected_elements(Object &object,
bke::AttrDomain selection_domain,
IndexMaskMemory &memory);
bke::CurvesGeometry create_drawing_data(const Span<float3> positions,
Member

Same as above.

Same as above.
ChengduLittleA marked this conversation as resolved
@ -0,0 +17,4 @@
* Create a new Curves instance by trimming the input curves. Copying the selected splines
* between the start and end points.
*/
bke::CurvesGeometry stretch_curves(const bke::CurvesGeometry &src_curves,
Member

A better name would probably be extend_curves

A better name would probably be `extend_curves`
Author
Member

I'll also change the file name.

I'll also change the file name.
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-01-29 12:17:47 +01:00
Hans Goudey requested changes 2024-01-29 15:22:57 +01:00
@ -151,6 +151,7 @@ class OBJECT_MT_modifier_add_generate(ModifierAddMenu, Menu):
self.operator_modifier_add(layout, 'WIREFRAME')
if ob_type == 'GREASEPENCIL':
self.operator_modifier_add(layout, 'GREASE_PENCIL_SUBDIV')
self.operator_modifier_add(layout, 'GREASE_PENCIL_LENGTH')
Member

Use alphabetical order

Use alphabetical order
ChengduLittleA marked this conversation as resolved
@ -40,12 +40,14 @@ set(SRC
intern/set_curve_type.cc
intern/smooth_curves.cc
intern/subdivide_curves.cc
intern/extend_curves.cc
Member

Use alphabetical order

Use alphabetical order
ChengduLittleA marked this conversation as resolved
@ -0,0 +14,4 @@
namespace blender::geometry {
/*
* Create a new Curves instance by trimming the input curves. Copying the selected splines
Member

This is copy & pasted, not correct here (the original comment isn't great here either, "Curves instance" isn't really correct). I think this sort of thing is much easier to catch if you read over the diff in Gitea before requesting review.

This is copy & pasted, not correct here (the original comment isn't great here either, "Curves instance" isn't really correct). I think this sort of thing is much easier to catch if you read over the diff in Gitea before requesting review.
ChengduLittleA marked this conversation as resolved
@ -0,0 +11,4 @@
#include "BLI_math_axis_angle.hh"
#include "BLI_math_matrix.h"
#include "BLI_math_quaternion.hh"
#include "BLI_math_rotation.h"
Member

If possible, try to only include the new math headers, that will make it clearer to only use the newer API

If possible, try to only include the new math headers, that will make it clearer to only use the newer API
ChengduLittleA marked this conversation as resolved
@ -0,0 +37,4 @@
const VArray<float> &max_angle,
const VArray<bool> &invert_curvature,
const GeometryNodeCurveSampleMode sample_mode,
const bke::AnonymousAttributePropagationInfo & /*propagation_info*/)
Member

If your function has this as an argument, it should be used

If your function has this as an argument, it should be used
ChengduLittleA marked this conversation as resolved
@ -0,0 +50,4 @@
Array<float> use_end_lengths(src_curves_num);
const OffsetIndices<int> points_by_curve = src_curves.points_by_curve();
Array<int> dst_point_counts(src_curves_num + 1);
Member

You can create the new curves earlier and use the new offsets instead of this array

You can create the new curves earlier and use the new offsets instead of this array
Author
Member

I'm not sure I follow... You mean to duplicate source by copy_only_curve_domain(src_curves) and just modify the dst_curves.offsets_for_write() array?

I'm not sure I follow... You mean to duplicate source by `copy_only_curve_domain(src_curves)` and just modify the `dst_curves.offsets_for_write()` array?
Member

Just declare the result curves with copy_only_curve_domain where you currently declare this array, and use the offsets from the new curves the entire time.

Just declare the result curves with `copy_only_curve_domain` where you currently declare this array, and use the offsets from the new curves the entire time.
Author
Member

got it.

got it.
ChengduLittleA marked this conversation as resolved
@ -0,0 +103,4 @@
local_front = start_points[curve];
}
if (end_points[curve]) {
MutableSpan<int> ends = new_points.slice(new_points.size() - end_points[curve],
Member

This crashes in debug builds when drawing a new curve

This crashes in debug builds when drawing a new curve
Author
Member

Hummm I'm still unable to reproduce on debug here in draw mode. Maybe try again?

Hummm I'm still unable to reproduce on debug here in draw mode. Maybe try again?
Member

@HooglyBoogly Is this fixed for you?

@HooglyBoogly Is this fixed for you?
@ -0,0 +124,4 @@
bke::MutableAttributeAccessor dst_attributes = dst_curves.attributes_for_write();
/* Transfer point attributes. */
gather_attributes(
Member

Should be easy to just pass the propagation info here. Also you can exclude "position" if you're copying that with a different method later.

Should be easy to just pass the propagation info here. Also you can exclude "position" if you're copying that with a different method later.
ChengduLittleA marked this conversation as resolved
@ -0,0 +127,4 @@
gather_attributes(
src_attributes, bke::AttrDomain::Point, {}, {}, dst_to_src_point, dst_attributes);
dst_curves.tag_topology_changed();
Member

This is unnecessary for new curves

This is unnecessary for new curves
ChengduLittleA marked this conversation as resolved
@ -0,0 +131,4 @@
MutableSpan<float3> positions = dst_curves.positions_for_write();
const OffsetIndices<int> new_points = dst_curves.points_by_curve();
Member

Try to use the standard names for this sort of thing: new_points -> new_points_by_curve.

If it helps to make the names shorter, typically I only include one of "src" or "new" in a function like this. Every variable associated with the old curves can have a "src" prefix, then the variables associated with "new" curves don't need any prefix at all, since they're different just because they don't have the prefix.

Try to use the standard names for this sort of thing: `new_points` -> `new_points_by_curve`. If it helps to make the names shorter, typically I only include one of "src" or "new" in a function like this. Every variable associated with the old curves can have a "src" prefix, then the variables associated with "new" curves don't need any prefix at all, since they're different just because they don't have the prefix.
ChengduLittleA marked this conversation as resolved
@ -0,0 +132,4 @@
MutableSpan<float3> positions = dst_curves.positions_for_write();
const OffsetIndices<int> new_points = dst_curves.points_by_curve();
for (const int curve : dst_curves.curves_range()) {
Member

Can you parallelize this loop with threading::parallel_for?

Can you parallelize this loop with `threading::parallel_for`?
ChengduLittleA marked this conversation as resolved
@ -0,0 +157,4 @@
// I think It should always be endpoint to overshoot point?
int index1 = math::floor(overshoot_point_param);
int index2 = math::ceil(overshoot_point_param);
result = math::interpolate(positions[new_points[curve][index1]],
Member

Better to store new_points[curve] in a variable so it's not repeated so much and to avoid accessing it many times (nice to explicitly optimize that away even if the compiler may do it)

Better to store `new_points[curve]` in a variable so it's not repeated so much and to avoid accessing it many times (nice to explicitly optimize that away even if the compiler may do it)
ChengduLittleA marked this conversation as resolved
@ -0,0 +164,4 @@
if (UNLIKELY(math::is_zero(result))) {
result = positions[new_points[curve][1]] - positions[new_points[curve][0]];
}
madd_v3_v3fl(
Member

Use the C++ math functions

Use the C++ math functions
ChengduLittleA marked this conversation as resolved
@ -0,0 +205,4 @@
}
const int start_i = k == 0 ? first_old_index :
last_old_index; /* first_old_index, last_old_index */
Member

Not sure what these comments mean

Not sure what these comments mean
ChengduLittleA marked this conversation as resolved
@ -0,0 +305,4 @@
}
}
dst_curves.tag_positions_changed();
Member

Unnecessary to call this on new curves

Unnecessary to call this on new curves
ChengduLittleA marked this conversation as resolved
@ -0,0 +191,4 @@
VArray<bool>::ForSingle(mmd.flag & GP_LENGTH_USE_CURVATURE, curves_num),
VArray<int>::ForSingle(mmd.point_density, curves_num),
VArray<float>::ForSingle(mmd.segment_influence, curves_num),
VArray<float>::ForSingle(mmd.max_angle, curves_num),
Member

I still have the same comment about not unnecessarily making VArray::ForSingle

I still have the same comment about not unnecessarily making `VArray::ForSingle`
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-01-30 03:53:18 +01:00
YimingWu added 1 commit 2024-01-30 03:59:32 +01:00
YimingWu added 1 commit 2024-01-31 02:37:04 +01:00
YimingWu added 1 commit 2024-01-31 02:42:48 +01:00
Falk David requested changes 2024-02-01 12:26:46 +01:00
Falk David left a comment
Member

Did another pass on this.

Did another pass on this.
@ -0,0 +137,4 @@
}
const IndexRange new_curve = new_points_by_curve[curve];
int new_size = new_curve.size();
float used_percent_length = overshoot_fac;
Member

Seems like the two lines here can be combined. Also used_percent_length can be const then.

Seems like the two lines here can be combined. Also `used_percent_length` can be const then.
ChengduLittleA marked this conversation as resolved
@ -0,0 +139,4 @@
int new_size = new_curve.size();
float used_percent_length = overshoot_fac;
used_percent_length = std::clamp(used_percent_length, 1e-4f, 1.0f);
if (!isfinite(used_percent_length)) {
Member

Is this check needed? The value is clamped just the line above.

Is this check needed? The value is clamped just the line above.
Author
Member

Not sure about whether std::clamp clamps inf or nan, not mentioned in the CPP reference, so maybe for safety.

Not sure about whether std::clamp clamps `inf` or `nan`, not mentioned in the [CPP reference](https://en.cppreference.com/w/cpp/algorithm/clamp), so maybe for safety.
Member

You should use math::clamp I missed that.

You should use `math::clamp` I missed that.
Author
Member

Does math one handle inf or nan? Looks like it just straight up calls the std one 😅 I'll change to math nonetheless.

~~Does `math` one handle `inf` or `nan`?~~ Looks like it just straight up calls the `std` one 😅 I'll change to `math` nonetheless.
ChengduLittleA marked this conversation as resolved
@ -0,0 +149,4 @@
used_percent_length = 0.1f;
}
/* Handle simple case first, straight stretching. */
if (!follow_curvature) {
Member

Since this function is getting very long, I would split this if/else out into two static functions. One for the linear extension and one for the curvature extension. Maybe extend_curves_positions_linear and extend_curves_positions_curvature.

Since this function is getting very long, I would split this `if/else` out into two static functions. One for the linear extension and one for the curvature extension. Maybe `extend_curves_positions_linear` and `extend_curves_positions_curvature`.
Author
Member

Hummm that way you'll have a bunch of variables passed into it still...

Hummm that way you'll have a bunch of variables passed into it still...
Member

That's ok :) Now that there is less indentation, I can see better what's happening.

That's ok :) Now that there is less indentation, I can see better what's happening.
ChengduLittleA marked this conversation as resolved
@ -0,0 +130,4 @@
/* Use random to modify start/end factors. Put the modified values outside the
* branch so it could be accessed in later stretching/shrinking stages. */
Array<float> modified_starts;
Member

Seems like these are only used inside the if? They shouldn't be declared outside then.

Seems like these are only used inside the if? They shouldn't be declared outside then.
ChengduLittleA marked this conversation as resolved
@ -0,0 +136,4 @@
if (mmd.rand_start_fac != 0.0 || mmd.rand_end_fac != 0.0) {
modified_starts = Array<float>(curves.curves_num());
modified_ends = Array<float>(curves.curves_num());
modified_starts.fill(mmd.start_fac);
Member

Instead, you can call the constructor with a default value like so: Array<float> modified_starts(curves.curves_num(), mmd.start_fac); Same for the ends.

Instead, you can call the constructor with a default value like so: `Array<float> modified_starts(curves.curves_num(), mmd.start_fac);` Same for the ends.
ChengduLittleA marked this conversation as resolved
@ -0,0 +156,4 @@
Array<float> noise_table_length = noise_table(
4 + curves_num, int(math::floor(mmd.rand_offset)), seed + 2);
for (int i = 0; i < curves_num; i++) {
Member

This should be a threading::parallel_for. You can make needs_additional_shrinking a std::atomic

This should be a `threading::parallel_for`. You can make `needs_additional_shrinking` a `std::atomic`
ChengduLittleA marked this conversation as resolved
@ -0,0 +210,4 @@
needs_removal.fill(false);
curves.ensure_evaluated_lengths();
for (const int curve : curves.curves_range()) {
Member

This can be a threading::parallel_for. The arrays are pre-allocated and have the right length, so writing in parallel to them is ok.

This can be a `threading::parallel_for`. The arrays are pre-allocated and have the right length, so writing in parallel to them is ok.
ChengduLittleA marked this conversation as resolved
@ -0,0 +234,4 @@
/* #trim_curves() will leave the last segment there when trimmed length is greater than
* curve original length, thus we need to remove those curves afterwards. */
IndexMaskMemory memory_remove;
IndexMask to_remove = IndexMask::from_bools(needs_removal.as_span(), memory_remove);
Member

Can be const.

Can be `const`.
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-02-01 13:59:39 +01:00
YimingWu added 1 commit 2024-02-01 14:24:22 +01:00
YimingWu added 1 commit 2024-02-01 14:38:31 +01:00
YimingWu added 1 commit 2024-02-04 03:51:29 +01:00
YimingWu added 1 commit 2024-02-05 10:56:45 +01:00
Hans Goudey requested changes 2024-02-05 15:17:17 +01:00
@ -0,0 +24,4 @@
const float overshoot_fac,
const bool follow_curvature,
const int point_density,
const float segment_influence,
Member

When passing by value, const is meaningless in the definition in the header, and is removed in other headers. (so const float segment_influence -> float segment_influence

When passing by value, `const` is meaningless in the _definition_ in the header, and is removed in other headers. (so `const float segment_influence` -> `float segment_influence`
ChengduLittleA marked this conversation as resolved
@ -0,0 +24,4 @@
namespace blender::geometry {
static auto extend_curves_straight(const float used_percent_length,
Member

Replace auto with the actual return type

Replace `auto` with the actual return type
ChengduLittleA marked this conversation as resolved
@ -0,0 +30,4 @@
const Array<int> &end_points,
const int curve,
const IndexRange new_curve,
const Array<float> &use_start_lengths,
Member

Pass Span instead of Array &

Pass `Span` instead of `Array &`
ChengduLittleA marked this conversation as resolved
@ -0,0 +38,4 @@
float3 result;
if (start_points[curve]) {
// TODO: I don't think the algorithm here is correct?
// I think It should always be endpoint to overshoot point?
Member

The algorithm is just the same as the old modifier, right?

The algorithm is just the same as the old modifier, right?
Author
Member

Yes. The comment was there for the old modifier.

I'll update the comment to better reflect on what's going on. This can be a improvement downstream.

Yes. The comment was there for the old modifier. I'll update the comment to better reflect on what's going on. This can be a improvement downstream.
ChengduLittleA marked this conversation as resolved
@ -0,0 +78,4 @@
const bool invert_curvature,
MutableSpan<float3> positions)
{
Member

Unnecessary newline

Unnecessary newline
ChengduLittleA marked this conversation as resolved
@ -0,0 +230,4 @@
int point_count = points_by_curve[curve].size();
dst_point_offsets[curve] = point_count;
/* Curve not suitable for stretching... */
if (point_count <= 2 || (!selection.contains(curve))) {
Member

Rather than calling selection.contains (which has logarithmic runtime), iterate over the index mask directly. Looks like this loop can be parallel too

Rather than calling `selection.contains` (which has logarithmic runtime), iterate over the index mask directly. Looks like this loop can be parallel too
ChengduLittleA marked this conversation as resolved
@ -0,0 +234,4 @@
continue;
}
float density = point_density;
if (density < 1e-4f) {
Member

Looks like this is different than the clamping in the legacy modifier.

const float density = std::clamp(point_density, 1e-4f, 1.0f); seems like the old logic?

Looks like this is different than the clamping in the legacy modifier. `const float density = std::clamp(point_density, 1e-4f, 1.0f);` seems like the old logic?
Author
Member

the 1e-4f should be the used_percent_length clamp, I don't know why I have this here but looks like it should not be clamped.

the `1e-4f` should be the `used_percent_length` clamp, I don't know why I have this here but looks like it should not be clamped.
ChengduLittleA marked this conversation as resolved
@ -0,0 +292,4 @@
gather_attributes(src_attributes,
bke::AttrDomain::Point,
propagation_info,
{}, //{"position"},
Member

Why is this commented?

Why is this commented?
ChengduLittleA marked this conversation as resolved
@ -0,0 +309,4 @@
int new_size = new_curve.size();
/* #used_percent_length must always be finite, otherwise a segfault occurs.
* Since this function should never segfault, set #used_percent_length to a safe fallback.
Member

Since this function should never segfault

No function should ever segfault, this comment is pointless

>Since this function should never segfault No function should ever segfault, this comment is pointless
ChengduLittleA marked this conversation as resolved
@ -0,0 +312,4 @@
* Since this function should never segfault, set #used_percent_length to a safe fallback.
*/
/* NOTE: This fallback is used if `gps->totpoints == 2`, see
* `MOD_gpencil_legacy_length.cc`.
Member

These legacy files will be deleted, I wouldn't bother adding a reference to them here

These legacy files will be deleted, I wouldn't bother adding a reference to them here
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-02-06 03:31:45 +01:00
Falk David added 1 commit 2024-02-06 11:54:23 +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
23f0e11f37
Merge branch 'main' into gp3-length-modifier
Member

@blender-bot build

@blender-bot build
Member

While testing this, I ran into some issues when turning Curvature off:

  • Setting Start or End to 0 I get a crash: BLI_assert failed: source/blender/blenlib/BLI_length_parameterize.hh:105, sample_at_length(), at 'sample_length >= 0.0f'.
  • The strokes are not extended properly. Points are jumping around randomly.
While testing this, I ran into some issues when turning `Curvature` off: * Setting `Start` or `End` to 0 I get a crash: `BLI_assert failed: source/blender/blenlib/BLI_length_parameterize.hh:105, sample_at_length(), at 'sample_length >= 0.0f'`. * The strokes are not extended properly. Points are jumping around randomly.
Member

Also:

/.../blender-git/blender/source/blender/geometry/intern/extend_curves.cc:53:68: runtime error: division by zero
/.../blender-git/blender/source/blender/geometry/intern/extend_curves.cc:66:77: runtime error: division by zero
Also: ``` /.../blender-git/blender/source/blender/geometry/intern/extend_curves.cc:53:68: runtime error: division by zero /.../blender-git/blender/source/blender/geometry/intern/extend_curves.cc:66:77: runtime error: division by zero ```
Author
Member

Looks like the problem is that I should not add new points when it's extending straight.

Looks like the problem is that I should not add new points when it's extending straight.
YimingWu added 1 commit 2024-02-06 13:02:43 +01:00
Falk David approved these changes 2024-02-08 14:46:26 +01:00
Falk David left a comment
Member

Code in the modifier looks ok to me.

Code in the modifier looks ok to me.
Hans Goudey approved these changes 2024-02-08 16:26:22 +01:00
Hans Goudey left a comment
Member

Found some smaller things, doesn't need another round of review from me though.

Found some smaller things, doesn't need another round of review from me though.
@ -0,0 +48,4 @@
if (index2 == 0) {
index2 = 1;
}
result = math::interpolate(positions[new_curve[index1]],
Member

Declare result here

Declare `result` here
ChengduLittleA marked this conversation as resolved
@ -0,0 +55,4 @@
if (UNLIKELY(math::is_zero(result))) {
result = positions[new_curve[1]] - positions[new_curve[0]];
}
positions[new_curve[0]] += result * (-use_start_lengths[curve] / len_v3(result));
Member

len_v3 -> math::length

`len_v3` -> `math::length`
ChengduLittleA marked this conversation as resolved
@ -0,0 +75,4 @@
static void extend_curves_curved(const float used_percent_length,
const Span<int> start_points,
const Span<int> end_points,
const OffsetIndices<int> &points_by_curve,
Member

Pass OffsetIndices by value, just like Span, for the same reasons

Pass `OffsetIndices` by value, just like `Span`, for the same reasons
ChengduLittleA marked this conversation as resolved
@ -0,0 +268,4 @@
end_points[curve] = count_end;
});
OffsetIndices dst_indicies = offset_indices::accumulate_counts_to_offsets(dst_point_offsets);
Member

Typo dst_indicies -> dst_indices

Also call this dst_points_by_curve to be consistent

Typo `dst_indicies` -> `dst_indices` Also call this `dst_points_by_curve` to be consistent
ChengduLittleA marked this conversation as resolved
YimingWu added 1 commit 2024-02-15 13:41:53 +01:00
YimingWu added 1 commit 2024-02-15 13:48:35 +01:00
YimingWu added 1 commit 2024-02-15 14:35:55 +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
350bdcc267
Merge branch 'main' into gp3-length-modifier
Member

@blender-bot build

@blender-bot build
Falk David merged commit b661b368c4 into main 2024-02-15 16:38:05 +01:00
Falk David referenced this issue from a commit 2024-02-15 16:38:05 +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#117124
No description provided.