[Data-565] Fix for flaky join pointcloud naive test #1505
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
bhaney
requested review from
stevebriskin
and removed request for
stevebriskin
bhaney
force-pushed
the
data-565-flaky-join-pointcloud
branch
from
0066921
to
1605092
Compare
bhaney
commented
Oct 18, 2022
pointcloud/merging.go
Outdated
Comment on lines
104
to
117
| // one last read to flush out any potential last data | ||
| lastBatches := len(finalPoints) | ||
| logger.Debugf("number of last batches: %d", lastBatches) | ||
| for i := 0; i < lastBatches; i++ { | ||
| lastPoints := <-finalPoints | ||
| logger.Debugf("number of points in batch %d: %d", i, len(lastPoints)) | ||
| for _, p := range lastPoints { | ||
| myErr := pcTo.Set(p.P, p.D) | ||
| if myErr != nil { | ||
| err = myErr | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is the fix, essentially
bhaney
force-pushed
the
data-565-flaky-join-pointcloud
branch
from
a81421d
to
3679fd6
Compare
bhaney
changed the title
|
edaniels
approved these changes
Oct 19, 2022
| var pcTo PointCloud | ||
| var err error | ||
|
|
||
| dataLastTime := false // if there was data in the channel in the previous loop, continue reading. |
There was a problem hiding this comment.
this may be a naive question and can be discussed later, but how fast is this code compared to a simpler version. This feels very complicated.
There was a problem hiding this comment.
I will make a ticket for simplifying this merging function.
| var err error | ||
|
|
||
| dataLastTime := false // if there was data in the channel in the previous loop, continue reading. | ||
| for dataLastTime || atomic.LoadInt32(&activeReaders) > 0 { |
There was a problem hiding this comment.
why is this looping like this versus waiting on a channel and/or waitgroup?
There was a problem hiding this comment.
From what i understand, this code generates a thread for each pointcloud source, splits each pointcloud into 8 subset pointclouds and puts each into a seperate thread, and then does the necessary transforms before dumping all the points into a final point cloud.
So there are nCamera*8 threads all feeding into one channel. Then there is this loop, which is reading from that channel into this final point cloud. Is it waiting for all threads to finish pushing data in the channel before it exits the for loop.
| for _, p := range ps { | ||
| if pcTo == nil { | ||
| if p.D == nil { | ||
| pcTo = NewAppendOnlyOnlyPointsPointCloud(len(cloudFuncs) * 640 * 800) |
There was a problem hiding this comment.
what's the 640*800 for? should it be a const?
There was a problem hiding this comment.
To answer your question, this code was written in a rush to fulfill fast pointcloud merging for a demo. 640*800 is the resolution of the cubeEye cameras, so this was a pre-allocation for the number of points generated by the system of cubeEye cameras
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.
The bug was an edge-case when reading the batches of points from a channel into the final point cloud would not read all the points in the channel.
The loop would continue reading data into the final point cloud if
dataLastTime || atomic.LoadInt32(&activeReaders) > 0If there was no data in the channel, then the loop would wait 5 ms and then would set
dataLastTime = false.There was a situation where
dataListTimewas false, butatomic.LoadInt32(&activeReaders)had become 0 only within those 5 ms whendataLastTimewas being set to false, meaning there was still some last minute data in the channel. The solution is to do one last read to make sure all of the data is out of the pipeline.