Skip to content
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-1059 Add ParallelProjectionOntoXZwithRobotMarker for SLAM visualizations #1762

Merged
57 commits merged into from Jan 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
a6a7165
RSDK-1059 ParallelProjectionOntoXZWithRobotMarker
Jan 10, 2023
2a9c031
test
Jan 11, 2023
0cea639
voxel size fix
Jan 11, 2023
c90d1bd
voxel size fixes
Jan 11, 2023
95511ec
voxel voxel
Jan 11, 2023
feaf825
pull changes from pi
jeremyrhyde Jan 11, 2023
8dd7374
final fixes before pr submission
Jan 11, 2023
5ed05bc
remove comment
Jan 11, 2023
fec23e7
ad d tests
Jan 12, 2023
f0b51b5
delte iamge
Jan 12, 2023
8d5650c
add additional test
Jan 12, 2023
694f1de
i,j
Jan 12, 2023
f4fb61b
final comment edits and additional test
Jan 12, 2023
4542a75
make buildl int
Jan 12, 2023
b56cb09
Merge branch 'main' of github.com:viamrobotics/rdk into RSDK-1059_Par…
Jan 12, 2023
ad43e1d
PR fixes
Jan 13, 2023
e9a8e07
make buildl int
Jan 13, 2023
12112af
addiitonal testing of point pos
Jan 13, 2023
eecabe8
comment fixes
Jan 13, 2023
075fa01
PR fixes
Jan 17, 2023
f859d61
Merge branch 'main' of github.com:viamrobotics/rdk into RSDK-1059_Par…
Jan 17, 2023
909e7e0
add error for values >100 or <0
Jan 17, 2023
ad48a3f
condense lines
Jan 17, 2023
f22eef4
test
Jan 17, 2023
1cf8fdb
attempt new error
Jan 17, 2023
db347a3
Merge branch 'main' of github.com:viamrobotics/rdk into RSDK-1059_Par…
Jan 17, 2023
4fbebfa
finally solved the issue
Jan 17, 2023
a8a8e5a
nevermind this might solve it
Jan 18, 2023
8ba1be6
test separate case
Jan 18, 2023
cea4166
finally a fix!
Jan 18, 2023
6ae6176
imprvoemnts
Jan 19, 2023
bd6e93a
PR fixes
Jan 20, 2023
c0028e5
safe math
Jan 20, 2023
c81ca1c
expand on comment
Jan 20, 2023
4d9cb1b
forgot to remove print
Jan 20, 2023
8b7b4bb
small fix
Jan 20, 2023
e42856d
small fix
Jan 20, 2023
b43927b
small fix
Jan 20, 2023
610b3e3
ermege
Jan 20, 2023
2e24c1b
refactor
Jan 20, 2023
91219b8
change iamge coloring slightly
Jan 20, 2023
185d072
Update rimage/transform/parallel_projection.go
Jan 20, 2023
29c163b
Update rimage/transform/parallel_projection.go
Jan 20, 2023
8ba4984
Update rimage/transform/parallel_projection.go
Jan 20, 2023
69e3384
Update rimage/transform/parallel_projection.go
Jan 20, 2023
76c15d8
Update rimage/transform/parallel_projection_test.go
Jan 20, 2023
6ac6176
Update rimage/transform/parallel_projection.go
Jan 20, 2023
67ad566
Update rimage/transform/parallel_projection.go
Jan 20, 2023
3127c96
Update rimage/transform/parallel_projection_test.go
Jan 20, 2023
5fa0b4b
Update rimage/transform/parallel_projection.go
Jan 20, 2023
9c9b261
Update rimage/transform/parallel_projection.go
Jan 20, 2023
09a647d
Update rimage/transform/parallel_projection_test.go
Jan 20, 2023
5cad219
Update rimage/transform/parallel_projection.go
Jan 20, 2023
fde85d8
Update rimage/transform/parallel_projection_test.go
Jan 20, 2023
f4789e8
PR fixes
Jan 20, 2023
48cbf2a
add artifact tree
Jan 23, 2023
69fb771
Merge branch 'main' of github.com:viamrobotics/rdk into RSDK-1059_Par…
Jan 23, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .artifact/tree.json
Original file line number Diff line number Diff line change
Expand Up @@ -3721,6 +3721,10 @@
"test.pcd": {
"hash": "6eb977c07e0b40d9535318d94ba6db2e",
"size": 4693948
},
"test_short.pcd": {
"hash": "54c0246dbb47c2bfb758941ea5e873a2",
"size": 1734
}
},
"preprocessing": {
Expand Down
213 changes: 212 additions & 1 deletion rimage/transform/parallel_projection.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@ import (
"math"

"github.com/golang/geo/r3"
"github.com/montanaflynn/stats"
"github.com/pkg/errors"

"go.viam.com/rdk/pointcloud"
"go.viam.com/rdk/rimage"
"go.viam.com/rdk/spatialmath"
)

// ParallelProjection to pointclouds are done in a naive way that don't take any camera parameters into account.
Expand Down Expand Up @@ -95,5 +97,214 @@ func (pp *ParallelProjection) PointCloudToRGBD(cloud pointcloud.PointCloud) (*ri

// ImagePointTo3DPoint takes the 2D pixel point and assumes that it represents the X,Y coordinate in mm as well.
func (pp *ParallelProjection) ImagePointTo3DPoint(pt image.Point, d rimage.Depth) (r3.Vector, error) {
return r3.Vector{float64(pt.X), float64(pt.Y), float64(d)}, nil
return r3.Vector{X: float64(pt.X), Y: float64(pt.Y), Z: float64(d)}, nil
}

// ParallelProjectionOntoXZWithRobotMarker allows the creation of a 2D projection of a pointcloud and robot
// position onto the XZ plane.
type ParallelProjectionOntoXZWithRobotMarker struct {
robotPose *spatialmath.Pose
}

const (
sigmaLevel = 7 // level of precision for stdev calculation (determined through experimentation)
imageHeight = 1080 // image height
imageWidth = 1080 // image width
missThreshold = 0.49 // probability limit below which the associated point is assumed to be free space
hitThreshold = 0.55 // probability limit above which the associated point is assumed to indicate an obstacle
pointRadius = 1 // radius of pointcloud point
robotMarkerRadius = 5 // radius of robot marker point
)

// PointCloudToRGBD creates an image of a pointcloud in the XZ plane, scaling the points to a standard image
This conversation was marked as resolved.
Show resolved Hide resolved
// size. It will also add a red marker to the map to represent the location of the robot. The returned depthMap
nicksanford marked this conversation as resolved.
Show resolved Hide resolved
// is unused and so will always be nil.
func (ppRM *ParallelProjectionOntoXZWithRobotMarker) PointCloudToRGBD(cloud pointcloud.PointCloud,
) (*rimage.Image, *rimage.DepthMap, error) {
nicksanford marked this conversation as resolved.
Show resolved Hide resolved
meta := cloud.MetaData()

Copy link
Member

@nicksanford nicksanford Jan 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that one of the most likely reasons for stats.Mean & other aggregate math functions to return an error is for the slice passed in to be of length 0, which would only happen if the pointcloud had 0 elements it: In order to return more meaningful errors to end users, would it make sense to check the pointcloud is not empty before proceeding past this point & if so, returning an error, blank image, or whatever else is appropriate?

Otherwise the end user would get an error about math errors, which will probably be difficult for the to parse & infer that it is due to the fact that their slam algo returned an empty pointcloud.

if cloud.Size() == 0 {
return nil, nil, errors.New("projection point cloud is empty")
}

meanStdevX, meanStdevZ, err := calculatePointCloudMeanAndStdevXZ(cloud)
if err != nil {
return nil, nil, err
}

maxX := math.Min(meanStdevX.mean+float64(sigmaLevel)*meanStdevZ.stdev, meta.MaxX)
minX := math.Max(meanStdevX.mean-float64(sigmaLevel)*meanStdevZ.stdev, meta.MinX)
maxZ := math.Min(meanStdevZ.mean+float64(sigmaLevel)*meanStdevZ.stdev, meta.MaxZ)
minZ := math.Max(meanStdevZ.mean-float64(sigmaLevel)*meanStdevZ.stdev, meta.MinZ)

// Change the max and min values to ensure the robot marker can be represented in the output image
var robotMarker spatialmath.Pose
if ppRM.robotPose != nil {
This conversation was marked as resolved.
Show resolved Hide resolved
robotMarker = *ppRM.robotPose
maxX = math.Max(maxX, robotMarker.Point().X)
minX = math.Min(minX, robotMarker.Point().X)
maxZ = math.Max(maxZ, robotMarker.Point().Z)
minZ = math.Min(minZ, robotMarker.Point().Z)
}

// Calculate the scale factors
scaleFactor := calculateScaleFactor(maxX-minX, maxZ-minZ)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the scale factor is 0 this will return an image which is black right? Is that ever useful to an end user or would it be better to return an error in this case?


// Add points in the pointcloud to a new image
var pointColor rimage.Color
im := rimage.NewImage(imageWidth, imageHeight)
cloud.Iterate(0, 0, func(pt r3.Vector, data pointcloud.Data) bool {
x := int(math.Round((pt.X - minX) * scaleFactor))
nicksanford marked this conversation as resolved.
Show resolved Hide resolved
y := int(math.Round((pt.Z - minZ) * scaleFactor))

// Adds a point to an image using the value to define the color. If no value is available,
// the default color of white is used.
if x >= 0 && x < imageWidth && y >= 0 && y < imageHeight {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] could we please call x >= 0 && x < imageWidth && y >= 0 && y < imageHeight a variable like: pointWithinImageStandardDeviation so it is clear that this is where we are filtering out points which fall outside of the image borders as they are not within the standard deviation?

pointColor, err = getColorFromProbabilityValue(data)
if err != nil {
return false
}
im.Circle(image.Point{X: x, Y: y}, pointRadius, pointColor)
}
return true
})

if err != nil {
return nil, nil, err
}

// Add a red robot marker to the image
if ppRM.robotPose != nil {
x := int(math.Round((robotMarker.Point().X - minX) * scaleFactor))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the robot marker is outside of the standard deviation does this still do the right thing?

y := int(math.Round((robotMarker.Point().Z - minZ) * scaleFactor))
robotMarkerColor := rimage.NewColor(255, 0, 0)
im.Circle(image.Point{X: x, Y: y}, robotMarkerRadius, robotMarkerColor)
}
return im, nil, nil
}

// RGBDToPointCloud is unimplemented and will produce an error.
func (ppRM *ParallelProjectionOntoXZWithRobotMarker) RGBDToPointCloud(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we foresee ever truly implementing these other 2 interface methods on *ParallelProjectionOntoXZWithRobotMarker?

If not, why do we need to implement this interface at all?

As a user I would find it very confusing to have an interface instance which implements 2/3rds of an interface in name only.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The benefit of using an interface here is that any function (say in vision) that can use a Projector will ca able to inherently use this ParallelProjectionOntoXZwithRobotMarker as well.

Example

func display_my_points(Projector) {
}
pp := &ParallelProjection{}
display_my_points(pp)
ppRM := NewParallelProjectionOntoXZwithRobotMarker()
display_my_points(pp)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I guess I'm used to interface satisfaction being an all or nothing thing where, only implementing some of the interface requirements may yield unpredictable or incomplete functionality. I wasn't sure if that was the case here & if it is the case if we are ok with it as we plan on implementing these functions later.

img *rimage.Image,
dm *rimage.DepthMap,
crop ...image.Rectangle,
) (pointcloud.PointCloud, error) {
return nil, errors.New("converting an RGB image to Pointcloud is currently unimplemented for this projection")
}

// ImagePointTo3DPoint is unimplemented and will produce an error.
func (ppRM *ParallelProjectionOntoXZWithRobotMarker) ImagePointTo3DPoint(pt image.Point, d rimage.Depth) (r3.Vector, error) {
return r3.Vector{}, errors.New("converting an image point to a 3D point is currently unimplemented for this projection")
}

// getColorFromProbabilityValue returns an RGB color value based on the probability value and defined hit and miss
// thresholds
// TODO (RSDK-1705): Once probability values are available, a temporary algorithm is being used based on Cartographer's method
This conversation was marked as resolved.
Show resolved Hide resolved
// of painting images. Currently this function will return a shade of green if the probability is above the hit threshold and
// a shade of blue if it is below the miss threshold. These shades will be more distinct the further from the threshold they are.
func getColorFromProbabilityValue(d pointcloud.Data) (rimage.Color, error) {
var r, g, b uint8

if d == nil {
return rimage.NewColor(0, 0, 0), errors.New("data received was null")
}

if !d.HasValue() {
return rimage.NewColor(255, 255, 255), nil
}

if d.Value() > 100 || d.Value() < 0 {
return rimage.NewColor(0, 0, 0),
errors.Errorf("received a value of %v which is outside the range (0 - 100) representing probabilities", d.Value())
}

prob := float64(d.Value()) / 100.

switch {
case prob < missThreshold:
b = uint8(255 * ((missThreshold - prob) / (hitThreshold - 0)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Why are we subtracting 0 from hitThreshold? Isn't this a no op?

  2. Also, are these shades intended to mean anything? I.e. does more green mean something than less green, or are we just trying to make different probabilities in the 3 different buckets distinct from each other? If more green / less green or more glue / less blue is intended to mean something in the current form, can you please add that intent to the comment?

case prob > hitThreshold:
g = uint8(255 * ((prob - hitThreshold) / (1 - missThreshold)))
default:
b = uint8(255 * ((missThreshold - prob) / (hitThreshold - 0)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we subtracting 0 from hitThreshold? Isn't this a no op?

g = uint8(255 * ((prob - hitThreshold) / (1 - missThreshold)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't prob - hitThreshold result in a negative number here? Won't that result in a panic?

}

return rimage.NewColor(r, g, b), nil
}

// NewParallelProjectionOntoXZWithRobotMarker creates a new ParallelProjectionOntoXZWithRobotMarker with the given
// robot pose.
func NewParallelProjectionOntoXZWithRobotMarker(rp *spatialmath.Pose) ParallelProjectionOntoXZWithRobotMarker {
return ParallelProjectionOntoXZWithRobotMarker{robotPose: rp}
}

// Struct containing the mean and stdev.
type meanStdev struct {
mean float64
stdev float64
}

// Calculates the mean and standard deviation of the X and Z coordinates stored in the point cloud.
func calculatePointCloudMeanAndStdevXZ(cloud pointcloud.PointCloud) (meanStdev, meanStdev, error) {
var X, Z []float64
var x, z meanStdev

cloud.Iterate(0, 0, func(pt r3.Vector, data pointcloud.Data) bool {
X = append(X, pt.X)
Z = append(Z, pt.Z)
return true
})

meanX, err := safeMath(stats.Mean(X))
if err != nil {
return x, z, errors.Wrap(err, "unable to calculate mean of X values on given point cloud")
}
x.mean = meanX

stdevX, err := safeMath(stats.StandardDeviation(X))
if err != nil {
return x, z, errors.Wrap(err, "unable to calculate stdev of Z values on given point cloud")
}
x.stdev = stdevX

meanZ, err := safeMath(stats.Mean(Z))
if err != nil {
return x, z, errors.Wrap(err, "unable to calculate mean of Z values on given point cloud")
}
z.mean = meanZ

stdevZ, err := safeMath(stats.StandardDeviation(Z))
if err != nil {
return x, z, errors.Wrap(err, "unable to calculate stdev of Z values on given point cloud")
}
z.stdev = stdevZ

return x, z, nil
}

// Calculates the scaling factor needed to fit the projected pointcloud to the desired image size, cropping it
// if needed based on the mean and standard deviation of the X and Z coordinates.
func calculateScaleFactor(xRange, zRange float64) float64 {
var scaleFactor float64
if xRange != 0 || zRange != 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this allow for divide by zero errors, which will cause a panic?
e.g. xRange == 10 zRange == 0

widthScaleFactor := float64(imageWidth-1) / xRange
heightScaleFactor := float64(imageHeight-1) / zRange
scaleFactor = math.Min(widthScaleFactor, heightScaleFactor)
}
return scaleFactor
}

// Errors out if overflow has occurred in the given variable or if it is NaN.
func safeMath(v float64, err error) (float64, error) {
if err != nil {
return 0, err
}
switch {
case math.IsInf(v, 0):
return 0, errors.New("overflow detected")
case math.IsNaN(v):
return 0, errors.New("NaN detected")
}
return v, nil
}
Loading