RSDK-1072 Read PCD to Octree #1768
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
viambot
added
the
safe to test
viambot
added
safe to test
nicksanford
reviewed
Jan 17, 2023
nicksanford
reviewed
Jan 17, 2023
nicksanford
reviewed
Jan 17, 2023
viambot
added
safe to test
viambot
added
safe to test
ghost
commented
Jan 17, 2023
viambot
added
safe to test
nicksanford
reviewed
Jan 18, 2023
nicksanford
reviewed
Jan 18, 2023
|
|
||
| basicOct, err := ReadPCDToBasicOctree(strings.NewReader(gotPCD)) | ||
| test.That(t, err, test.ShouldBeNil) | ||
| test.That(t, basicOct.Size(), test.ShouldEqual, 3) |
There was a problem hiding this comment.
Also, do we want to have any more comprehensive testing that the conversion from PCD to Octree is lossless?
Would it be possible to write a test that confirms that (at a high level):
ToPCD(ToOctree(PCD)) == PCDToPointcloud(PCD)'s methods behave in the exact same way asToOctree(PCD)? for a givenPCD
There was a problem hiding this comment.
I have a note on line 174 in basic_octree_utils.go to do so once we decide which way to go (one way doesnt require this function to exist)
There was a problem hiding this comment.
If I were implementing this, I'd like to have tests that prove 1 & 2 in the PR that adds the functionality to go between PCD & Octree to ensure they are correct.
Could we add them before merging this PR?
There was a problem hiding this comment.
Can we please get tests for the behavior of https://github.com/viamrobotics/rdk/pull/1768/files#diff-e01dea69a2aaa9209cb5deeb1c4569f99ef8c51262c69799a0cec3a79d7434ebR543 ?
PCDAscii and PCDCompressed and an unexpected type?
There was a problem hiding this comment.
Never mind, I now see that these cases are covered in testASCIIRoundTrip & testBinaryRoundTrip
nicksanford
requested changes
Jan 19, 2023
| bOct.logger.Infof("%v %e %e %e - %v | Children: %v Side: %v Size: %v\n", s, | ||
| func printBasicOctree(bOct *BasicOctree, s string) { | ||
| //nolint:forbidigo | ||
| fmt.Printf("%v %e %e %e - %v | Children: %v Side: %v Size: %v\n", s, |
There was a problem hiding this comment.
why is this Printf needed, is there no global logger? also, why the//nolint:unsued there?
pointcloud/basic_octree_utils.go
Outdated
| } | ||
|
|
||
| return ok | ||
| } | ||
|
|
||
| // ConvertPointCloudToBasicOctree converts a pointcloud into a octree pointcloud representation. | ||
| func ConvertPointCloudToBasicOctree(cloud PointCloud) (PointCloud, error) { |
There was a problem hiding this comment.
I think if we end up having multiple octree types, we could just create another function which returns a that octree type. If the caller has opinions on which octree representation to use, they can call the function which gives them that flavor of octree.
Returning a PointCloud implies to me that returning anything that implements the PointCloud interface would be equally valid, which I don't think is the case for this function.
|
|
||
| basicOct, err := ReadPCDToBasicOctree(strings.NewReader(gotPCD)) | ||
| test.That(t, err, test.ShouldBeNil) | ||
| test.That(t, basicOct.Size(), test.ShouldEqual, 3) |
There was a problem hiding this comment.
If I were implementing this, I'd like to have tests that prove 1 & 2 in the PR that adds the functionality to go between PCD & Octree to ensure they are correct.
Could we add them before merging this PR?
ghost
marked this pull request as ready for review
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
ghost
assigned
nicksanford
reviewed
Jan 25, 2023
nicksanford
reviewed
Jan 25, 2023
|
only blocking feedback remaining is test cases for all the different PCD formats ReadPCDToOctree supports & doesn't support, so we can have at least high level tests of all the new code paths which are being added. |
viambot
added
safe to test
bhaney
approved these changes
Jan 25, 2023
pointcloud/basic_octree_utils.go
Outdated
Comment on lines
176
to
178
| X: meta.MinX + (meta.MaxX-meta.MinX)/2, | ||
| Y: meta.MinY + (meta.MaxY-meta.MinY)/2, | ||
| Z: meta.MinZ + (meta.MaxZ-meta.MinZ)/2, |
There was a problem hiding this comment.
[opt] This can also be expressed as (meta.MinX + meta.MaxX)/2 but it's not like an extra operation makes a big difference.
pointcloud/pointcloud_file.go
Outdated
Comment on lines
549
to
559
| switch header.data { | ||
| case PCDAscii: | ||
| meta, err = getPCDMetaDataASCII(*in, header) | ||
| case PCDBinary: | ||
| meta, err = getPCDMetaDataBinary(*in, header) | ||
| case PCDCompressed: | ||
| // return readPCDCompressed(in, header) | ||
| return nil, errors.New("compressed pcd not yet supported") | ||
| default: | ||
| return nil, fmt.Errorf("unsupported pcd data type %v", header.data) | ||
| } |
There was a problem hiding this comment.
It feels like this switch statement could be within the function more general called getPCDMetaData
pointcloud/pointcloud_file.go
Outdated
Comment on lines
682
to
693
| func getPCDMetaDataBinary(in bufio.Reader, header pcdHeader) (MetaData, error) { | ||
| meta := NewMetaData() | ||
| for i := 0; i < int(header.points); i++ { | ||
| pd, err := extractPCDPointBinary(&in, header) | ||
| if err != nil { | ||
| return MetaData{}, err | ||
| } | ||
|
|
||
| meta.Merge(pd.P, pd.D) | ||
| } | ||
| return meta, nil | ||
| } |
There was a problem hiding this comment.
yes, I think combining the getPCDMetaData functions into one will make all right sense
viambot
added
safe to test
viambot
added
safe to test
|
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements the
ReadPCDToOctreefunction which allows the creation of an octree from a pcd file/buffer. It also adds octrees to thepointcloudpackage, as the rescoping of map representation removed the formerly requiredMarshalerfunction allowing octrees to implement thepointcloudinterface exactly. This change over also allows us to take advantage of the pcd reader functions where are unexposed.The
ReadPCDToBasicOctreecurrent is implemented by creating a basic pointcloud which allows us to obtain the requried metadata for an octree (center and sideLength from the max and min X,Y and Z values and then converting this pointcloud to a basicOctree. This results in a call toReadPCDas well as a call to the newConvertPointCloudToOctreefunction. The ConvertPointCloudToOctree returns a Pointcloud interface that can be cast into aBasicOctreeand any other implementation of octree that should be created in the future, whileReadPCDToBasicOctreereturns the specificBasicOctreestructSeveral changes were made, including but not limited to:
BasicOctreesNewtoNewBasicOctreeto reduce confusion asNewalready exists in the pointcloud packageBasicOctree(basicOctree->BasicOctree)Additional information can be found in the comment below
JIRA Ticket: RSDK-1072