Move from string-based links and joints to id-based logic (using hashes) - #554
Move from string-based links and joints to id-based logic (using hashes)#554rjoomen wants to merge 60 commits into
Conversation
8e6a82e to
fa0f444
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR migrates trajectory-optimization and collision code from string-based link/joint identifiers to LinkId/JointId-based identifiers (hashed IDs), and updates collision evaluation/config to match.
Changes:
- Replace string link/frame usage with
tesseract::common::LinkId/JointIdacross trajopt and trajopt_ifopt code and tests. - Update collision coefficient utilities/serialization to use
LinkIdPairkeys and add deterministic ordering for continuous-collision constraint rows. - Switch contact manager plugin defaults to Coal managers and add new octomap regression tests/configs.
Reviewed changes
Copilot reviewed 54 out of 54 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| trajopt_common/src/collision_types.cpp | Reworks CollisionCoeffData storage/overloads around LinkIdPair and updates equality logic |
| trajopt_common/include/trajopt_common/yaml_extensions.h | Updates YAML emission for pair coefficient data using LinkId names |
| trajopt/src/file_write_callback.cpp | Updates FK output map key type to LinkId |
| trajopt_ifopt/src/constraints/collision/continuous_collision_constraint.cpp | Adds deterministic iteration order via sorted contact-pair keys |
| trajopt_common/data/contact_manager_plugins.yaml | Changes default collision managers to Coal and adds Coal plugin entries |
| trajopt/test/cast_cost_octomap_arm_unit.cpp / trajopt/test/cast_constraint_octomap_unit.cpp | Adds new octomap-focused backend comparison/constraint tests |
| trajopt_optimizers/trajopt_sqp/test/planning_unit.cpp | Adjusts OSQP settings (adaptive rho + higher max iterations) |
Comments suppressed due to low confidence (2)
trajopt_ifopt/src/constraints/collision/continuous_collision_constraint.cpp:1
- Sorting
ContactResultMap::KeyTyperelies onoperator<being well-defined for that type. With the move to ID-based keys, this may no longer be true (or may sort by an implementation detail). To keep determinism robust and compilation-independent, provide an explicit comparator (e.g., compare(first.value(), second.value())or(first.name(), second.name())) matching the intended stable ordering.
trajopt_optimizers/trajopt_sqp/test/planning_unit.cpp:1 - Increasing OSQP max iterations 4× (and enabling adaptive rho) can significantly increase test runtime and mask convergence/regression issues (tests may “eventually” converge instead of exposing a configuration or modeling problem). If this is needed only for a specific scenario, consider tightening the problem setup/tolerances or making these settings localized/conditional so the broader test suite doesn’t become slower.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - tesseract_collision_coal_coal_factories | ||
| discrete_plugins: | ||
| default: BulletDiscreteBVHManager | ||
| default: CoalDiscreteBVHManager |
There was a problem hiding this comment.
Changing the default discrete/continuous managers from Bullet to Coal is a behavior change that can break existing deployments where Coal plugins aren’t present/loaded, and it changes collision results/gradients by default. Consider keeping Bullet as the default and adding Coal as an opt-in (or selecting Coal only when the plugin is available). Also verify the search_libraries entry name (tesseract_collision_coal_coal_factories) matches the actual built plugin library; a mismatch will prevent plugin discovery at runtime.
| gjk_guess_threshold: 0.005 | ||
| continuous_plugins: | ||
| default: BulletCastBVHManager | ||
| default: CoalCastBVHManager |
There was a problem hiding this comment.
Changing the default discrete/continuous managers from Bullet to Coal is a behavior change that can break existing deployments where Coal plugins aren’t present/loaded, and it changes collision results/gradients by default. Consider keeping Bullet as the default and adding Coal as an opt-in (or selecting Coal only when the plugin is available). Also verify the search_libraries entry name (tesseract_collision_coal_coal_factories) matches the actual built plugin library; a mismatch will prevent plugin discovery at runtime.
…Bullet optimization test - yaml_extensions.h: construct pair_coeff_data_node as Map rather than Sequence. It was written to via operator[] with a non-integer key (map-style), and yaml-cpp silently coerced the type on first assignment. The declared type now matches the actual usage. - cast_cost_octomap_arm_unit.cpp: replace the broken optimization_discrete_verify test with CastOctomapOptimization. bullet_vs_coal. The old test ran the PR2 arm through a deep penetration into a 0.4m octree, producing dozens of simultaneous contact normals that OSQP could not resolve; both backends hit OPT_FAILED and the comparative assertions passed trivially while the optimizer did zero work (~100s of CI for no real coverage). The new test uses the proven boxbot + octree scene from cast_cost_octomap_unit, runs the full SQP loop explicitly with each backend, and asserts both reach OPT_CONVERGED with a collision-free final trajectory and costs matching within 1%. Runtime ~1s. Full trajopt test suite drops from ~140s to ~42s. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…Bullet optimization test - yaml_extensions.h: construct pair_coeff_data_node as Map rather than Sequence. It was written to via operator[] with a non-integer key (map-style), and yaml-cpp silently coerced the type on first assignment. The declared type now matches the actual usage. - cast_cost_octomap_arm_unit.cpp: replace the broken optimization_discrete_verify test with CastOctomapOptimization. bullet_vs_coal. The old test ran the PR2 arm through a deep penetration into a 0.4m octree, producing dozens of simultaneous contact normals that OSQP could not resolve; both backends hit OPT_FAILED and the comparative assertions passed trivially while the optimizer did zero work (~100s of CI for no real coverage). The new test uses the proven boxbot + octree scene from cast_cost_octomap_unit, runs the full SQP loop explicitly with each backend, and asserts both reach OPT_CONVERGED with a collision-free final trajectory and costs matching within 1%. Runtime ~1s. Full trajopt test suite drops from ~140s to ~42s. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
23bf9d8 to
7f415c3
Compare
…Bullet optimization test - yaml_extensions.h: construct pair_coeff_data_node as Map rather than Sequence. It was written to via operator[] with a non-integer key (map-style), and yaml-cpp silently coerced the type on first assignment. The declared type now matches the actual usage. - cast_cost_octomap_arm_unit.cpp: replace the broken optimization_discrete_verify test with CastOctomapOptimization. bullet_vs_coal. The old test ran the PR2 arm through a deep penetration into a 0.4m octree, producing dozens of simultaneous contact normals that OSQP could not resolve; both backends hit OPT_FAILED and the comparative assertions passed trivially while the optimizer did zero work (~100s of CI for no real coverage). The new test uses the proven boxbot + octree scene from cast_cost_octomap_unit, runs the full SQP loop explicitly with each backend, and asserts both reach OPT_CONVERGED with a collision-free final trajectory and costs matching within 1%. Runtime ~1s. Full trajopt test suite drops from ~140s to ~42s. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7f415c3 to
d31c461
Compare
Replace string-keyed lookup_table_ and zero_coeff_ with LinkIdPair-keyed containers (CoeffEntry value struct stores names for serialization/display). Add three-tier query overloads (LinkId hot-path, string convenience) and hasZeroCoeff() method that callers now use instead of raw set access. Cereal serialization split into save/load with backwards-compatible string format. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Test LinkId/string query consistency, hasZeroCoeff, entry name preservation, and LinkIdPair-keyed zero_coeff set. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ContactResultMap keys changed from pair<string,string> to LinkIdPair. Update GradientResultsSet::key type to match. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace string-based getActiveLinkNames/getJointNames calls with their integer ID counterparts (getActiveLinkIds/getJointIds) in all collision evaluator constructors and methods. Use the existing manip_active_link_names_ member for addInterpolatedCollisionResults instead of re-allocating a name vector. Replace linear search in CartVelTermInfo::fromJson with O(1) isActiveLinkName lookup. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Switch getActiveLinkNames/getJointNames/LinkId::fromName to getActiveLinkIds/getJointIds and pass manip_active_link_names_ to addInterpolatedCollisionResults in all three evaluator classes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
getCollisionCoeff() and hasZeroCoeff() now accept a LinkIdPair directly. The (LinkId, LinkId) overloads delegate to the new implementation.
…traints Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add overloads accepting tesseract::common::LinkId directly to DynamicCartPoseErrCalculator, DynamicCartPoseJacCalculator, CartPoseErrCalculator, and CartPoseJacCalculator. The LinkId overloads become the primary constructors; existing string constructors delegate to them via LinkId::fromName. This lets callers that already hold a LinkId avoid redundant string-to-ID conversion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ulators Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Eliminates the LinkId->string->LinkId round-trip in the hot operator() by using calcJacobian(vals, link_id_) instead of calcJacobian(vals, getBaseLinkName(), link_id_.name()). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Changes private members from std::string to LinkId in CartPosConstraint, CartLineConstraint, and ContinuousCollisionConstraint. Eliminates repeated fromName() calls in hot-path operators (calcValues, error_diff lambdas). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update test files that construct CartPosConstraint with string literals to use LinkId::fromName() since the constructor now takes LinkId. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…fields CoeffEntry stored redundant name1/name2 strings alongside the LinkIdPair key. Replace PairsCollisionCoeffData from map<LinkIdPair, CoeffEntry> to map<LinkIdPair, double>. Also change CartesianLineConstraint to store LinkId directly for source/target frames instead of string + LinkId pairs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ys resolve collisions natively Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sites Restores the construct-once-reuse idiom the narrowphase callbacks already follow; halves per-contact key construction on the gradient path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… carries names Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NPsSTwLFt91Tc7hDRemssL
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… gradient path The gradient loops iterate the pair-keyed contact map with the key already in scope, so look the margin up once per pair alongside the coeff instead of re-deriving the fat pair per contact inside a virtual. Call the free getGradient directly and drop the now-trivial getGradient/calcGradientData virtuals. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NPsSTwLFt91Tc7hDRemssL
Zero callers workspace-wide. Also give the surviving 6-arg two-timestep overload its numbered @PARAM dofvals0/dofvals1 doc entries — the block documented a single dofvals parameter that does not exist. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The kinematic costs tests gained six calcFwdKin call sites through the upstream toleranced-Cartesian-waypoint fix (tesseract-robotics#560) merged in 3f01390, written against the string-keyed TransformMap that the NameId migration replaced with LinkIdTransformMap. Update the state-cache type at all six sites and include eigen_types.h, which defines the alias. All 8 tests in trajopt_kinematic_costs_unit pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Tesseract's createWithValueForTesting was replaced by a friend struct defined only in test-local headers; carry trajopt's own copy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H64vnNxAtUyarHHa4edpo7
These helpers only compared joint names for equality, so the strings were never needed. Removes the last name dependency in kinematic_terms.
checkTrajectory and getCurrentJointValues go id-native, dropping an ids->names->ids round trip from 44 call sites. addVar keeps its string labels, since a Var is a generic optimization variable, so ids convert to names once per scope at the 41 addVar sites.
Matches PlotCosts, which went id-native with the rest of trajopt, and drops a re-hash of every joint name per plotted trajectory row. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115dXrv5yUqynisbKY9HjGe
The T7 construct-once folds paid an owning pair - two heap string copies - per contact per evaluation pass. Hoisting the pair above each loop and assigning into it gives the same one-hash profile the deferred Phase 2 views were meant to provide, with no allocations and nothing to wait for. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Agent guidance, not part of the library; it was swept into an unrelated commit. The file stays on disk and is now ignored locally. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
JointState no longer takes joint names, and trajopt_ifopt Var labels stay strings, so the conversion moves to the call site. Same per-row cost the deleted constructor paid internally, now visible. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Unused since the 2021 switch to ContactResultsMarker, whose margin_fn lambda recomputes the margin itself instead of consuming the array. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The pre-fix/post-fix analysis already states the failure mode; the "Stash-revert failure (observed)" / "stash-reverting ... SHOULD cause" lines just recorded the verification workflow. Reword to state the pre-fix behavior directly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Both convex() methods fetched GetContactResultVectorCached, marked the result UNUSED, and discarded it. The fetch lost its last reader when margin/coeff resolution moved into CalcDistExpressions (results carried in exprs_margin/exprs_coeff); it only survived because the cache-accessor return-by-value conversion silenced the resulting unused-variable warning with UNUSED() instead of deleting it. CalcDistExpressions directly above already fills the cache for the same x, so each removed call drops a hash of x plus a cache probe per convexification pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Switches every trajopt/trajopt_ifopt/trajopt_sqp caller of the string-spelled Environment identity methods (getActiveJointNames, getRootLinkName, setState(unordered_map<string,double>)) to the already-existing id-typed twins, so these packages keep compiling once the string half is deleted.
Drop 4 explicit LinkId(...) wrappers where the target type is already fixed by context; test TUs enable implicit string->id conversion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XnQ84NASLsf6oQ2kGqXdnk
DiscreteCollisionConstraintD iterated contact_results_map directly, so after the ordered->unordered ContactResultMap migration its QP row layout followed unordered_map bucket order and could reshuffle between SQP iterations, misaligning OSQP's row-indexed warm-start duals. Mirror the continuous constraint: build a sorted key vector in update() and reuse it in getJacobian() so both iterate in a stable, deterministic order. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
value() fetched distances through CalcDists and then fetched the contact results again for the pair margins - two cache probes and a temporary DblVec per call, a shape left over from when margin and coeff were scalars. Read .distance off the cached flattened vector instead, and delete the now-empty CalcDists/CollisionsToDistances (uncalled in-tree, no overrides). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
OSQP's default 8192 iterations settles on a still-colliding trajectory; 32768 converges to a collision-free one. The cap is load-bearing for correctness, so keep it and note why. Also revert a stray id-migration rename: CartPoseJacCalculator's constructor parameter stays target_frame (matching its header declaration, sibling constructors, and upstream) rather than the off-minimal target_frame_id. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The migration reverted trajopt PRs tesseract-robotics#552 and tesseract-robotics#553 without saying so: the cast evaluator's batched pair-map setter became one broadphase flush per link, and seven calcFwdKin sites reassigned the persistent caches instead of filling them in place, undoing the thread_local-to-member change tesseract-robotics#553 made. Two ifopt evaluators had gained whole-map batching in the opposite direction, writing every entry of the supplied state and so reconciling the whole scene to a group-local FK snapshot instead of the diff and manip active sets they own. Also restores the continuous four-argument GetGradient overload, dropped while its discrete twin absorbed the same migration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
isSuperset goes back to a concrete declaration in utils.hpp with its definition in utils.cpp, and CollisionCoeffData::operator== back to the templated isIdenticalMap helper it hand-rolled a 15-line loop to replace. The "This does work but could be faster" notes are dropped rather than kept: they were accurate over the 2020 coeffRef loops they were written for, but every site is now reserve/startVec/insertBack, the fix they asked for. The migration had already deleted six of the ten. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cast_cost_octomap_arm_unit.cpp and the Coal default-manager swap in contact_manager_plugins.yaml were net-new work bundled into migration commits; both go back to upstream. The rest is churn: five redundant LinkId() wrappers where the implicit conversion is live in test TUs, undefined "three-tier overload" jargon naming an overload set that does not exist, and history narration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The local copy was made because a downstream repo could not include another package's test directory. Now that the header is an installed component that reason is gone, and the two copies had already drifted apart in their create() signature — one namespaced entity with two definitions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The string-compat save/load pair is unnecessary now that a LinkIdPair archives as its two names on its own, so both serializers collapse back to upstream's serialize() one-liner with the wire format unchanged. PairCoeffEntry existed only to keep a name alongside the coefficient while the key could not carry one; the key carries names now, so the map holds a plain double. The YAML decode gains a pair-taking setCollisionCoeff overload and passes its key straight through instead of splitting it into two ids for the setter to re-canonicalize. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both GetGradient overloads built a LinkIdPair per contact result, and the pair owns two strings. Neither lookup stores the key, so a thread-local pair that assign() rewrites in place reuses the string capacity and drops to zero allocations in steady state. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3849f32 to
ed11802
Compare
Requires tesseract-robotics/tesseract#1274 and tesseract-robotics/tesseract_planning#734