Conversation
gchenfc
left a comment
gchenfc
left a comment
There was a problem hiding this comment.
Looks great! All comments are minor.
See minor comments, ignore comments about using std::map instead of gtsam::Values for joint angles, though goal_poses comment still applies.
Joobz
left a comment
Joobz
left a comment
There was a problem hiding this comment.
Wow, sorry for commenting comment by comment giving you probably a lot of notifications... I just found about the reviewing the changes directly. I will know for the next time!
Also, I have done some more commits that should be addressing the comments. The most important change is the creation of PoseGoals, a container similar to the ContactGoals one., like you asked @gchenfc
There was a problem hiding this comment.
Just the couple minor things.
Also, in the future, reply "Done" on comments you fixed/addressed so that we only have to look at comments still in question. (can you do that on the previous review comments as well?)
I think it's fine to comment on the comments instead of "starting a review".
dellaert
left a comment
dellaert
left a comment
There was a problem hiding this comment.
Could not finish entire review but please take my comments (really the same comment twice) and generalize them to the rest of the PR, then re-request my review.
|
@Joobz please finish up this PR before you head out, else it will stay in review purgatory. :) |
# gtdynamics/kinematics/Kinematics.h # gtdynamics/kinematics/KinematicsInterval.cpp # gtdynamics/kinematics/KinematicsSlice.cpp # gtdynamics/kinematics/KinematicsTrajectory.cpp # gtdynamics/pandarobot/gpmp/GaussianProcessPrior.h # gtdynamics/universal_robot/Joint.cpp
|
@copilot check if all previous review comments were addressed, and how. |
…d joint_priors parameters Co-authored-by: dellaert <10515273+dellaert@users.noreply.github.com>
Co-authored-by: dellaert <10515273+dellaert@users.noreply.github.com>
Co-authored-by: dellaert <10515273+dellaert@users.noreply.github.com>
[WIP] Update kinematics functions for improved objectives
dellaert
left a comment
dellaert
left a comment
There was a problem hiding this comment.
I merged in master in this very old PR and resolved conflicts. Co-pilot addressed some very old comments. Now ready to merge.
dellaert
dismissed
gchenfc’s stale review
I think/hope I addressed those issues with copilot.
|
I had mentioned way back in 2021 moving the robot specific directories to their own repositories. This was motivated by the fact that we would have to own this technical debt moving forward (seems like the debt has come to collect now). It may be prudent to do that now. |
I don't understand. Why do you say it's an issue now? |
This PR is about 4 years old and a lot has changed in GTSAM and GTDynamics in that time. The Panda, Cable Robot and Jumping Robot code is not essential to GTDynamics so having them in this codebase with potentially failing tests in the future adds a burden to maintainers (in this case you), especially when their need is intermittent. My recommendation to have them in a separate repo could have saved us this current effort since that codebase would be smaller and have cleaner abstractions. From the comments, it seems like there is uncertainty if all the the issues raised have been fixed and it has been so long that I don't even remember the context/purpose of this PR in the overall plan. I also don't trust AI fully yet on these things (there's a full report on how AI incurs significant technical debt). |
|
This was a relatively simple PR, including an ikfast for the Panda. I hear your arguments, but I'd like to keep things as they are for now. Coordinating multiple repos is also its own level of pain. |
|
It's easier than it seems. I had separate repos for the walking and hybrid walking, while working across GTSAM and GTD. |
This PR changes and adds some functions in kinematics:
poseGoalObjectives(): add prior to link COM poses.jointAngleObjectives(): modified it so we can add any mean to the prior. Previously it was always centered at the origin.jointAngleLimits(): add a jointLimitFactor only to the angles, compared to the one in joints that adds a factor to all dynamics related variables.initialValues(): modified it so we can start from non-random points for arbitrary joints.inverseWithPose(): IK with a given COM pose for the 7th link.There are also some changes to Robot and Joint files that @dellaert changed because we ran into some problems.