From ea10c84d6ba328063db242fa19b589f8824861ff Mon Sep 17 00:00:00 2001 From: Ian Bassi Date: Sat, 10 Oct 2026 16:03:34 -0300 Subject: [PATCH] Fix scale and mirror of rotated mirrored objects (#16131) * Fix decomposition of mirrored transformations computeRotationScaling() may place the mirror on any axis, giving unstable rotation/scale for mirrored matrices. Decompose them by flipping each local axis and choosing the one needing the least rotation (lowest axis on ties). Merge the extract helpers into a single extract_rotation_scale() and simplify contains_skew() to check that the scale is diagonal. set_scaling_factor() now preserves the mirror sign on the scaled axis. Selection::scale_and_translate() uses Transformation's rotation and scale instead of calling computeRotationScaling() directly, so scaling a mirrored instance adds no skew. Add tests for mirrored decomposition, scaling, and skew detection. * Use Eigen polar decomposition for instance scaling Selection::scale_and_translate now splits the instance matrix with computeRotationScaling instead of get_rotation_matrix(). Adds a Model test checking that an instance created from another's offset, scale, rotation and mirror reproduces its matrix (as Add instance and Fill bed do). * tidy --- src/libslic3r/Geometry.cpp | 72 ++++++++---------- tests/libslic3r/test_geometry.cpp | 119 ++++++++++++++++++++++++++++++ tests/libslic3r/test_model.cpp | 26 +++++++ 3 files changed, 176 insertions(+), 41 deletions(-) diff --git a/src/libslic3r/Geometry.cpp b/src/libslic3r/Geometry.cpp index ca134eda79..e8c64867d0 100644 --- a/src/libslic3r/Geometry.cpp +++ b/src/libslic3r/Geometry.cpp @@ -428,55 +428,44 @@ Transform3d Transformation::get_offset_matrix() const return translation_transform(get_offset()); } -static Transform3d extract_rotation_matrix(const Transform3d& trafo) -{ - Matrix3d rotation; - Matrix3d scale; - trafo.computeRotationScaling(&rotation, &scale); - return Transform3d(rotation); -} - -static Transform3d extract_scale(const Transform3d& trafo) -{ - Matrix3d rotation; - Matrix3d scale; - trafo.computeRotationScaling(&rotation, &scale); - return Transform3d(scale); -} - +// computeRotationScaling() may put a mirror on any axis; use the local axis needing the least rotation. static std::pair extract_rotation_scale(const Transform3d& trafo) { Matrix3d rotation; Matrix3d scale; - trafo.computeRotationScaling(&rotation, &scale); + if (trafo.linear().determinant() >= 0.0) { + trafo.computeRotationScaling(&rotation, &scale); + return { Transform3d(rotation), Transform3d(scale) }; + } + for (int axis = 0; axis < 3; ++axis) { + Transform3d flipped = trafo; + flipped.linear().col(axis) *= -1.0; + Matrix3d r; + Matrix3d s; + flipped.computeRotationScaling(&r, &s); + // EPSILON keeps the lowest axis on ties, e.g. a mirror rotated by 90 degrees. + if (axis == 0 || r.trace() > rotation.trace() + EPSILON) { + rotation = r; + scale = s; + scale.col(axis) *= -1.0; + } + } return { Transform3d(rotation), Transform3d(scale) }; } +static Transform3d extract_rotation_matrix(const Transform3d& trafo) +{ + return extract_rotation_scale(trafo).first; +} + +static Transform3d extract_scale(const Transform3d& trafo) +{ + return extract_rotation_scale(trafo).second; +} + static bool contains_skew(const Transform3d& trafo) { - Matrix3d rotation; - Matrix3d scale; - trafo.computeRotationScaling(&rotation, &scale); - - if (scale.isDiagonal()) - return false; - - if (scale.determinant() >= 0.0) - return true; - - // the matrix contains mirror - const Matrix3d ratio = scale.cwiseQuotient(trafo.matrix().block<3,3>(0,0)); - - auto check_skew = [&ratio](int i, int j, bool& skew) { - if (!std::isnan(ratio(i, j)) && !std::isnan(ratio(j, i))) - skew |= std::abs(ratio(i, j) * ratio(j, i) - 1.0) > EPSILON; - }; - - bool has_skew = false; - check_skew(0, 1, has_skew); - check_skew(0, 2, has_skew); - check_skew(1, 2, has_skew); - return has_skew; + return !extract_scale(trafo).linear().isDiagonal(); } Vec3d Transformation::get_rotation() const @@ -550,7 +539,8 @@ void Transformation::set_scaling_factor(Axis axis, double scaling_factor) assert(scaling_factor > 0.0); auto [rotation, scale] = extract_rotation_scale(m_matrix); - scale(axis, axis) = scaling_factor; + // Keep a mirror that sits on this axis. + scale(axis, axis) = std::copysign(scaling_factor, scale(axis, axis)); const Vec3d offset = get_offset(); m_matrix = rotation * scale; diff --git a/tests/libslic3r/test_geometry.cpp b/tests/libslic3r/test_geometry.cpp index caf9bd23fc..a8a6315628 100644 --- a/tests/libslic3r/test_geometry.cpp +++ b/tests/libslic3r/test_geometry.cpp @@ -3,6 +3,10 @@ #include #include +#include +#include +#include +#include #include "libslic3r/Point.hpp" #include "libslic3r/BoundingBox.hpp" #include "libslic3r/Polygon.hpp" @@ -758,3 +762,118 @@ TEST_CASE("Convex polygon intersection test prusa polygons", "[Geometry][Rotcali REQUIRE(res == ref); } } + +namespace { +// Scale on the local axes, with a mirror on `mirror_axis`, then rotation. +Transform3d mirrored_transform(const Matrix3d &rotation, const Vec3d &scale, int mirror_axis) +{ + Vec3d signed_scale = scale; + signed_scale[mirror_axis] = -signed_scale[mirror_axis]; + Transform3d trafo = Transform3d::Identity(); + trafo.linear() = rotation * signed_scale.asDiagonal(); + return trafo; +} + +const std::vector &test_rotations() +{ + static const std::vector rotations = { + Matrix3d::Identity(), + Eigen::AngleAxisd(0.5 * PI, Vec3d::UnitZ()).toRotationMatrix(), + Eigen::AngleAxisd(0.7, Vec3d(1., 2., 3.).normalized()).toRotationMatrix(), + Eigen::AngleAxisd(2.5, Vec3d(-1., 0.5, 2.).normalized()).toRotationMatrix(), + }; + return rotations; +} +} // namespace + +TEST_CASE("Mirrored transformations decompose into a rotation and a diagonal scale", "[Geometry]") +{ + const size_t rotation_idx = GENERATE(range(size_t(0), test_rotations().size())); + const int mirror_axis = GENERATE(0, 1, 2); + const Vec3d scale = GENERATE(Vec3d(1., 1., 1.), Vec3d(2., 2., 2.), Vec3d(1., 1., 3.), Vec3d(1., 2., 3.)); + CAPTURE(rotation_idx, mirror_axis, scale.transpose()); + const Geometry::Transformation trafo(mirrored_transform(test_rotations()[rotation_idx], scale, mirror_axis)); + + const Vec3d scaling = trafo.get_scaling_factor(); + const Vec3d mirror = trafo.get_mirror(); + for (int i = 0; i < 3; ++i) { + CHECK_THAT(scaling[i], Catch::Matchers::WithinAbs(scale[i], 1e-9)); + CHECK_THAT(std::abs(mirror[i]), Catch::Matchers::WithinAbs(1., 1e-12)); + } + CHECK_THAT(mirror.prod(), Catch::Matchers::WithinAbs(-1., 1e-12)); + CHECK_FALSE(trafo.has_skew()); + const bool uniform = scale.x() == scale.y() && scale.y() == scale.z(); + CHECK(trafo.is_scaling_uniform() == uniform); + const Matrix3d rotation = trafo.get_rotation_matrix().linear(); + CHECK_THAT(rotation.determinant(), Catch::Matchers::WithinAbs(1., 1e-9)); + const Matrix3d rebuilt = rotation * trafo.get_scaling_factor_matrix().linear() * trafo.get_mirror_matrix().linear(); + CHECK(rebuilt.isApprox(trafo.get_matrix().linear(), 1e-9)); +} + +TEST_CASE("An unrotated mirror keeps its axis and needs no rotation", "[Geometry]") +{ + const int mirror_axis = GENERATE(0, 1, 2); + const Vec3d scale = GENERATE(Vec3d(1., 1., 1.), Vec3d(1., 2., 3.)); + CAPTURE(mirror_axis, scale.transpose()); + const Geometry::Transformation trafo(mirrored_transform(Matrix3d::Identity(), scale, mirror_axis)); + const Vec3d mirror = trafo.get_mirror(); + for (int i = 0; i < 3; ++i) + CHECK_THAT(mirror[i], Catch::Matchers::WithinAbs(i == mirror_axis ? -1. : 1., 1e-12)); + CHECK(trafo.get_rotation().isZero(1e-9)); +} + +TEST_CASE("A mirrored and rotated transformation at unit scale reports unit scales", "[Geometry]") +{ + // Valid mirrored instance transform as stored in a 3MF project. + Transform3d matrix = Transform3d::Identity(); + matrix.linear() << 4.4408921e-16, 0.819152044, 0.573576436, + 1.0, -4.4408921e-16, -1.11022302e-16, + -5.55111512e-17, -0.573576436, 0.819152044; + const Geometry::Transformation trafo(matrix); + CHECK(trafo.get_scaling_factor().isApprox(Vec3d::Ones(), 1e-8)); + CHECK(trafo.get_mirror().allFinite()); + CHECK(trafo.is_scaling_uniform()); +} + +TEST_CASE("Scaling one axis of a mirrored transformation keeps the mirror and adds no skew", "[Geometry]") +{ + const size_t rotation_idx = GENERATE(range(size_t(0), test_rotations().size())); + const int mirror_axis = GENERATE(0, 1, 2); + const int scaled_axis = GENERATE(0, 1, 2); + CAPTURE(rotation_idx, mirror_axis, scaled_axis); + const Matrix3d &rotation = test_rotations()[rotation_idx]; + Geometry::Transformation trafo(mirrored_transform(rotation, Vec3d::Ones(), mirror_axis)); + trafo.set_scaling_factor(Axis(scaled_axis), 2.); + Vec3d expected_scale = Vec3d::Ones(); + expected_scale[scaled_axis] = 2.; + CHECK(trafo.get_matrix().linear().isApprox(mirrored_transform(rotation, expected_scale, mirror_axis).linear(), 1e-9)); + CHECK(trafo.is_left_handed()); + CHECK_FALSE(trafo.has_skew()); +} + +TEST_CASE("Transformations without a mirror decompose as computeRotationScaling() does", "[Geometry]") +{ + Transform3d skewed = Transform3d::Identity(); + skewed.linear() << 1., 0.3, 0., + 0., 1., 0., + 0., 0., 2.; + const Transform3d matrix = GENERATE_COPY(Transform3d(test_rotations()[2]) * Geometry::scale_transform(Vec3d(1., 2., 3.)), + Transform3d(test_rotations()[3]) * skewed); + Matrix3d rotation; + Matrix3d scale; + matrix.computeRotationScaling(&rotation, &scale); + const Geometry::Transformation trafo(matrix); + CHECK(trafo.get_rotation_matrix().linear() == rotation); + CHECK(trafo.has_skew() == !scale.isDiagonal()); +} + +TEST_CASE("Skew is detected with and without a mirror", "[Geometry]") +{ + Transform3d skewed = Transform3d::Identity(); + skewed.linear() << 1., 0.3, 0., + 0., 1., 0., + 0., 0., 1.; + const bool mirrored = GENERATE(false, true); + const Transform3d matrix = Transform3d(test_rotations()[2]) * skewed * Geometry::scale_transform(Vec3d(mirrored ? -1. : 1., 1., 1.)); + CHECK(Geometry::Transformation(matrix).has_skew()); +} diff --git a/tests/libslic3r/test_model.cpp b/tests/libslic3r/test_model.cpp index f62c0c43bc..5fc72a2b9b 100644 --- a/tests/libslic3r/test_model.cpp +++ b/tests/libslic3r/test_model.cpp @@ -8,10 +8,13 @@ #include "libslic3r/Point.hpp" #include +#include +#include #include #include #include "libslic3r/Model.hpp" #include "libslic3r/Geometry.hpp" +#include "libslic3r/libslic3r.h" using namespace Slic3r; @@ -71,3 +74,26 @@ TEST_CASE("An object's raw mesh keeps the triangles of each part on its own vert CHECK_THAT(min_x.front(), Catch::Matchers::WithinAbs(0., 1e-4)); CHECK_THAT(min_x.back(), Catch::Matchers::WithinAbs(30., 1e-4)); } + +TEST_CASE("An instance added from another's scale, rotation and mirror matches it", "[Model]") +{ + // Add instance and Fill bed copy an instance this way. + const int case_idx = GENERATE(0, 1); + Transform3d trafo = Transform3d::Identity(); + if (case_idx == 0) + trafo.linear() = Eigen::AngleAxisd(0.5 * PI, Vec3d::UnitZ()).toRotationMatrix() * Vec3d(-1., 1., 1.).asDiagonal(); + else + trafo.linear() << 4.4408921e-16, 0.819152044, 0.573576436, + 1.0, -4.4408921e-16, -1.11022302e-16, + -5.55111512e-17, -0.573576436, 0.819152044; + CAPTURE(case_idx); + + Model model; + ModelObject *object = model.add_object(); + object->add_volume(make_cube(10, 20, 30)); + ModelInstance *original = object->add_instance(); + original->set_transformation(Geometry::Transformation(trafo)); + const ModelInstance *copy = object->add_instance(original->get_offset(), original->get_scaling_factor(), + original->get_rotation(), original->get_mirror()); + CHECK(copy->get_matrix().linear().isApprox(original->get_matrix().linear(), 1e-9)); +}