Skip to content

Move from string-based links and joints to id-based logic (using hashes)#14

Draft
rjoomen wants to merge 52 commits into
tesseract-robotics:mainfrom
rjoomen:feature/integer-link-ids
Draft

Move from string-based links and joints to id-based logic (using hashes)#14
rjoomen wants to merge 52 commits into
tesseract-robotics:mainfrom
rjoomen:feature/integer-link-ids

Conversation

@rjoomen

@rjoomen rjoomen commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 46 out of 46 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

tesseract/collision/coal/src/coal_utils.cpp:914

  • updateCollisionObjectFilters() calls isLinkActive(active_ids, cow->getLinkId()), but there is no definition of isLinkActive anywhere in this repository (searching only finds these call sites). This will not compile. Replace the call with an inline membership test against active_ids (e.g., active_ids.find(cow->getLinkId()) != active_ids.end()) or add the missing helper function in a shared header/source file.
void updateCollisionObjectFilters(
    const std::unordered_set<tesseract::common::LinkId, tesseract::common::LinkId::Hash>& active_ids,
    const COW::Ptr& cow,
    const std::unique_ptr<coal::BroadPhaseCollisionManager>& static_manager,
    const std::unique_ptr<coal::BroadPhaseCollisionManager>& dynamic_manager)
{
  // For discrete checks we can check static to kinematic and kinematic to
  // kinematic
  if (!isLinkActive(active_ids, cow->getLinkId()))
  {
    if (cow->m_collisionFilterGroup != CollisionFilterGroups::StaticFilter)
    {
      const std::vector<CollisionObjectPtr>& objects = cow->getCollisionObjects();

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tesseract/collision/coal/src/coal_cast_managers.cpp Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 47 out of 47 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 47 out of 47 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

tesseract/collision/test/coal/coal_deferred_octree_expansion_unit.cpp:71

  • getCastCollisionObjectMap() returns a Link2COW keyed by tesseract::common::LinkId, so calling cast_map.find("octree_link") will not compile (no heterogeneous lookup for std::unordered_map). Use cast_map.find(tesseract::common::LinkId("octree_link")) (and update the other occurrences in this file).
  const auto& cast_map = checker_.getCastCollisionObjectMap();
  auto it = cast_map.find("octree_link");
  ASSERT_NE(it, cast_map.end());

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread STRING_PERF_BRIEF.md Outdated
rjoomen and others added 17 commits May 4, 2026 19:30
Update test validator subclasses to use (LinkId, LinkId) interface.
Switch coal_utils key construction from THREAD_LOCAL LinkNamesPair to
LinkIdPair::make(LinkId::fromName(...)). Update test key constructions.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ds.name()

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Since LinkId now carries the name, the parallel std::vector<string>
active_ member in Coal collision managers is redundant. Remove it,
keeping only the unordered_set<LinkId> active_ids_ for O(1) lookup.

- Change getActiveCollisionObjects to return by value
- Update both Coal managers and updateCollisionObjectFilters helpers
- Fix test_suite headers to avoid binding temporaries to const auto&

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Override the new virtual setActiveCollisionObjects(vector<LinkId>)
for both CoalDiscreteBVHManager and CoalCastBVHManager. Clone methods
now pass IDs directly. Disambiguate brace-init-list call sites in
test/benchmark code to resolve overload ambiguity.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update collision_objects_ member type in Coal discrete and cast manager
headers to match the base class return type change.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Change collision_objects_ member from vector<string> to vector<LinkId>
in both Coal managers. Update getCollisionObjects() return type
accordingly. Eliminates fromName() hash calls in base class defaults.

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>
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>
Match base class change: operator()(const LinkId&, const LinkId&).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
LinkId now carries the name string. getName() returns link_id_.name().
getLinkId() returns const&. Constructor takes name by const& since
it's no longer moved into a member. Saves ~32 bytes per COW and
one string copy at construction.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Hoist link_pair construction before margin lookup to eliminate a redundant
LinkIdPair::make() call.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
rjoomen and others added 4 commits May 4, 2026 19:30
…d adopt set-based active-ids interface

std::hash<NameId> is now the identity function (see Tesseract's
IDENTITY_DESIGN.md), so the explicit LinkId::Hash template argument on
std::unordered_set<LinkId> / std::unordered_map<LinkId, ...> is no longer
necessary — the default specialization is already identical. Drops the
custom arg from active_ids_, Link2COW, and the two updateCollisionObjectFilters
signatures.

setActiveCollisionObjects now takes std::unordered_set<LinkId> directly
instead of std::vector<LinkId>, matching the base-class interface flip.
clone() passes active_ids_ through unchanged, and the managers store
the set with a single move/copy instead of clear()+insert(range).

Also switches CollisionCacheMap from boost::unordered_flat_map to
std::unordered_map, dropping the boost include.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…PLICIT

Mirror the tesseract repo split of TESSERACT_COMPILE_DEFINITIONS into
_PUBLIC / _PRIVATE and apply TESSERACT_NAMEID_NO_IMPLICIT PRIVATE on
the coal and coal_factories libraries. Tests, benchmarks, and the
test_suite INTERFACE library keep PUBLIC-only defs, so implicit
string→LinkId conversion stays available there.

Drop the now-redundant LinkId("...") wraps from map indexing and
find/assign sites across test/ and test_suite/ headers. The 26
remaining wraps are auto-deduction declarations where the wrap is
load-bearing.

Drive-by: re-run clang-format on touched files and merge a few split
string literals that fit on one line again.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@rjoomen
rjoomen force-pushed the feature/integer-link-ids branch from 4357489 to 2b47728 Compare May 4, 2026 17:30
rjoomen and others added 20 commits May 9, 2026 02:47
…e + cast managers

Pairs with the upstream tesseract commit adding the pure-virtual getter to
the abstract Discrete/Continuous contact managers. CoalDiscreteBVHManager
and CoalCastBVHManager forward to the underlying COW's
getCollisionObjectsTransform(); the cast manager returns pose1.
…ream cleanup

The pair-form tesseract::collision::isContactAllowed lost its verbose
parameter (verbose name-based logging now lives in the (LinkId, LinkId)
overload because LinkIdPair stores only NameIdValue ids and not names).
Mirror the upstream FCL change in coal: drop the now-unused verbose
parameter from the coal-internal needsCollisionCheck wrapper and from
its single call site.
…ntactResultMapUnit

The master-compatible ContactResultMap cereal save derives the on-disk
string key from results.front().link_ids[0/1].name() and tesseract-side
commit bf1e769b9 added a defensive throw when those names are empty
(default-constructed ContactResult would silently collapse all keys to
("","") on the wire). The shared TesseractCoreUnit.ContactResultMapUnit
test passed default-constructed ContactResults, which now trips the guard.

Mirror the tesseract-side helper makeContactResultForKey + named-LinkId
constants pattern so each result's link_ids match its LinkIdPair key.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adopt the NameId operator<< from tesseract_common: drop .name() at
streamed display sites, which write the identical text.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CollisionCallback is a per-contactTest local, so it can own the scratch pair
outright - cheaper than a thread-local, and it stops the callback building two
heap string copies for every candidate it is handed.

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>
The mirrored suite never picked up tesseract 43c5eb678, fd378a8ba and
03162bd50, nor the Bullet/FCL half of c140df934. The material gap was the
ContactResultMap cereal wire format - a cross-repo on-disk contract this
package exercised nowhere.

Also restores verifyLinkIdsConsistency, which was dropped along with both of
its call sites, and the five backend cases that get_collision_objects_transform
replaced rather than extended.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The mirrored copies still round-tripped LinkId through std::string to call
the id-taking manager getters, compared via .name(), and declared ids
explicitly where tesseract constructs them implicitly from literals.

No semantics change - it makes the two copies diff cleanly, so the next
drift is visible instead of buried in spelling noise.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Clears the 133 vector<string> call sites in coal's forked copy of the
collision test suite plus its coal-specific tests, ahead of the
overloads' removal from tesseract.
Drop the explicit std::vector<LinkId>{...} wrapper in favor of the bare braced list now accepted by the core managers, and reconcile the shared test_suite active-object assertions with the tesseract copy. Tests only.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Port the two parallel-array setCollisionObjectsTransform size-mismatch
EXPECT_THROW tests from tesseract so the shared driver stays aligned, and
correct the benchmark @brief comments to the id-native setter signatures.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Mirror tesseract's cleanup of the ported ContactResultMap cereal test file:
drop line-number anchors and the "Phase 2 test additions" banner, and reframe
the cereal wire-format comments as string-keyed / hash-non-portable rather than
"master"-relative. Comment-only; keeps this port aligned with tesseract's copy.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mirror tesseract: rename the ported ContactResultMap cereal tests and their
local fixture identifiers to name the string-keyed wire format directly. Keeps
the port aligned. Test bodies and assertions are unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
BM_CLONE pre-sized active_obj to num_obj then push_back'd num_obj more,
leaving the vector at 2*num_obj with the first half default-constructed
(invalid) LinkIds. Index-assign like the sibling setup functions so the
active set holds exactly num_obj valid ids.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Inline the map subscript and qualify LinkIdTransformMap in the doc comment
to match the tesseract copy of the shared benchmark header, removing a
pre-existing drift between the two forked test suites.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A prior change shared CastHullShape's GJK warm-start state with the continuous-
collision support averaging. With penetration off, GJK boolean/distance
early-outs without EPA, so that seed points away from the contact normal and
the averaging hill-climb runs long — a ~10-12% CoalCast Contact-Only regression.

Fall back to the direction-independent O(V) scan when penetration is off, and
run the warm climb on thread_local scratch so averaging never writes back into
the sweep's warm-start state. Contact-Only recovers above the string baseline;
penetration-enabled modes keep the warm-start win.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
computeLocalAABB fit the pose-1 swept-hull AABB to convex hulls exactly, an
O(num_points) per-vertex pass run on every cast transform update. That cost
is paid once per continuous collision check and outweighs the broadphase
pruning it buys, slowing continuous checking. Route convex hulls to the O(1)
computeBV<AABB, ShapeBase> bound; primitives keep their exact analytic AABB.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The collision-result paths built full transform inverses only to map points
into link-local frames. For a rigid transform, tf.inverse() * p == Rᵀ(p − t),
so a stateless applyInverse helper does the same work without constructing the
inverse. This also drops the CastHullShape castTransformInv_ cache, whose
rotation was only ever used as a transpose.

Results are unchanged (exact for rigid transforms).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The base-class array setters loop the single-object setter, flushing the
broadphase per object and never reaching the full-refit branch. Override them
(discrete one-pose, cast one-pose and two-pose) to collect then flush once. Also
restores the debug size/presence asserts on the cast map-pair setter. Adds a
conformance test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@rjoomen
rjoomen force-pushed the feature/integer-link-ids branch from 8ae2f28 to d15387a Compare July 21, 2026 05:55
rjoomen and others added 4 commits July 21, 2026 15:10
The :*-master image provides string-based tesseract, which lacks
tesseract::common::LinkId, so the id-branch fails to compile. Overlay
tesseract feature/integer-link-ids from source so CI builds against the
migrated API (and picks up the PR's ongoing fixes).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
link_names/names/static_names/active_names hold vector<LinkId>;
rename to the id-equivalents in the shared collision test suite.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants