Refactor is_close: fix broken overloads, add operator==, centralize eps#69
Merged
Conversation
Cleans up the is_close family of comparison functions: - Remove the infinitely-recursive generic is_close<T> template and the redundant integer overloads; integral comparisons now use == / !=. - Fix is_close(ObjMatrix<T>) to actually compare elements, and add a working ObjMatrix::operator== that dispatches via if constexpr (is_close for floating-point, == for integral/custom types). - Replace the per-type is_close overloads for Manipulator, Bspline, ConstraintSelection, Objective, Guess, Optimization and Result with operator==; add operator== for Mat3. - Fix correctness bugs: inverted logic in Guess::random comparison, dropped eps forwarding, missing element comparison in ObjMatrix, and a non-const Result parameter. - Centralize the default tolerance on the previously-unused BLAST_EPSILON constant so it can be changed in one place. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e, early-out Follow-up fixes from code review of the is_close refactor: - Manipulator::operator== compares the joint limit arrays (position/velocity/ acceleration/torque max, position min) with is_close over [0, n_joints) instead of exact std::array == over all MAX_JOINTS slots, restoring eps tolerance and ignoring unused trailing slots. - Result::operator== guards the raw opt pointer before dereferencing, avoiding UB on a null/equal pointer. - is_close(real) collapses the nested INF ternary to r1 == r2 || abs(r1 - r2) < eps (equivalent, adds an exact-equality fast path). - Manipulator::operator== hoists the cheap _n_caps / _n_internal_collisions integer checks above the per-joint loops to fail fast. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refactors the
is_closefamily of comparison functions, which had accumulated several broken and inconsistent overloads.Bugs fixed
is_close<T>template called itself unconditionally. Removed entirely.Guess::randomreturned "not close" when the shot counts were equal.ObjMatrix:is_close(ObjMatrix<T>)only compared.size, never the elements;ObjMatrix::operator==had an emptyis_close()call that wouldn't compile once instantiated.eps: several call sites (n_points,static_rotations,Resultfields) silently fell through to the broken generic template instead of forwarding the tolerance.is_close(const Result&, Result&, ...)took its second argument by non-const reference.Changes
==/!=directly instead of dedicated overloads.ObjMatrix::operator==dispatches withif constexpr:is_closefor floating-point elements,==for integral and custom types. Forward-declaresis_close(real,...)so the floating path resolves under strict two-phase lookup (Clang/GCC), not just MSVC.is_closeoverloads forManipulator,Bspline,ConstraintSelection,Objective,Guess,Optimization, andResulttooperator==; addedoperator==forMat3.Manipulatorcomparison (it duplicated the array-level checks).BLAST_EPSILONconstant so it can be changed in one place.Test plan
cmake --build build --config Release, exit 0).tests/manipulators/test_auto_manip.cppupdated to use==and exercisesManipulator::operator==(and transitivelyObjMatrix<u8>::operator==).🤖 Generated with Claude Code