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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RSDK-5391 - Create distinct planning and execution frames for PTG KinematicWrapper #3876
base: main
Are you sure you want to change the base?
RSDK-5391 - Create distinct planning and execution frames for PTG KinematicWrapper #3876
Changes from 23 commits
247d4d1
652ddcc
685d518
de3189d
413cfa3
14fc7fe
2aab600
6a49f8a
aa98dea
ff9e945
44b3287
cde23a6
ba2be7d
526b14d
656d154
09c27a8
662ef08
a8eca26
61d8fc1
5bc3a0a
7c07b15
7185d6e
d454d21
5e39ce8
db3ca28
d8ab4ce
6917eb9
7e25a8d
ce18c03
dfeec43
ce5a122
1d18bb5
2570e15
4b42874
a4d816f
0aa7c7f
fff9cf6
074cfbb
c745c4b
a21cc99
c064634
e91f0a9
ac7d32a
5bb3375
716f095
eb5a38a
d7e1a6b
9b0405d
923f362
c197731
01b7d93
eaf8ffc
7f7e4c6
02fe62f
c3569dd
1344a84
b6aac16
4f68a9f
cfa4aea
c6c995c
21cdb8e
40d4a77
43f910d
dbf6c19
ce50772
ddbde4d
d39e5d8
1d99d0c
29af1f5
8118206
21c8a4f
f149090
a3e1ee2
6a3a794
668a27a
7648907
5004e4c
3f2df50
ec18e95
3e186d9
34f5269
9d6dbe7
24c7349
03ed80a
3978b66
56636f4
ba682e7
7afc8ed
ee14bf1
9864457
f43114d
34352cd
59481af
d4e1c11
3fc360b
3409b40
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO there should be a better name for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will think of a better name. Maybe
ExecutionKinematics
?@raybjork lmk if you have any opinions on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kinematics is not really correct here the frame has nothing to do with kinematics of the base
Maybe something related to its localization, since thats the transformation the frame is responsible for making? Perhaps
LocalizationFrame
or something similar? Open to other suggestions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a full 6dof pose, not a 2d pose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its premature to make this change there is no use case yet where we need more DOF than what exists for (x, y, theta)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating such a frame would require construction of a new frame with inputs [x, y, z, roll, pitch, yaw] and since we would only be using 3 of those for now it seems unnecessary to introduce this code at this moment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree.
Consider a 2d robot rolling over a bump with a lidar. It may be temporarily looking up or down, and seeing bonus obstacles that are actually the floor or ceiling. If we capture the pose of the robot, we are able to account for this.
As written we are throwing information away and actively weakening our current spatial representation.
A 2d robot can only actuate in those three dof, but that's captured by the planning frame. A 2d robot can exist in a full 6dof pose, and that's what this frame should be able to capture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is weird. Why just the first geometry? What are you doing with the DoF?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right that this is weird. We now choose to not take geometry at all for the mobile frame. We also now define our own separate limits, although they might be incorrect in this iteration.