fix(constitution): use per-joint anchor positions in spherical joint simplified API#410
Conversation
…simplified API Change the simplified `apply_to` overload to accept `span<Vector3> r_local_pos` instead of a single `Vector3`, allowing each joint to specify its own anchor point in body1's local frame. Update the pybind binding and test case accordingly. Made-with: Cursor
There was a problem hiding this comment.
Code Review
This pull request updates the AffineBodySphericalJoint::apply_to API to support per-joint anchor positions by adding a new r_local_pos parameter. These changes are propagated through the C++ implementation, test cases, and Python bindings. The review feedback suggests renaming several parameters (such as l_instance_id and strength_ratio) to their plural forms in both the C++ and Python signatures to ensure naming consistency across the codebase.
| void AffineBodySphericalJoint::apply_to(geometry::SimplicialComplex& sc, | ||
| span<S<geometry::SimplicialComplexSlot>> l_geo_slots, | ||
| span<IndexT> l_instance_id, | ||
| span<IndexT> l_instance_id, |
There was a problem hiding this comment.
For consistency with the simplified apply_to overload and general C++ best practices, it's recommended to use plural names for parameters that are collections (like span).
Consider renaming the following parameters in this function signature (and its declaration in the header file) to reflect that they are collections:
l_instance_id->l_instance_idsr_instance_id->r_instance_idsstrength_ratio->strength_ratios
This will improve readability and consistency across the API.
| span<IndexT> l_instance_id, | |
| span<IndexT> l_instance_ids, |
| py::array_t<IndexT> l_instance_id, | ||
| py::list r_geo_slots, | ||
| py::array_t<IndexT> r_instance_id, | ||
| py::array_t<Float> r_local_pos, | ||
| py::array_t<Float> strength_ratio) |
There was a problem hiding this comment.
For consistency and to improve readability, it's better to use plural names for parameters that are collections, like py::array_t. This makes the Python API more intuitive.
Please consider renaming l_instance_id, r_instance_id, and strength_ratio to their plural forms. Remember to also update their usage inside the lambda body.
| py::array_t<IndexT> l_instance_id, | |
| py::list r_geo_slots, | |
| py::array_t<IndexT> r_instance_id, | |
| py::array_t<Float> r_local_pos, | |
| py::array_t<Float> strength_ratio) | |
| py::array_t<IndexT> l_instance_ids, | |
| py::list r_geo_slots, | |
| py::array_t<IndexT> r_instance_ids, | |
| py::array_t<Float> r_local_pos, | |
| py::array_t<Float> strength_ratios) |
Rename l_instance_id → l_instance_ids, r_instance_id → r_instance_ids, strength_ratio → strength_ratios in the full apply_to overload across header, implementation, and pybind binding for consistency. Made-with: Cursor
Summary
apply_tooverload wherevector<Vector3> r_local_posredefined the parameterVector3 r_local_pos, causing a compilation error.span<Vector3> r_local_pos(per-joint anchor positions) instead of a singleVector3, giving callers more flexibility while keeping the uniformstrength_ratioscalar.py::array_t<Float>) converted viaas_span_of<Vector3>.80_abd_spherical_joint) to use the new simplified API.Changed Files
include/uipc/constitution/affine_body_spherical_joint.hspan<Vector3> r_local_posparam to simplified overloadsrc/constitution/affine_body_spherical_joint.cppspan<Vector3>src/pybind/pyuipc/constitution/affine_body_spherical_joint.cpppy::array_t<Float>→span<Vector3>apps/tests/sim_case/80_abd_spherical_joint.cpp