-
Notifications
You must be signed in to change notification settings - Fork 111
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
Add method which converts a pointcloud into an octree. #4608
Add method which converts a pointcloud into an octree. #4608
Conversation
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.
worth adding a test for this?
@JohnN193 I will add :) |
pointcloud/pointcloud_utils.go
Outdated
@@ -91,3 +91,22 @@ func StatisticalOutlierFilter(meanK int, stdDevThresh float64) (func(PointCloud) | |||
} | |||
return filterFunc, nil | |||
} | |||
|
|||
// PointCloudToBasicOctree takes a pointcloud object and converts it into an octree. | |||
func PointCloudToBasicOctree(cloud PointCloud) (*BasicOctree, error) { |
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.
If cloud
is already a BasicOctree, we should return that directly instead of copying
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.
seems that is is not.
return utils.AssertType[*BasicOctree](cloud)
gives error
expected *pointcloud.BasicOctree but got *pointcloud.basicPointCloud
Unless, I am completely mis-understanding this comment.
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.
octrees still satify the pointcloud interface iirc, so a user could call this function with something thats already an octree.
if we realize that we have an octree already, then we should just return it
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.
That would be because you passed in a basicPointCloud
.
You should not be returning AssertType directly, only if the err is nil.
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.
PointCloud is an interface type -- there are many structs that fulfill the PointCloud interface. You should do a type assertion to see if the cloud
passed is already of the BasicOctree Type, and return it if it is.
This isn't just for your use case, but a check so that all future users can ensure they're not accidently passing in a pointcloud that already an OctTree type
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.
if octTree, ok := cloud.(*BasicOctree); ok {
return octTree, nil
}
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.
LGTM
…e which returns an octree populated with either positive or negative data
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.
LGTM
We already have the ReadPCDToBasicOctree method. However in order to use it on a pointcloud.PointCloud object, a user first needs to convert it into a PCD file and only then are they able to use the mentioned method. This is not an optimal route.
The goal of this PR is to add a function which removes the step of converting a pointcloud into a file.