[RSDK-2259, et. al.] Remove RobotConstructor, clean up constraint descriptions #2190
Conversation
viambot
added
the
safe to test
|
Warning your change may break code samples. If your change modifies any of the following functions please contact @viamrobotics/fleet-management. Thanks!
|
viambot
added
safe to test
viambot
added
safe to test
| defaultRobotCollisionConstraintName = "defaultRobotCollisionConstraint" | ||
| defaultJointConstraint = "defaultJointSwingConstraint" | ||
| // descriptions of constraints. | ||
| defaultLinearConstraintDesc = "Constraint to follow linear path" |
motionplan/plannerOptions.go
Outdated
| defaultOrientationConstraintDesc = "Constraint to maintain orientation within bounds" | ||
| defaultObstacleConstraintDesc = "Collision between the robot and an obstacle" | ||
| defaultSelfCollisionConstraintDesc = "Collision between components that are moving" | ||
| defaultRobotCollisionConstraintDesc = "Collision between different pieces of the robot" |
There was a problem hiding this comment.
Want to further disambiguate these two:
"Collision between two robot components that are moving"
vs
"Collision between a robot component that is moving and one that is stationary"
There was a problem hiding this comment.
Yeah this was tough. More context is that I'm trying to avoid "reserved" terms in the Viam lexicon (which is why I didn't use parts, as originally suggested in the discussion thread talking about this).
I like the suggestions, though. Will swap over to those.
There was a problem hiding this comment.
Yeah its tricky. I think components is probably the preferred word
| defaultRobotCollisionConstraintName: robotConstraint, | ||
| defaultObstacleConstraintDesc: obstacleConstraint, | ||
| defaultSelfCollisionConstraintDesc: selfCollisionConstraint, | ||
| defaultRobotCollisionConstraintDesc: robotConstraint, |
There was a problem hiding this comment.
potentially dumb question: does caching longer strings affect performance? @biotinker
There was a problem hiding this comment.
Broadly yes it can; when storing in a map, a longer string takes longer to run through whatever hash function. Plus larger sizes in bytes mean larger memory copies that take longer, if copies of a map are made.
At the scale of this change, and with the number of iterations of things that we run, we are likely several orders of magnitude away from noticing a difference.
Theoretically, maps with keys from finite domains (such as ints) are trivially O(1) in space and time, and maps with keys with infinite domains (such as strings) require hashing and the specifics of equality testing to be included in the cost, which makes inserts and lookups best case O(N log N) on average (since the keys have to be at least O(log N) in size on average to construct a hash table with N entries.
From https://stackoverflow.com/questions/29677670/what-is-the-big-o-performance-of-maps-in-golang
There was a problem hiding this comment.
Yeah we aren't using this map a lot so I think this is fine, but something to be aware of I think
There was a problem hiding this comment.
An example of somewhere this is relevant is our RRT maps. Currently, it is a map of pointers to pointers, which is fast. However, this means that identical underlying node configurations are not only possible but common.
De-duplicating nodes in the RRT tree would involve actually parsing their contents rather than simply using an atomic memory address, and would have significant performance implications on path planning.
Additionally, this is why the CachedTransform function on model frames is not used; the performance penalty is sufficient that it is not consistently faster than simply recalculating forward kinematics.
viambot
added
safe to test
|
Availability
Quality
Performance
The above data was generated by running scenes defined in the
|
I tested this with a real xArm6, and a fake xArm6 and 7.
MoveSingleComponentworks equally well in place of a regulararm.MoveToPositionandmotion.Movecommands, given similar circumstances and equivalent goal construction.