GPv3: Length modifier #117124
No reviewers
Labels
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#117124
Loading…
Reference in New Issue
No description provided.
Delete Branch "ChengduLittleA/blender:gp3-length-modifier"
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?
Length modifier ported to Grease Pencil v3
All features now work as expected
@ -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());
So here it seems to have some problems.
@ -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()){
Looks like
do_curves
isn't necessary. You can copy the existing point counts withcopy_group_sizes
(using the full curves mask). Then iterate through the selection mask, increasing the point count for those curves as necessary@ -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]);
.slice(dst_point_offsets[curve])
@ -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());
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 ofdst_point_counts
@ -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();
You didn't change the types here, so no need for
remove_attributes_based_on_types
WIP: GPv3: Length modifierto GPv3: Length modifier@ -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.
@ -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))) {
Use C++ math vector API
@ -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);
std::clamp
@ -0,0 +193,4 @@
CLAMP(overshoot_pointcount, 1, orig_totpoints - 2);
/* Do for both sides without code duplication. */
float3 no, vec1, vec2, total_angle;
Declare variables where their values are assigned
@ -0,0 +200,4 @@
}
const int start_i = k == 0 ? first_old_index :
last_old_index; // first_old_index, last_old_index
Comment style
@ -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. */
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.@ -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];
Use the C++ matrix and rotaiton API here, that will give us less work to do in the future
Not sure the equivalent of
quat_to_mat3
andaxis_angle_to_quat
inmath::
.Tried following, doesn't seem to work:
@ -2610,0 +2612,4 @@
typedef struct GreasePencilLengthModifierData {
ModifierData modifier;
GreasePencilModifierInfluenceData influence;
/** Flags. */
This comment isn't helpful. Best to save comments for when they explain something non-obvious.
@ -2610,0 +2624,4 @@
int seed;
/** How many frames before recalculate randoms. */
int step;
/** Modifier mode. */
Reference the enum type instead, like
/** #EnumType. */
@ -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++) {
for (const int i : table.index_range())
@ -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));
Array<float> &
->MutableSpan<float>
@ -0,0 +107,4 @@
}
static void deform_drawing(ModifierData &md,
Depsgraph *depsgraph,
Use references and const
@ -0,0 +122,4 @@
const IndexMask selection = modifier::greasepencil::get_filtered_stroke_mask(
&ob, curves, mmd.influence, memory);
const int cnum = curves.curves_num();
cnum
->curves_num
@ -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);
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.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 tomodified_starts
array when this happens.I'm confused, in the old modifier all these were just a single value for the whole modifier:
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.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.
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.
@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.@ChengduLittleA I suspect that you can call
remove_curves
for the curves that get fully trimmed away.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,
These changes should be reverted.
@ -1135,3 +1132,1 @@
const Span<int> offsets,
const Span<int> materials,
const float4x4 &matrix)
bke::CurvesGeometry create_drawing_data(const Span<float3> positions,
Looks like these changes aren't needed.
@ -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,
Same as above.
@ -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,
A better name would probably be
extend_curves
I'll also change the file name.
@ -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')
Use alphabetical order
@ -40,12 +40,14 @@ set(SRC
intern/set_curve_type.cc
intern/smooth_curves.cc
intern/subdivide_curves.cc
intern/extend_curves.cc
Use alphabetical order
@ -0,0 +14,4 @@
namespace blender::geometry {
/*
* Create a new Curves instance by trimming the input curves. Copying the selected splines
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.
@ -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"
If possible, try to only include the new math headers, that will make it clearer to only use the newer API
@ -0,0 +37,4 @@
const VArray<float> &max_angle,
const VArray<bool> &invert_curvature,
const GeometryNodeCurveSampleMode sample_mode,
const bke::AnonymousAttributePropagationInfo & /*propagation_info*/)
If your function has this as an argument, it should be used
@ -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);
You can create the new curves earlier and use the new offsets instead of this array
I'm not sure I follow... You mean to duplicate source by
copy_only_curve_domain(src_curves)
and just modify thedst_curves.offsets_for_write()
array?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.got it.
@ -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],
This crashes in debug builds when drawing a new curve
Hummm I'm still unable to reproduce on debug here in draw mode. Maybe try again?
@HooglyBoogly Is this fixed for you?
@ -0,0 +124,4 @@
bke::MutableAttributeAccessor dst_attributes = dst_curves.attributes_for_write();
/* Transfer point attributes. */
gather_attributes(
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.
@ -0,0 +127,4 @@
gather_attributes(
src_attributes, bke::AttrDomain::Point, {}, {}, dst_to_src_point, dst_attributes);
dst_curves.tag_topology_changed();
This is unnecessary for new curves
@ -0,0 +131,4 @@
MutableSpan<float3> positions = dst_curves.positions_for_write();
const OffsetIndices<int> new_points = dst_curves.points_by_curve();
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.
@ -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()) {
Can you parallelize this loop with
threading::parallel_for
?@ -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]],
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)@ -0,0 +164,4 @@
if (UNLIKELY(math::is_zero(result))) {
result = positions[new_points[curve][1]] - positions[new_points[curve][0]];
}
madd_v3_v3fl(
Use the C++ math functions
@ -0,0 +205,4 @@
}
const int start_i = k == 0 ? first_old_index :
last_old_index; /* first_old_index, last_old_index */
Not sure what these comments mean
@ -0,0 +305,4 @@
}
}
dst_curves.tag_positions_changed();
Unnecessary to call this on new curves
@ -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),
I still have the same comment about not unnecessarily making
VArray::ForSingle
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;
Seems like the two lines here can be combined. Also
used_percent_length
can be const then.@ -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)) {
Is this check needed? The value is clamped just the line above.
Not sure about whether std::clamp clamps
inf
ornan
, not mentioned in the CPP reference, so maybe for safety.You should use
math::clamp
I missed that.DoesLooks like it just straight up calls themath
one handleinf
ornan
?std
one 😅 I'll change tomath
nonetheless.@ -0,0 +149,4 @@
used_percent_length = 0.1f;
}
/* Handle simple case first, straight stretching. */
if (!follow_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. Maybeextend_curves_positions_linear
andextend_curves_positions_curvature
.Hummm that way you'll have a bunch of variables passed into it still...
That's ok :) Now that there is less indentation, I can see better what's happening.
@ -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;
Seems like these are only used inside the if? They shouldn't be declared outside then.
@ -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);
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.@ -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++) {
This should be a
threading::parallel_for
. You can makeneeds_additional_shrinking
astd::atomic
@ -0,0 +210,4 @@
needs_removal.fill(false);
curves.ensure_evaluated_lengths();
for (const int curve : curves.curves_range()) {
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.@ -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);
Can be
const
.@ -0,0 +24,4 @@
const float overshoot_fac,
const bool follow_curvature,
const int point_density,
const float segment_influence,
When passing by value,
const
is meaningless in the definition in the header, and is removed in other headers. (soconst float segment_influence
->float segment_influence
@ -0,0 +24,4 @@
namespace blender::geometry {
static auto extend_curves_straight(const float used_percent_length,
Replace
auto
with the actual return type@ -0,0 +30,4 @@
const Array<int> &end_points,
const int curve,
const IndexRange new_curve,
const Array<float> &use_start_lengths,
Pass
Span
instead ofArray &
@ -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?
The algorithm is just the same as the old modifier, right?
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.
@ -0,0 +78,4 @@
const bool invert_curvature,
MutableSpan<float3> positions)
{
Unnecessary newline
@ -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))) {
Rather than calling
selection.contains
(which has logarithmic runtime), iterate over the index mask directly. Looks like this loop can be parallel too@ -0,0 +234,4 @@
continue;
}
float density = point_density;
if (density < 1e-4f) {
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?the
1e-4f
should be theused_percent_length
clamp, I don't know why I have this here but looks like it should not be clamped.@ -0,0 +292,4 @@
gather_attributes(src_attributes,
bke::AttrDomain::Point,
propagation_info,
{}, //{"position"},
Why is this commented?
@ -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.
No function should ever segfault, this comment is pointless
@ -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`.
These legacy files will be deleted, I wouldn't bother adding a reference to them here
@blender-bot build
While testing this, I ran into some issues when turning
Curvature
off:Start
orEnd
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'
.Also:
Looks like the problem is that I should not add new points when it's extending straight.
Code in the modifier looks ok to me.
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]],
Declare
result
here@ -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));
len_v3
->math::length
@ -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,
Pass
OffsetIndices
by value, just likeSpan
, for the same reasons@ -0,0 +268,4 @@
end_points[curve] = count_end;
});
OffsetIndices dst_indicies = offset_indices::accumulate_counts_to_offsets(dst_point_offsets);
Typo
dst_indicies
->dst_indices
Also call this
dst_points_by_curve
to be consistent@blender-bot build