blender
/
blender
136
411
Fork 595
Code Issues 6.3k Pull Requests 770 Projects 19 Wiki Activity

Fix #104842: Incorrect cyclic curve parameter node factor #105079

Closed
Hans Goudey wants to merge 3 commits from HooglyBoogly:fix-curve-parameter-cyclic into blender-v3.5-release
pull from: HooglyBoogly:fix-curve-parameter-cyclic
merge into: blender:blender-v3.5-release

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Conversation 1 Commits 3 Files Changed 1 +40 -32
1 changed files with 40 additions and 32 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

72
source/blender/nodes/geometry/nodes/node_geo_curve_spline_parameter.cc
View File

@ -80,7 +80,9 @@ static Array<float> calculate_curve_parameters(const bke::CurvesGeometry &curves
* - NURBS Curves: Treat the control points as if they were a poly curve, because there * - NURBS Curves: Treat the control points as if they were a poly curve, because there
* is no obvious mapping from each control point to a specific evaluated point. * is no obvious mapping from each control point to a specific evaluated point.
*/ */
static Array<float> calculate_point_lengths(const bke::CurvesGeometry &curves) static Array<float> calculate_point_lengths(
const bke::CurvesGeometry &curves,
const FunctionRef<void(MutableSpan<float>, float)> postprocess_lengths_for_curve)
HooglyBoogly marked this conversation as resolved
Jacques Lucke commented 2023-02-22 17:59:06 +01:00
Review
Copy Link

Bit unconventional but seems ok with improved naming and comment. Maybe use total_curve_length and postprocess_lengths_for_curve.

Bit unconventional but seems ok with improved naming and comment. Maybe use `total_curve_length` and `postprocess_lengths_for_curve`.
Hans Goudey commented 2023-02-22 18:06:31 +01:00
Review
Copy Link

I thought going over the lengths with a single pass using a callback instead of two passes might improve performance. Didn't notice any difference though. It matches what's happening conceptually fairly well though at least. Thanks for the naming suggestions.

I thought going over the lengths with a single pass using a callback instead of two passes might improve performance. Didn't notice any difference though. It matches what's happening conceptually fairly well though at least. Thanks for the naming suggestions.
{ {
curves.ensure_evaluated_lengths(); curves.ensure_evaluated_lengths();
const OffsetIndices points_by_curve = curves.points_by_curve(); const OffsetIndices points_by_curve = curves.points_by_curve();
@ -93,26 +95,31 @@ static Array<float> calculate_point_lengths(const bke::CurvesGeometry &curves)
threading::parallel_for(curves.curves_range(), 128, [&](IndexRange range) { threading::parallel_for(curves.curves_range(), 128, [&](IndexRange range) {
for (const int i_curve : range) { for (const int i_curve : range) {
const IndexRange points = points_by_curve[i_curve]; const IndexRange points = points_by_curve[i_curve];
const Span<float> evaluated_lengths = curves.evaluated_lengths_for_curve(i_curve, const bool is_cyclic = cyclic[i_curve];
cyclic[i_curve]); const Span<float> evaluated_lengths = curves.evaluated_lengths_for_curve(i_curve, is_cyclic);
MutableSpan<float> lengths = result.as_mutable_span().slice(points); MutableSpan<float> lengths = result.as_mutable_span().slice(points);
lengths.first() = 0.0f; lengths.first() = 0.0f;
float total;
switch (types[i_curve]) { switch (types[i_curve]) {
case CURVE_TYPE_CATMULL_ROM: { case CURVE_TYPE_CATMULL_ROM: {
const int resolution = resolutions[i_curve]; const int resolution = resolutions[i_curve];
for (const int i : IndexRange(points.size()).drop_back(1)) { for (const int i : IndexRange(points.size()).drop_back(1)) {
lengths[i + 1] = evaluated_lengths[resolution * (i + 1) - 1]; lengths[i + 1] = evaluated_lengths[resolution * (i + 1) - 1];
} }
total = evaluated_lengths.last();
break; break;
} }
case CURVE_TYPE_POLY: case CURVE_TYPE_POLY:
lengths.drop_front(1).copy_from(evaluated_lengths.take_front(lengths.size() - 1)); lengths.drop_front(1).copy_from(evaluated_lengths.take_front(lengths.size() - 1));
total = evaluated_lengths.last();
break; break;
case CURVE_TYPE_BEZIER: { case CURVE_TYPE_BEZIER: {
const Span<int> offsets = curves.bezier_evaluated_offsets_for_curve(i_curve); const Span<int> offsets = curves.bezier_evaluated_offsets_for_curve(i_curve);
for (const int i : IndexRange(points.size()).drop_back(1)) { for (const int i : IndexRange(points.size()).drop_back(1)) {
lengths[i + 1] = evaluated_lengths[offsets[i + 1] - 1]; lengths[i + 1] = evaluated_lengths[offsets[i + 1] - 1];
} }
total = evaluated_lengths.last();
break; break;
} }
case CURVE_TYPE_NURBS: { case CURVE_TYPE_NURBS: {
@ -123,44 +130,44 @@ static Array<float> calculate_point_lengths(const bke::CurvesGeometry &curves)
length += math::distance(positions[i], positions[i + 1]); length += math::distance(positions[i], positions[i + 1]);
} }
lengths.last() = length; lengths.last() = length;
if (is_cyclic) {
length += math::distance(positions.first(), positions.last());
}
total = length;
break; break;
} }
} }
postprocess_lengths_for_curve(lengths, total);
} }
}); });
return result; return result;
} }
static void convert_lengths_to_factors(MutableSpan<float> lengths, const float total_curve_length)
{
if (total_curve_length > 0.0f) {
const float factor = 1.0f / total_curve_length;
for (float &value : lengths.drop_front(1)) {
value *= factor;
}
}
else if (lengths.size() == 1) {
/* The curve is a single point. */
lengths[0] = 0.0f;
}
else {
/* It is arbitrary what to do in those rare cases when all the
* points are in the same position. In this case we are just
* arbitrarily giving a valid
* value in the range based on the point index. */
for (const int i : lengths.index_range()) {
lengths[i] = i / (lengths.size() - 1.0f);
}
}
}
static Array<float> calculate_point_parameters(const bke::CurvesGeometry &curves) static Array<float> calculate_point_parameters(const bke::CurvesGeometry &curves)
{ {
Array<float> lengths = calculate_point_lengths(curves); return calculate_point_lengths(curves, convert_lengths_to_factors);
const OffsetIndices points_by_curve = curves.points_by_curve();
threading::parallel_for(curves.curves_range(), 1024, [&](IndexRange range) {
for (const int i_curve : range) {
MutableSpan<float> curve_lengths = lengths.as_mutable_span().slice(points_by_curve[i_curve]);
const float total_length = curve_lengths.last();
if (total_length > 0.0f) {
const float factor = 1.0f / total_length;
for (float &value : curve_lengths) {
value *= factor;
}
}
else if (curve_lengths.size() == 1) {
/* The curve is a single point. */
curve_lengths[0] = 0.0f;
}
else {
/* It is arbitrary what to do in those rare cases when all the points are
* in the same position. In this case we are just arbitrarily giving a valid
* value in the range based on the point index. */
for (const int i : curve_lengths.index_range()) {
curve_lengths[i] = i / (curve_lengths.size() - 1.0f);
}
}
}
});
return lengths;
} }
class CurveParameterFieldInput final : public bke::CurvesFieldInput { class CurveParameterFieldInput final : public bke::CurvesFieldInput {
@ -210,7 +217,8 @@ class CurveLengthParameterFieldInput final : public bke::CurvesFieldInput {
{ {
switch (domain) { switch (domain) {
case ATTR_DOMAIN_POINT: case ATTR_DOMAIN_POINT:
return VArray<float>::ForContainer(calculate_point_lengths(curves)); return VArray<float>::ForContainer(calculate_point_lengths(
curves, [](MutableSpan<float> /*lengths*/, const float /*total*/) {}));
case ATTR_DOMAIN_CURVE: case ATTR_DOMAIN_CURVE:
return VArray<float>::ForContainer(accumulated_lengths_curve_domain(curves)); return VArray<float>::ForContainer(accumulated_lengths_curve_domain(curves));
default: default: