RSDK-251 Add support for capsule geometry types #1766
Conversation
viambot
added
the
safe to test
viambot
added
safe to test
…-cylinder-geometry-types
viambot
added
safe to test
biotinker
changed the title
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
removed
the
safe to test
viambot
added
the
safe to test
| // box density corresponding to how many points per square mm. | ||
| const defaultBoxPointDensity = .5 | ||
| // Ordered list of box vertices. | ||
| var boxVertices = [8]r3.Vector{ |
There was a problem hiding this comment.
[nit] this is the most minor but can we move this section down to where it is used? I'd like the top of this file to stay the most relevant things to a user, and this is only used in mesh creation
There was a problem hiding this comment.
Globals like this should generally be the first thing (or one of the first things) in a file after the imports.
I don't want to declare these in the function where they're used, because that's a performance-sensitive function and redeclaring these variables on each call will be noticeably expensive.
Perhaps a different file would be appropriate for these? geometry_utils.go if you like
There was a problem hiding this comment.
Yeah I agree that generally you're right. I just feel it hurts readability a bit. You can do whatever you think is right here just wanted to raise this point
| // .x| |-------O-------| |x | ||
| // ...\___________________/ | ||
| // | ||
| // Length is the distance between the x's, or internal segment length + 2*radius. |
There was a problem hiding this comment.
I don't think I like specifying length as the distance between X marks. If Length were just the internal segment length, we could eliminate some checks for odd values of L versus R, but I haven't yet looked ahead to see if this helps simplify other collision checks. Some idea of why you chose this representation would be appreciated.
There was a problem hiding this comment.
To be clear, this is not a big deal to me.
There was a problem hiding this comment.
I put quite a bit of thought into that choice and went back and forth. This is purely a user experience decision, not something that impacts collision checks at all.
I agree that specifying segment length rather than total length makes for a "cleaner" mathematical representation, for exactly the reasons you mentioned.
However, if we do that, we make it mildly more difficult for a user to actually use. Currently the values are ones that are easy to physically measure. Let's say we go to make a capsule representation of the internal geometries of a UR5 next week. We pull out a tape measure, say "this limb is 175mm long, and 34.3mm diameter, so 17.15mm radius" and we plug that in. We don't have to worry about simple arithmetic to compute the internal segment length, which isn't something that's easily physically measured.
The majority of capsule implementations I found in the wild similarly specify full length, at least at the user-facing interfaces.
There was a problem hiding this comment.
FWIW I agree with you on this choice Peter, the most measurable thing should be the thing exposed to the user
There was a problem hiding this comment.
If you can't measure both aspects I'm not sure what the user is doing.
| vecList := s.ToPoints(resolution) | ||
| // move points to be correctly located on capsule endcaps | ||
| adj := c.length/2 - c.radius | ||
| for _, pt := range vecList { |
There was a problem hiding this comment.
Does this block mean vecList has some points which are duplicated along the hemisphere of each end of the capsule?
There was a problem hiding this comment.
Never mind, I see what you're doing. You're splitting a sphere into its two halves and shifting the halves to each end. Nice, I like it.
viambot
added
safe to test
viambot
added
safe to test
|
This adds capsules as a geometry type, supporting efficient collisions with all other geometry types.
Along with this is an initial implementation of meshes- not supported as a geometry type, simply as a backend representation. This is due to a box-capsule collision implementation being taken from here: https://github.com/google/brax/blob/7eaa16b4bf446b117b538dbe9c9401f97cf4afa2/brax/physics/colliders.py
The separating axis theorum is instead used here for the capsules when doing collisions as it is faster. Perhaps we should PR against brax?