[RSDK-1258] World State to Point Cloud Conversion Method #1675
Conversation
viambot
added
the
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
spatialmath/box_test.go
Outdated
| myMap["xIter"] = 0.5 | ||
| myMap["yIter"] = 0.5 | ||
| myMap["zIter"] = 0.5 | ||
| box.ToPointCloud(myMap) |
There was a problem hiding this comment.
We'll need to think about how to test this better, because as is this will not catch any changes that would break your function
There was a problem hiding this comment.
based off the dimensions we can calculate how many points the r3.vector list contains. because we also have the center and rotation matrix we can apply that to the existing vertices and check within the r3.vector list for each corresponding vertex
There was a problem hiding this comment.
What I would like to see as a test for this, would look like, say, a 2x2x2 box, a 1x1.5x4 box, and a radius=2.5 sphere, at hardcoded poses, with the expected set of points also hardcoded.
spatialmath/box.go
Outdated
| @@ -352,3 +355,84 @@ func separatingAxisTest(positionDelta, plane r3.Vector, halfSizeA, halfSizeB [3] | |||
| } | |||
| return sum | |||
| } | |||
|
|
|||
| // TODO: add function description | |||
| func (b *box) ToPointCloud(options map[string]interface{}) ([]r3.Vector, error) { | |||
There was a problem hiding this comment.
I think this can just become resolution int since it should be the same in all dimensions
There was a problem hiding this comment.
float64, not int. Probably in units of points per mm?
There was a problem hiding this comment.
ok sure, but we need to keep map[string]interface{} because this allows for empty inputs, i.e. the user can use default spacing if they don't want to thing about it
spatialmath/box.go
Outdated
|
|
||
| var faces [][]float64 | ||
| // which faces are these | ||
| for i := 0.0; i <= dims[0]; i += options["xIter"].(float64) { |
There was a problem hiding this comment.
starting at the center, adding 2 points per iteration, per face, and working inside to outsid iterating until i <= halfSize will mean that we'll run into symmetric errors if the resolution does not evenly divide the dimension
There was a problem hiding this comment.
agreed, but if we allow the user to dictate the resolution should it be on them to figure this out?
What do you recommend?
There was a problem hiding this comment.
also suppose we want to render a cube with side length 1.
both 1 / 0.0.3 and 1 / 0.13 would result in symmetric errors with the former being much less noticeable.
spatialmath/box.go
Outdated
| } | ||
| } | ||
| // which faces are these | ||
| for j := 0.0; j <= dims[1]; j += options["yIter"].(float64) { |
There was a problem hiding this comment.
This should probably be a helper function
There was a problem hiding this comment.
agreed.
edit: now that i am thinking about it a helper function for these three nested for loops could be a little tricky. This is because the inside of all three does different things. I think that it is still possible to have them be a helper function with the use of switch statements or something of that manner.
spatialmath/box.go
Outdated
| } | ||
| } | ||
| // what does this do | ||
| min := b.Vertices()[0] |
There was a problem hiding this comment.
Can't we offset by the box.Pose().Point() instead of going through this?
spatialmath/point.go
Outdated
|
|
||
| // TODO: add descriptions | ||
| func (p *point) ToPointCloud(options map[string]interface{}) ([]r3.Vector, error) { | ||
| myVec := r3.Vector{p.pose.Point().X, p.pose.Point().Y, p.pose.Point().Z} |
There was a problem hiding this comment.
return []r3.Vector{p.pose.Point()} should work here. Also I'm noticing I really shouldn't have made Point store a Pose since theres no point in storing orientation data 🙃
There was a problem hiding this comment.
should I edit the struct of Point to remove Pose?
There was a problem hiding this comment.
This would actually be a good fix too thanks
spatialmath/box.go
Outdated
| } | ||
| } | ||
| // what does this do | ||
| for _, v := range faces { |
There was a problem hiding this comment.
I think that both this, and the matrix transformation below, can be replaced by transforming each point by PoseInverse(box.Pose())
There was a problem hiding this comment.
I agree that this will work but this part of the code will be gone in the newest version - pushing now
viambot
added
safe to test
viambot
added
safe to test
nfranczak
changed the title
nfranczak
changed the title
spatialmath/box.go
Outdated
| // the boolean values which are passed into the fillFaces method allow for common edges to be | ||
| // ignored. This removes duplicate points on the box's surface | ||
| var facePoints [][]r3.Vector | ||
| facePoints = append(facePoints, fillFaces(b.halfSize, iter, 0, true, false)) |
There was a problem hiding this comment.
facePoints should be a []r3.Vector, and these appends should use elipses ... to expand the arguments for append.
There was a problem hiding this comment.
Example usage:
https://stackoverflow.com/questions/40657553/all-uses-of-ellipsis
Here's some more background on variadic functions such as append:
https://programming.guide/go/three-dots-ellipsis.html
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
|
Note: merge base coverage results not available, comparing against closest f361208~2=(6ab9820) instead
|
This ticket involves adding a conversion function for box, sphere, and point
geometrytypes into aPointCloudThe conversion is two stepped:
ToPointsmethod on the givengeometry.ToPointsreturns type[]r3.VectorVectorsToPointCloud. This method has one argument of type[]r3.Vector[].VectorsToPointCloudreturns aPointCloudThis ticket also removes the
Posefield from thePointstruct and replaces it withr3.Vector