Fix VMobject dimensions and critical points using Bézier extrema - #4943
Fix VMobject dimensions and critical points using Bézier extrema#4943kyoai-zhao wants to merge 16 commits into
Conversation
- Add VMobject.get_bezier_bounding_box, computing the exact axis-aligned bounding box of the quadratic/cubic bezier curves by finding interior extrema analytically (roots of the derivative). - Override reduce_across_dimension so width/height reflect the rendered curve extent, not the control-point extent. Closes ManimCommunity#3619
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Replace the reduce_across_dimension override (which changed critical-point semantics and shifted rendering) with a length_over_dim override, so only width/height/depth become exact while centering and edges keep their control-point behavior.
4af8809 to
d497260
Compare
Width/height (and depth) now reflect the exact extent of the rendered Bézier curves, including interior extrema, instead of the bounding box of their control points. Critical points (get_left, get_center, ...) keep their control-point semantics, so only dimensional reads change. SVGMobject imports are scaled to their target height using the exact curve extent; the control frames of the five affected SVG tests are updated accordingly.
86a476e to
ceb944e
Compare
|
|
||
| Returns an array of shape ``(n_curves, 2)``. A single point yields a | ||
| degenerate curve whose extent in every dimension is that point. | ||
| """ |
There was a problem hiding this comment.
Fixed in 8098b63: the unused anchors assignment was removed, and the redundant t = np.zeros(n_curves) initialization was dropped (the subsequent np.where always overwrites it).
nikolajmunk
left a comment
nikolajmunk
left a comment
There was a problem hiding this comment.
This is a welcome change! #3625 never really got off the ground so it's nice to see it picked up again.
I haven't studied the math in detail (I assume it's roughly the same as this with some extra numpy steps), but I'll trust that it's correct! Some of your tests seem unnecessary or straightforwardly wrong, in particular the one for your "180° arc underestimation case". I also note that you aren't checking correctness for depth.
I think it's worth having a conversation about the intended behavior of get_critical_point. IMO the purpose of get_critical_point is precisely to return a point on the actual AABB of the mobject rather than use the control points, even if this is a breaking change for some users (though I can't imagine it's very many!). It seems weird to deliberately introduce a difference between, say mob.width and mob.get_right()[0] - mob.get_left()[0] for the sake of backwards compatibility.
Am I correct in suspecting that this code and PR are heavily LLM-generated and that you perhaps aren't that familiar with the Manim library? That's not disqualifying at all, but I think this change would benefit a lot from having a human take a second pass over this code.
| def test_width_height_setters_round_trip_on_rotated_circle(): | ||
| c = Circle(radius=3).rotate(30 * DEGREES) | ||
| c.width = 5.0 | ||
| assert c.width == pytest.approx(5.0) | ||
| assert c.height == pytest.approx(5.0) |
There was a problem hiding this comment.
I would turn this into a general test that confirms that setting width/height/depth to val results in a mobject of size val in that dimension. Maybe parametrize the test to try out both single dimensions as well as combinations.
It might also be nice to explicitly construct a VMobject for this, just in case Circle changes the ways its points are drawn down the line (since we're not testing the implementation of Circle).
| def test_arc_height_is_not_underestimated(): | ||
| # A 180-degree arc with near-vertical handles: the raw control points | ||
| # span only ~1x the radius in height, while the rendered arc spans 2x. | ||
| arc = Arc(radius=2, angle=PI) | ||
| assert arc.height == pytest.approx(2.0) | ||
| assert arc.width == pytest.approx(4.0) |
There was a problem hiding this comment.
Am I misunderstanding your point/comment here?
ThisArc consists of 8 subcurves, and the middle anchor point is already located at (0, 2, 0) so the width and height are already correctly computed.
In fact, a 180° arc should span only 1x its radius in height. Here's a render from the current main branch:
| def test_quadratic_width_height_use_curve_extrema(): | ||
| # Quadratic counterpart: interior extrema must be included too. | ||
| vmob = VMobject().set_points( | ||
| np.array([[0.0, 0.0, 0.0], [2.0, 4.0, 0.0], [4.0, 0.0, 0.0]]) | ||
| ) | ||
| assert vmob.width == pytest.approx(4.0) | ||
| assert vmob.height == pytest.approx(4.0) |
There was a problem hiding this comment.
This is probably a good test, but it feels weird to test that width/height computation is correct on an invalid VMobject? Standard VMobjects always use cubic beziers so I don't think a three-point VMobject would be rendered. There might be some utility for this I'm unaware of, happy to be wrong.
There was a problem hiding this comment.
I see no tests for depth, is this intentional?
| """ | ||
| n_curves = len(pts) // nppcc | ||
| if nppcc == 3: | ||
| # Quadratic Bézier: P'(t) = 2*((p1-p0) + t*(p0-2p1+p2)). |
There was a problem hiding this comment.
I assume this is in preparation for adding the same feature to OpenGLVMobject since AFAIK VMobject always has npcc == 4. Could maybe be left out for now but I guess it doesn't hurt anything.
…animCommunity#3619) get_left/get_right/get_top/get_bottom/get_center (and get_critical_point generally) now use the exact bounding box of the rendered Bezier curves instead of the raw control points, so they always agree with width, height and depth. Previously the control-point semantics could inflate the box (the curve lies inside its control hull by de Casteljau's convex-hull property), and the 180-degree arc test could not distinguish the semantics because its subdivided anchors happen to reach the extrema. Replace that test with a 70% arc whose interior extrema are not anchored, and add a depth (z) coverage test. As a consequence this is a behavior change for users who align mobjects by their critical points: alignment now tracks the rendered shape.
…tic case Address reviewer feedback (nikolajmunk): setters are now verified against an explicitly constructed VMobject (not Circle) and parametrized over each dimension singly; the quadratic case is kept and reframed as a direct test of the quadratic curve-data path (SVG Q/T commands are quadratic). A note documents why combined setter sequences are not meaningful round-trips (uniform scaling rescales dimensions set earlier).
…l frames Reviewer and author check: default VMobjects are cubic-only - the SVG path code degree-elevates Q/T segments to cubics (add_quad -> add_cubic), so a three-point VMobject never occurs on the default path and the quadratic test only exercised the degenerate fallback. Remove the nppcc=3 branch of the extrema computation and its test. Comment on the 70% arc test now distinguishes all three boxes: control points span 3.648879 in x, the exact curve 3.618034, anchors only 3.562774. Regenerate graphical control frames rendered under the unified critical-point semantics.
for more information, see https://pre-commit.ci
|
Thanks for the review — this was really helpful. On the arc test, you're right, and my comment there was wrong too. There were actually two different old bounds involved: width/height used all control points, whose AABB can only overestimate the Bézier extent because of the convex-hull property; I replaced it with Depth is covered now as well, using a 3D cubic whose z handles span On The setter tests are now parametrized over width/height/depth on a hand-built VMobject. I also dropped the quadratic test. I realized that the default Cairo SVG path code degree-elevates Q/T segments to cubics anyway, so the previous three-point And yes — much of the first draft of this PR was written with LLM assistance. Since your review I went through the code myself, re-derived the extrema calculations, checked the root/degenerate branches, and ran the full suite: tests/module 523 passed; the remaining 32 failures are the pre-existing Typst environment failures; graphical unit and scene-rendering suites pass apart from the two pre-existing ffmpeg vp9 codec environment failures. I'm happy to keep iterating on anything that still looks off. |
…cstrings per review
kyoai-zhao
changed the title
…act Bézier bounds; scale-relative tolerance; fold family extrema into one kernel - Override VMobject.get_extremum_along_dim so get_x/get_coord/set_x/ match_x/align_to use the same exact curve bounds as get_critical_point and the width/height/depth setters; explicit-point calls keep the base class semantics. set_x(0) and align_to now move to the rendered extent. - Add _get_bezier_family_bounding_box helper (single kernel over merged family points; per-member fallback for non-cubic subclasses) and use it from length_over_dim and get_critical_point, removing duplicated loops. - Make extrema tolerances relative to the coefficient scale so that uniformly scaling a curve does not change root classification (e.g. width now exact at scale 1e-14, setter round-trips hold). - Tests: coord planning consistency (get_x/set_x/align_to), scale invariance across 1e-16..1e9.
for more information, see https://pre-commit.ci
nikolajmunk
left a comment
nikolajmunk
left a comment
There was a problem hiding this comment.
Okay, things look much improved! :) I'm still hoping someone with a better handle on the math can let us know if the methods themselves check out, but with a good enough test array that should be a minor task.
I've left some comments on your tests. Overall they don't seem very cohesive, and depth still feels like an afterthought. It might be useful to think one more time about which things must be true when the methods are working as intended, and then make sure that this holds for all dimensions of the mobject.
| vmob = VMobject().set_points( | ||
| np.array([[0.0, 0.0, 0.0], [5.0, 3.0, 0.0], [-5.0, 3.0, 0.0], [1.0, 0.0, 0.0]]) | ||
| ) |
There was a problem hiding this comment.
To properly test that this works, I would like to see a wild cubic that has three dimensions (currently the z dimension is 0). That way we know that the formula correctly calculates a 3D bounding box.
| def _wild_cubic() -> VMobject: | ||
| return VMobject().set_points( | ||
| np.array([[0.0, 0.0, 0.0], [5.0, 3.0, 0.0], [-5.0, 3.0, 0.0], [1.0, 0.0, 0.0]]) | ||
| ) |
There was a problem hiding this comment.
Same as above, would be nice to see this in 3D.
| def test_depth_and_critical_points_cover_curve_extrema(): | ||
| # Control points along z span [-5, 5], but the rendered curve only | ||
| # reaches ~[-0.985, 1.453]. depth and the OUT/IN critical points must | ||
| # agree with each other and stay within the control hull. | ||
| vmob = VMobject().set_points( | ||
| np.array([[0.0, 0.0, 0.0], [0.0, 0.0, 5.0], [0.0, 0.0, -5.0], [0.0, 0.0, 1.0]]) | ||
| ) | ||
| assert vmob.depth == pytest.approx(2.437719, abs=1e-4) | ||
| out = vmob.get_critical_point(np.array([0.0, 0.0, 1.0])) | ||
| inn = vmob.get_critical_point(np.array([0.0, 0.0, -1.0])) | ||
| assert out[2] == pytest.approx(1.45299067, abs=1e-6) | ||
| assert inn[2] == pytest.approx(-0.98472845, abs=1e-6) | ||
| assert out[2] - inn[2] == pytest.approx(vmob.depth, abs=1e-6) |
There was a problem hiding this comment.
Instead of having a separate test for depth, make sure that each test works for all three dimensions.
| assert arc.width == pytest.approx(3.618034, abs=1e-4) | ||
| assert arc.height == pytest.approx(4.0, abs=1e-3) | ||
| assert arc.get_right()[0] - arc.get_left()[0] == pytest.approx(arc.width, abs=1e-6) | ||
| assert arc.get_top()[1] - arc.get_bottom()[1] == pytest.approx(arc.height, abs=1e-6) |
There was a problem hiding this comment.
The arc example isn't that convincing to me. If the purpose is to test "curve with control points forming a larger box than the curve itself" I would suggest something like this:
start = ORIGIN
planes = [
(Y_AXIS + Z_AXIS, X_AXIS),
(X_AXIS + Z_AXIS, Y_AXIS),
(X_AXIS + Y_AXIS, Z_AXIS),
]
for i, (end, handle_direction) in enumerate(planes):
control_points = [start, start + handle_direction, end + handle_direction, end]
curve = VMobject().set_points(control_points)
expected_size = [1.0, 1.0, 1.0]
expected_size[i] = 0.75
computed_size = [curve.width, curve.height, curve.depth]
np.testing.assert_allclose(computed_size, expected_size)This could also easily be parametrized instead, then you could just do expected_size = np.array([1.0, 1.0, 1.0]) - direction * 0.25.
| def test_width_height_of_single_point_vmobject(): | ||
| vmob = VMobject().set_points(np.array([[3.0, 4.0, 0.0]])) | ||
| assert vmob.width == pytest.approx(0.0) | ||
| assert vmob.height == pytest.approx(0.0) |
There was a problem hiding this comment.
Also needs depth. It's also unclear if this will ever be relevant since VMobject expects at least 4 points, but I guess it doesn't hurt?
| def test_family_fast_path_aggregates_exact_bounds(): | ||
| left = _wild_cubic().shift(20 * LEFT) | ||
| right = _wild_cubic().shift(20 * RIGHT) | ||
| group = VGroup(left, right) | ||
| assert group.get_left()[0] == pytest.approx(-20.98472845, abs=1e-6) | ||
| assert group.get_right()[0] == pytest.approx(21.45299067, abs=1e-6) | ||
| assert group.width == pytest.approx(42.4377191, abs=1e-4) |
There was a problem hiding this comment.
A few more tests like this for multi-mobject families would be nice. Perhaps some of the above tests can be parametrized to test:
- Single-curve mobject
- Multi-curve mobject (both contiguous and otherwise)
- Multi-mobject group (including a case where the parent also has a curve)
nikolajmunk
left a comment
nikolajmunk
left a comment
There was a problem hiding this comment.
Added a few more comments about docstrings; I suspect your AI agent wrote them and did that annoying thing they tend to do where they write comments and docstrings explaining all the work they did instead of explaining the contents of the code 😅
| bounding box of the full family is used, so planning methods such | ||
| as ``get_coord``, ``set_coord`` and ``align_to`` are consistent | ||
| with :meth:`get_critical_point` and the width/height/depth setters. |
There was a problem hiding this comment.
This last bit sounds LLM-y, the user doesn't need to know that the method is consistent with the others (since that's the default assumption)
| Like :meth:`~.Mobject.length_over_dim`, this covers every point in | ||
| this :class:`VMobject` and its submobjects, and the length is | ||
| computed from the actual Bézier curves (including their interior | ||
| extrema) rather than from the raw control points. Dimensions such | ||
| as :attr:`~Mobject.width` and :attr:`~Mobject.height` therefore | ||
| correspond to the physical extent of the rendered shape, and the | ||
| critical points (:meth:`~Mobject.get_left` etc.) agree with them. |
There was a problem hiding this comment.
This seems like an LLM-ism; I would personally write a docstring more similar to the one in Mobject.
| Unlike :meth:`~.Mobject.get_critical_point`, the bounding box is the | ||
| exact box of the rendered Bézier curves (see | ||
| :meth:`get_bezier_bounding_box`), not the box of their control points. | ||
| ``get_left()``, ``get_right()``, ``get_top()``, ``get_bottom()``, | ||
| ``get_center()`` etc. therefore always agree with ``width``, ``height`` | ||
| and ``depth``. |
There was a problem hiding this comment.
This doesn't really make sense; Mobjects don't have control points at all, they just have points. The user already expects all of these things to be the case, so we don't need to tell them.
3 participants
Motivation
VMobject.width/height/depthare computed from the raw control points of the Bézier curves. Control points (handles) generally do not lie on the rendered curve, so dimensions can be wrong even for simple shapes:Circle(radius=3).rotate(30 * DEGREES)reports width == 6.2074 instead of 6.0 (the diameter).Change
VMobject.get_bezier_bounding_box(): exact axis-aligned bounding box of the cubic Bézier curves, found analytically (derivative roots for interior extrema), with a control-point fallback for unsupported or incomplete point layouts. Root tolerances are relative to the coefficient scale, so uniformly scaling a curve does not change which roots count as interior extrema.VMobject._get_bezier_family_bounding_box(): family-level box computed in a single kernel over the merged points of all submobjects (with a per-member fallback for non-cubic subclasses).VMobject.length_over_dim()(backs width/height/depth) so these reflect the physical extent of the rendered curves, recursively through submobjects (VGroup, Text, ...).VMobject.get_critical_point()to use the same exact curve box, soget_right()[0] - get_left()[0] == widthholds as an invariant (likewise height/depth). Only VMobjects whose curve extrema fall between anchors change behavior; polygonal/anchor-extrema shapes are unchanged.VMobject.get_extremum_along_dim()so the planning API (get_x,get_coord,set_x,match_x,align_to) is consistent with the exact critical points: explicit-point calls keep the base-class semantics.Control frames updated
SVGMobject is scaled to its target height using the exact extent, and the unified critical-point semantics shifts a few scenes' layout slightly. Control frames for QuadraticPath, SmoothCurves, HalfEllipse, Heart, WeightSVG, three_points_Angle were regenerated with
pytest --set_test.Tests
get_x()==get_center()[0],get_x(RIGHT)==get_right()[0],set_x(0)centers the rendered extent,align_to(..., RIGHT)aligns rendered right edgesscale(1e-14)Verified: analytic extrema agree with dense sampling across Circle, Arc, Line, Triangle, Star, Dot, Polygon, cubic curves, ParametricFunction, Text, VGroup. tests/module (523 passed) and graphical unit suites show no regression vs. main beyond pre-existing typst/ffmpeg environment failures. Microbenchmarks vs. main:
Text(...).width+0.3 ms (+17%),Text(...).get_center()is ~30% faster,VGroupwith 20 submobjects ~0.15 ms.Note: the OpenGL backend (OpenGLMobject bounding boxes) still uses raw control points; separate class hierarchy, left for a follow-up.
Closes #3619