[DATA-420 & DATA-421] Octree Implementation of Basic Functionality Set, At, MetaData and Size #1596
Conversation
viambot
added
the
safe to test
viambot
added
safe to test
| test.That(t, checkPointPlacement(center, side, r3.Vector{X: -1000, Y: 0, Z: 0}), test.ShouldBeFalse) | ||
| } | ||
|
|
||
| // Helper functions for visualizing octree during testing |
There was a problem hiding this comment.
Functions here are used for test creation/debugging in the future they may be fully integrated into testing but for now they were made unused once test creation was complete
pointcloud/octree/basic_utils.go
Outdated
| } | ||
|
|
||
| // Checks that point should be inside octree based on octree center and defined side length. | ||
| func checkPointPlacement(center r3.Vector, sideLength float64, p r3.Vector) bool { |
There was a problem hiding this comment.
See second open question regarding check
pointcloud/octree/basic.go
Outdated
| } | ||
|
|
||
| // OctreeMetaData returns the octree metadata. | ||
| func (octree *basicOctree) OctreeMetaData() MetaData { |
There was a problem hiding this comment.
Octree Metadata is a superset of Pointcloud metadata. The interface for pointcloud requires the return of pointcloud metadata. To return the full Octree metadata a new function OctreeMetaData is introduced.
Note alternatives include:
- Only storing pc.MetaData and doing calculations of the additional necessary octree metadata before serialization (calls to Size, center... etc)
pointcloud/octree/basic.go
Outdated
|
|
||
| // At traverses the octree to see in a point exists at the specified location. If a point does exist, its data is | ||
| // returned along with true, if one is not then no data is returned and the boolean is returned false. | ||
| func (octree *basicOctree) At(x, y, z float64) (pc.Data, bool) { |
There was a problem hiding this comment.
As discussed, current implementation is for exact match to return true and the point data. nearest neighbor has not been implemented
viambot
added
safe to test
ghost
marked this pull request as ready for review
|
Can you make the title for the PR title more descriptive and remove the date from it? The date is already recorded inherently in the commits & PR and is not helpful in a PR title. It might be helpful to add more to the title than "Octree Implementation" because I suspect there will be more PRs following that implement Octrees. If not then ignore this last point. |
There was a problem hiding this comment.
pretty much looks good to me, only real question is if this should live in pointcloud. In the tech spec we have it at go.viam.com/rdk/octree, but I also see why it could live in pointcloud since its a variation of that
ghost
changed the title
I spoke to Bijan about that and he recommended it live in pointcloud. Happy to move it if we feel it is better suited to primary level in rdk |
viambot
added
safe to test
…eeSetAndNew_11142022
viambot
added
safe to test
What are the arguments for this recommendation? I think we should stick to the tech spec unless there are good reasons not to |
viambot
added
safe to test
viambot
added
safe to test
makes sense, I think once the rest of octtree is setup we can bring this question back up, and for now we can just follow the spec |
There was a problem hiding this comment.
Great work so far! A major concern is that the octree assumes a fixed size and does not accept points outside its current bounds. Instead of doing that, we can grow the octree in size so it can be more adaptive, and we don't have to worry about predicting the size of the space it'll map out.
viambot
added
safe to test
viambot
added
safe to test
…eeSetAndNew_11142022
viambot
added
safe to test
viambot
added
safe to test
| } | ||
|
|
||
| // Helper function that recursively checks a basic octree's structure and metadata. | ||
| func validateBasicOctree(t *testing.T, bOct *basicOctree, center r3.Vector, sideLength float64) int32 { |
…eeSetAndNew_11142022
viambot
added
safe to test
|
Summary: This PR is the initial push for the octree data structure that will be used by SLAM to store generated maps and ease the ingestion of them by other teams. It implements a basic octree data struct which abides by the Octree interface (which includes the Pointcloud functions as well as a marshaling function although it is not included in this PR).
The focus of this PR is the New(), At(), Set(), Metadata() and Size() functions although several additional helper functions can be found in
basic_utils.goAssociated JIRA Tickets: DATA-420 DATA-421
Open Questions:
newLeafNodeEmpty,newLeafNodeFilled,newInternalNodefunctions. Editing the data struct or accessing helper function to create improper nodes is not possible due to them not being exposed.