From 5888ae5d5d6bfb6e5451ef9437b360155126db64 Mon Sep 17 00:00:00 2001 From: Laurynas Duburas Date: Wed, 30 Apr 2025 08:12:20 +0200 Subject: [PATCH] Fix: Curves: wrong behavior in split operator Fixes problem with cyclic curves when non selected point is on (or near) loop stitch. Also fixes crash when all points are selected. Pull Request: https://projects.blender.org/blender/blender/pulls/137931 --- .../editors/curves/intern/curves_edit.cc | 14 ++- .../editors/curves/tests/curves_edit_test.cc | 108 +++++++++++++++++- 2 files changed, 114 insertions(+), 8 deletions(-) diff --git a/source/blender/editors/curves/intern/curves_edit.cc b/source/blender/editors/curves/intern/curves_edit.cc index 137355d1f0f..ae33a69c002 100644 --- a/source/blender/editors/curves/intern/curves_edit.cc +++ b/source/blender/editors/curves/intern/curves_edit.cc @@ -51,6 +51,9 @@ static void curve_offsets_from_selection(const Span selected_points, Vector &r_dst_offsets, Vector &r_dst_to_src_curve) { + if (selected_points.is_empty()) { + return; + } const bool merge_loop = cyclic && selected_points.first().size() < points.size() && selected_points.first().first() == points.first() && selected_points.last().last() == points.last(); @@ -423,7 +426,6 @@ bke::CurvesGeometry split_points(const bke::CurvesGeometry &curves, points_to_split, points_by_curve, [&](const int curve, const IndexRange points, const Span selected_curve_points) { - const int points_start = new_offsets.last(); curve_offsets_from_selection(selected_curve_points, points, curve, @@ -433,7 +435,6 @@ bke::CurvesGeometry split_points(const bke::CurvesGeometry &curves, src_ranges, dst_offsets, curve_map); - const int split_points_num = new_offsets.last() - points_start; /* Invert ranges to get non selected points. */ invert_ranges(points, selected_curve_points, unselected_curve_points); /* Extended every range to left and right by one point. Any resulting intersection is @@ -441,11 +442,16 @@ bke::CurvesGeometry split_points(const bke::CurvesGeometry &curves, extend_range_by_1_within_bounds( points, cyclic[curve], unselected_curve_points, curve_points_to_preserve); const int size_before = curve_map.size(); + /* Unselected part can contain all points from original curve, but have cuts. This happens + * when pairs of adjacent points are selected. To prevent loop merge and result curve from + * cyclic additional condition is checked. */ + const bool can_merge_loop = !unselected_curve_points.is_empty() && + (unselected_curve_points.first().first() == points.first() || + unselected_curve_points.last().last() == points.last()); curve_offsets_from_selection(curve_points_to_preserve, points, curve, - cyclic[curve] && - (split_points_num <= curve_points_to_preserve.size()), + cyclic[curve] && can_merge_loop, new_offsets, new_cyclic, src_ranges, diff --git a/source/blender/editors/curves/tests/curves_edit_test.cc b/source/blender/editors/curves/tests/curves_edit_test.cc index fe29556c733..8ae0f5218d1 100644 --- a/source/blender/editors/curves/tests/curves_edit_test.cc +++ b/source/blender/editors/curves/tests/curves_edit_test.cc @@ -162,7 +162,7 @@ TEST(curves_editors, SplitPointsTwoSingle) const Vector> expected_positions = { {{-1, 1, 0}, {1, 1, 0}}, {{-1.5, 0, 0}, {-1, 1, 0}}, {{1, 1, 0}, {1.5, 0, 0}}}; - EXPECT_EQ(new_curves.curves_num(), expected_positions.size()); + GTEST_ASSERT_EQ(new_curves.curves_num(), expected_positions.size()); validate_positions(expected_positions, new_curves.points_by_curve(), new_curves.positions()); } @@ -187,7 +187,7 @@ TEST(curves_editors, SplitPointsFourThree) {{1, -1, 0}}, {{-1.5, 0, 0}, {-1, 1, 0}, {1, 1, 0}, {1.5, 0, 0}, {1, -1, 0}}}; - EXPECT_EQ(new_curves.curves_num(), expected_positions.size()); + GTEST_ASSERT_EQ(new_curves.curves_num(), expected_positions.size()); validate_positions(expected_positions, new_curves.points_by_curve(), new_curves.positions()); } @@ -213,7 +213,7 @@ TEST(curves_editors, SplitPointsTwoCyclic) {{1, 1, 0}, {1, -1, 0}, {-1, -1, 0}, {-1, 1, 0}}, {{-1.5, 0, 0}, {-1, 1, 0}, {1, 1, 0}, {1.5, 0, 0}, {1, -1, 0}}}; - EXPECT_EQ(new_curves.curves_num(), expected_positions.size()); + GTEST_ASSERT_EQ(new_curves.curves_num(), expected_positions.size()); validate_positions(expected_positions, new_curves.points_by_curve(), new_curves.positions()); Array expected_cyclic = {false, false, false, false, false}; VArray cyclic = new_curves.cyclic(); @@ -244,8 +244,108 @@ TEST(curves_editors, SplitPointsTwoTouchCyclic) {{1, -1, 0}, {-1, -1, 0}, {-1, 1, 0}, {1, 1, 0}}, {{-1.5, 0, 0}, {-1, 1, 0}, {1, 1, 0}, {1.5, 0, 0}, {1, -1, 0}}}; - EXPECT_EQ(new_curves.curves_num(), expected_positions.size()); + GTEST_ASSERT_EQ(new_curves.curves_num(), expected_positions.size()); validate_positions(expected_positions, new_curves.points_by_curve(), new_curves.positions()); } +TEST(curves_editors, SplitEverySecondCyclic) +{ + /* Split every second point in cyclic curve. Expected result all selected points + * as separate curves and original curve. */ + const Vector> positions = {{{0, -1, 0}, + {-1, -1, 0}, + {-1, 0, 0}, + {-1, 1, 0}, + {0, 1, 0}, + {1, 1, 0}, + {1, 0, 0}, + {1, -1, 0}}}; + + bke::CurvesGeometry curves = create_curves(positions, 4, {0}); + IndexMaskMemory memory; + const IndexMask mask = IndexMask::from_indices(Array{0, 2, 4, 6}.as_span(), memory); + + bke::CurvesGeometry new_curves = split_points(curves, mask); + + const Vector> expected_positions = {{{0, -1, 0}}, + {{-1, 0, 0}}, + {{0, 1, 0}}, + {{1, 0, 0}}, + {{0, -1, 0}, + {-1, -1, 0}, + {-1, 0, 0}, + {-1, 1, 0}, + {0, 1, 0}, + {1, 1, 0}, + {1, 0, 0}, + {1, -1, 0}}}; + + GTEST_ASSERT_EQ(new_curves.curves_num(), expected_positions.size()); + validate_positions(expected_positions, new_curves.points_by_curve(), new_curves.positions()); +} + +TEST(curves_editors, SplitAllSelectedButFirstCyclic) +{ + /* Split all except first points in cyclic curve. Expected result two curves. One from selected + * points another from first, second and last. Both not cyclic. */ + const Vector> positions = {{{0, -1, 0}, + {-1, -1, 0}, + {-1, 0, 0}, + {-1, 1, 0}, + {0, 1, 0}, + {1, 1, 0}, + {1, 0, 0}, + {1, -1, 0}}}; + + bke::CurvesGeometry curves = create_curves(positions, 4, {0}); + IndexMaskMemory memory; + const IndexMask mask = IndexMask::from_indices(Array{1, 2, 3, 4, 5, 6, 7}.as_span(), + memory); + + bke::CurvesGeometry new_curves = split_points(curves, mask); + + const Vector> expected_positions = { + {{-1, -1, 0}, {-1, 0, 0}, {-1, 1, 0}, {0, 1, 0}, {1, 1, 0}, {1, 0, 0}, {1, -1, 0}}, + {{1, -1, 0}, {0, -1, 0}, {-1, -1, 0}}, + }; + + GTEST_ASSERT_EQ(new_curves.curves_num(), expected_positions.size()); + validate_positions(expected_positions, new_curves.points_by_curve(), new_curves.positions()); + EXPECT_EQ(new_curves.curves_num(), expected_positions.size()); + EXPECT_FALSE(new_curves.cyclic()[0]); + EXPECT_FALSE(new_curves.cyclic()[1]); +} + +TEST(curves_editors, SplitTwoOnSeamAndExtraCyclic) +{ + /* Split first, last and pair in the middle. Expected result four non cyclic curves. */ + const Vector> positions = {{{0, -1, 0}, + {-1, -1, 0}, + {-1, 0, 0}, + {-1, 1, 0}, + {0, 1, 0}, + {1, 1, 0}, + {1, 0, 0}, + {1, -1, 0}}}; + + bke::CurvesGeometry curves = create_curves(positions, 4, {0}); + IndexMaskMemory memory; + const IndexMask mask = IndexMask::from_indices(Array{0, 3, 4, 7}.as_span(), memory); + + bke::CurvesGeometry new_curves = split_points(curves, mask); + + const Vector> expected_positions = { + {{-1, 1, 0}, {0, 1, 0}}, + {{1, -1, 0}, {0, -1, 0}}, + {{0, -1, 0}, {-1, -1, 0}, {-1, 0, 0}, {-1, 1, 0}}, + {{0, 1, 0}, {1, 1, 0}, {1, 0, 0}, {1, -1, 0}}}; + + GTEST_ASSERT_EQ(new_curves.curves_num(), expected_positions.size()); + validate_positions(expected_positions, new_curves.points_by_curve(), new_curves.positions()); + EXPECT_FALSE(new_curves.cyclic()[0]); + EXPECT_FALSE(new_curves.cyclic()[1]); + EXPECT_FALSE(new_curves.cyclic()[2]); + EXPECT_FALSE(new_curves.cyclic()[3]); +} + } // namespace blender::ed::curves::tests