Adds Sphere Radii to the ObstacleGP factors - #437
Conversation
|
Could you check for copy- paste functionality that should be shared rather than copied? |
There was a problem hiding this comment.
Pull request overview
This PR extends GP-interpolated obstacle avoidance (ObstacleSDFFactorGP) to support sphere-based query points by adding a per-query-point radius vector (folded into the standoff distance), consistent with existing sphere handling in other factors. It also centralizes argument validation shared across obstacle SDF factors and updates the wrapper interface and tests accordingly.
Changes:
- Add per-query-point
radiisupport toObstacleSDFFactorGP(new constructor + error computation usesepsilon + radius). - Refactor shared validation into
validateObstacleSDFFactorArgs(...)and reuse it from both unary and GP obstacle factors. - Update gtwrap interface (
gtdynamics.i) and unit tests to exercise nonzero radii behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| gtdynamics/gpmp2/tests/testObstacleFactors.cpp | Updates tests to pass nonzero radii and validate GP/unary agreement and Jacobians with radii folded into standoff. |
| gtdynamics/factors/ObstacleSDFFactorGP.h | Adds radii_, a radii-aware constructor, and shared validation call. |
| gtdynamics/factors/ObstacleSDFFactorGP.cpp | Uses epsilon_ + radii_(i) when evaluating obstacle cost at each query point. |
| gtdynamics/factors/ObstacleSDFFactor.h | Introduces exported shared validation function and uses it in constructors. |
| gtdynamics/factors/ObstacleSDFFactor.cpp | Implements shared validation logic for sdf/epsilon/radii consistency. |
| gtdynamics.i | Exposes the new ObstacleSDFFactorGP constructor with radii and the radii() accessor to the wrapper. |
|
I did check for copy-paste functionality, but the sphere requirements here are slightly different for obstacleFactor vs. selfCollisionFactor. I'll check again though, but for now I put the obstacleSDFFactor to share its sphere-checking with the GP- version. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Karthik, I vibed with 5.6 Sol about my gut feeling about copy paste - it agreed and we came up with the following plan. Please read and evaluate whether you agree, and if so, work with an AI of your choice to plan and execute :-) The shared radius validation is an improvement. I agree that obstacle avoidance and self-collision have different requirements, so I am not asking you to combine their actual collision calculations. However, there is still substantial duplication between each regular factor and its GP version. Please consolidate that before merging. For obstacle avoidance,
Please move this work into one shared function or small class that evaluates the obstacle cost for a given configuration Please make the equivalent change for
Again, the GP factor should only add interpolation around this shared calculation. There is also duplicated GP-specific code in the two GP factors: both interpolate Please also address the following while doing the refactor:
In short, obstacle avoidance and self-collision should remain two separate calculations, but each calculation should have only one implementation. The distinction between the regular and GP factors should be how the configuration is obtained, not a second copy of the collision calculation. |
…oid exposing these shared helpers in the api
|
I did the cleanups, and also edited the tests. The shared helpers I moved to the ''details'' folder so they wouldn't get exposed publicly. I thought this was the best option for that, but please let me know if there's another method you prefer! |
dellaert
left a comment
There was a problem hiding this comment.
Three follow-ups on organizing the now-consolidated implementation:
|
Amendment: I don't think the header-only factors need to imply that the internal helpers need to be header-only. So I'm fine leaving those as two files. |
dellaert
left a comment
There was a problem hiding this comment.
Looks great now! Thanks for being so responsive!
This PR adds the ability to do sphere-based obstacle avoidance in interpolation by adding it to ObstacleSDFFactorGP. It follows similarly to how this was done in selfCollisionSphereFactor.
There sphere-based obstacle avoidance already exists in ObstacleSDFFactor, so it's not needed to be added there.