From 30698cf885a9c5caa4e6c1286600513d710f1ea1 Mon Sep 17 00:00:00 2001 From: Nathan Vegdahl Date: Thu, 1 May 2025 12:25:52 +0200 Subject: [PATCH] Fix #137932: compute correct armature bounds There were actually two issues here: 1. The dimension reported for armatures were often wildly incorrect, including negative values and zero! 2. The dimensions reported for objects are supposed to be invariant with rotation, representing the dimensions along the object's local axes. However, armature objects' reported dimensions changed with rotation. The respective causes were: 1. `BKE_armature_min_max()` was using an incorrect formula (acknowledged in a comment) for transforming the bounding box between spaces. This worked fine for some of the places that `BKE_armature_min_max()` was called, since they just reverse the transform using the same(!) erroneous formula, but it didn't work for others. 2. `BKE_armature_min_max()` first computed the bounds in world space, and then transformed them into object space, rather than computing them in object space directly like the respective functions for other object types. Even when done correctly, this causes the reported dimension to vary with rotation. This PR fixes these issues by simply computing the armature bounding box in object space directly instead. There is one place in the code base that was directly using the world-space bounds: `view3d_calc_minmax_selected()`. However, for every object type other than armatures, it takes the object-space bounds and transforms them (with an incorrect formula!) to world space. So this PR also changes `view3d_calc_minmax_selected()`'s armature code to do the same, except with a correct formula. Note that the reason for using the correct transform formula (departing from other object types) is that the world-space bounds for armatures were already correct prior to this PR due to being computed in that space. Therefore using the incorrect formula has the potential to introduce regressions in this case. Pull Request: https://projects.blender.org/blender/blender/pulls/137961 --- source/blender/blenkernel/BKE_armature.hh | 4 +- source/blender/blenkernel/intern/armature.cc | 22 ++------ source/blender/blenlib/BLI_bounds.hh | 40 +++++++++++++++ source/blender/blenlib/BLI_math_vector.hh | 50 +++++++++++++++++++ .../blenlib/tests/BLI_math_vector_test.cc | 39 +++++++++++++++ .../space_view3d/view3d_navigate_view_all.cc | 6 ++- 6 files changed, 139 insertions(+), 22 deletions(-) diff --git a/source/blender/blenkernel/BKE_armature.hh b/source/blender/blenkernel/BKE_armature.hh index a3e92eeccc3..89200ee924f 100644 --- a/source/blender/blenkernel/BKE_armature.hh +++ b/source/blender/blenkernel/BKE_armature.hh @@ -159,7 +159,7 @@ void BKE_armature_transform(bArmature *arm, const float mat[4][4], bool do_props std::optional> BKE_armature_min_max(const Object *ob); /** - * Calculate the axis-aligned bounds of `pchan` in world-space, + * Calculate the axis-aligned bounds of `pchan` in object-space, * taking into account custom transform when set. * * `r_min` and `r_max` are expanded to fit `pchan` so the caller must initialize them @@ -179,7 +179,7 @@ void BKE_pchan_minmax(const Object *ob, blender::float3 &r_min, blender::float3 &r_max); /** - * Calculate the axis aligned bounds of the pose of `ob` in world-space. + * Calculate the axis aligned bounds of the pose of `ob` in object-space. * * This only considers visible bones. When they are either directly (via a flag on the bone) or * indirectly (via bone collections) hidden, they are not part of the bounds calculation. When a diff --git a/source/blender/blenkernel/intern/armature.cc b/source/blender/blenkernel/intern/armature.cc index 58f9b9afb30..2f0788f59bf 100644 --- a/source/blender/blenkernel/intern/armature.cc +++ b/source/blender/blenkernel/intern/armature.cc @@ -3094,18 +3094,7 @@ void BKE_pose_where_is(Depsgraph *depsgraph, Scene *scene, Object *ob) std::optional> BKE_armature_min_max(const Object *ob) { - std::optional> bounds_world = BKE_pose_minmax(ob, false); - - if (!bounds_world) { - return std::nullopt; - } - - /* NOTE: this is not correct (after rotation the AABB may not be the smallest enclosing AABB any - * more), but acceptable because this is called via BKE_object_boundbox_get(), which is called by - * BKE_object_minmax(), which does the opposite transform. */ - return blender::Bounds{ - math::transform_point(ob->world_to_object(), bounds_world->min), - math::transform_point(ob->world_to_object(), bounds_world->max)}; + return BKE_pose_minmax(ob, false); } void BKE_pchan_minmax(const Object *ob, @@ -3142,17 +3131,14 @@ void BKE_pchan_minmax(const Object *ob, pchan->custom_translation[0], pchan->custom_translation[1], pchan->custom_translation[2]); - mul_m4_series(mat.ptr(), ob->object_to_world().ptr(), tmp.ptr(), rmat.ptr(), smat.ptr()); + mul_m4_series(mat.ptr(), tmp.ptr(), rmat.ptr(), smat.ptr()); BoundBox bb; BKE_boundbox_init_from_minmax(&bb, bb_custom->min, bb_custom->max); BKE_boundbox_minmax(bb, mat, r_min, r_max); } else { - float vec[3]; - mul_v3_m4v3(vec, ob->object_to_world().ptr(), pchan_tx->pose_head); - minmax_v3v3_v3(r_min, r_max, vec); - mul_v3_m4v3(vec, ob->object_to_world().ptr(), pchan_tx->pose_tail); - minmax_v3v3_v3(r_min, r_max, vec); + minmax_v3v3_v3(r_min, r_max, pchan_tx->pose_head); + minmax_v3v3_v3(r_min, r_max, pchan_tx->pose_tail); } } diff --git a/source/blender/blenlib/BLI_bounds.hh b/source/blender/blenlib/BLI_bounds.hh index 11f43f42501..a79fdc30d1b 100644 --- a/source/blender/blenlib/BLI_bounds.hh +++ b/source/blender/blenlib/BLI_bounds.hh @@ -14,6 +14,7 @@ #include "BLI_bounds_types.hh" #include "BLI_index_mask.hh" +#include "BLI_math_matrix.hh" #include "BLI_math_vector.hh" #include "BLI_task.hh" #include "BLI_virtual_array.hh" @@ -183,6 +184,45 @@ template inline std::optional max(const VArray &values) [](const int a, const int b) { return std::max(a, b); }); } +/** + * Return the eight corners of a 3D bounding box. + */ +template +inline std::array, 8> corners(const Bounds> &bounds) +{ + return { + VecBase{bounds.min[0], bounds.min[1], bounds.min[2]}, + VecBase{bounds.min[0], bounds.min[1], bounds.max[2]}, + VecBase{bounds.min[0], bounds.max[1], bounds.min[2]}, + VecBase{bounds.min[0], bounds.max[1], bounds.max[2]}, + VecBase{bounds.max[0], bounds.min[1], bounds.min[2]}, + VecBase{bounds.max[0], bounds.min[1], bounds.max[2]}, + VecBase{bounds.max[0], bounds.max[1], bounds.min[2]}, + VecBase{bounds.max[0], bounds.max[1], bounds.max[2]}, + }; +} + +/** + * Transform a 3D bounding box. + * + * Note: this necessarily grows the bounding box, to ensure that the transformed + * bounding box fully contains the original. Therefore, calling this iteratively + * to transform from space A to space B, and then from space B to space C, etc., + * will also iteratively grow the bounding box on each call. Try to avoid doing + * that, and instead first compose the transform matrices and then use that to + * transform the bounding box. + */ +template +inline Bounds> transform_bounds(const MatBase &matrix, + const Bounds> &bounds) +{ + std::array, 8> points = corners(bounds); + for (VecBase &p : points) { + p = math::transform_point(matrix, p); + } + return {math::min(Span(points)), math::max(Span(points))}; +} + } // namespace bounds namespace detail { diff --git a/source/blender/blenlib/BLI_math_vector.hh b/source/blender/blenlib/BLI_math_vector.hh index c7d9d8f2dc4..b95e4c3cb7b 100644 --- a/source/blender/blenlib/BLI_math_vector.hh +++ b/source/blender/blenlib/BLI_math_vector.hh @@ -59,12 +59,62 @@ template BLI_UNROLL_MATH_VEC_FUNC_VEC_VEC(math::min, a, b); } +/** + * Element-wise minimum of the passed vectors. + */ +template +[[nodiscard]] inline VecBase min(Span> values) +{ + BLI_assert(!values.is_empty()); + + VecBase result = values[0]; + for (const VecBase &v : values.drop_front(1)) { + result = min(result, v); + } + + return result; +} + +/** + * Element-wise minimum of the passed vectors. + */ +template +[[nodiscard]] inline VecBase min(std::initializer_list> values) +{ + return min(Span(values)); +} + template [[nodiscard]] inline VecBase max(const VecBase &a, const VecBase &b) { BLI_UNROLL_MATH_VEC_FUNC_VEC_VEC(math::max, a, b); } +/** + * Element-wise maximum of the passed vectors. + */ +template +[[nodiscard]] inline VecBase max(Span> values) +{ + BLI_assert(!values.is_empty()); + + VecBase result = values[0]; + for (const VecBase &v : values.drop_front(1)) { + result = max(result, v); + } + + return result; +} + +/** + * Element-wise maximum of the passed vectors. + */ +template +[[nodiscard]] inline VecBase max(std::initializer_list> values) +{ + return max(Span(values)); +} + template [[nodiscard]] inline VecBase clamp(const VecBase &a, const VecBase &min, diff --git a/source/blender/blenlib/tests/BLI_math_vector_test.cc b/source/blender/blenlib/tests/BLI_math_vector_test.cc index d3a6a7e12db..fff76d8d20e 100644 --- a/source/blender/blenlib/tests/BLI_math_vector_test.cc +++ b/source/blender/blenlib/tests/BLI_math_vector_test.cc @@ -6,6 +6,7 @@ #include "BLI_math_vector.h" #include "BLI_math_vector.hh" +#include "BLI_vector.hh" namespace blender::tests { @@ -65,6 +66,44 @@ TEST(math_vector, Clamp) EXPECT_EQ(result_2.z, -50); } +TEST(math_vector, MinList) +{ + EXPECT_EQ(float3(1.0, 2.0, 3.0), math::min({float3(1.0, 2.0, 3.0)})); + EXPECT_EQ(float3(0.0, 2.0, 2.0), math::min({float3(1.0, 2.0, 3.0), float3(0.0, 5.0, 2.0)})); + EXPECT_EQ(float3(0.0, 2.0, 1.5), + math::min({float3(1.0, 2.0, 3.0), float3(0.0, 5.0, 2.0), float3(2.0, 4.0, 1.5)})); + + const float inf = std::numeric_limits::infinity(); + EXPECT_EQ(float3(0.0, -inf, -inf), + math::min({float3(inf, 2.0, 3.0), float3(0.0, -inf, inf), float3(2.0, 4.0, -inf)})); + + const float nan = std::numeric_limits::quiet_NaN(); + const float3 result = math::min( + {float3(nan, 2.0, 3.0), float3(0.0, nan, 2.0), float3(2.0, 4.0, nan)}); + EXPECT_TRUE(std::isnan(result.x)); + EXPECT_EQ(result.y, 2.0); + EXPECT_EQ(result.z, 2.0); +} + +TEST(math_vector, MaxList) +{ + EXPECT_EQ(float3(1.0, 2.0, 3.0), math::max({float3(1.0, 2.0, 3.0)})); + EXPECT_EQ(float3(1.0, 5.0, 3.0), math::max({float3(1.0, 2.0, 3.0), float3(0.0, 5.0, 2.0)})); + EXPECT_EQ(float3(2.0, 5.0, 3.0), + math::max({float3(1.0, 2.0, 3.0), float3(0.0, 5.0, 2.0), float3(2.0, 4.0, 1.5)})); + + const float inf = std::numeric_limits::infinity(); + EXPECT_EQ(float3(inf, 4.0, inf), + math::max({float3(inf, 2.0, 3.0), float3(0.0, -inf, inf), float3(2.0, 4.0, -inf)})); + + const float nan = std::numeric_limits::quiet_NaN(); + const float3 result = math::max( + {float3(nan, 2.0, 3.0), float3(0.0, nan, 2.0), float3(2.0, 4.0, nan)}); + EXPECT_TRUE(std::isnan(result.x)); + EXPECT_EQ(result.y, 4.0); + EXPECT_EQ(result.z, 3.0); +} + TEST(math_vector, InterpolateInt) { const int3 a(0, -100, 50); diff --git a/source/blender/editors/space_view3d/view3d_navigate_view_all.cc b/source/blender/editors/space_view3d/view3d_navigate_view_all.cc index de76de6ea70..015f0f4e572 100644 --- a/source/blender/editors/space_view3d/view3d_navigate_view_all.cc +++ b/source/blender/editors/space_view3d/view3d_navigate_view_all.cc @@ -317,8 +317,10 @@ std::optional> view3d_calc_minmax_selected(Depsgraph *de { const std::optional> bounds = BKE_pose_minmax(ob_eval_iter, true); if (bounds) { - minmax_v3v3_v3(min, max, bounds->min); - minmax_v3v3_v3(min, max, bounds->max); + const blender::Bounds world_bounds = blender::bounds::transform_bounds( + ob_eval->object_to_world(), *bounds); + minmax_v3v3_v3(min, max, world_bounds.min); + minmax_v3v3_v3(min, max, world_bounds.max); changed = true; } }