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

Fix octree sampling #540

Merged
merged 2 commits into from
Dec 15, 2023
Merged

Conversation

boxanm
Copy link
Collaborator

@boxanm boxanm commented Dec 4, 2023

I accidentally noticed this bug when unit testing an NDT-inspired octree sampling method.
The current octree sampling implementation takes into account the situation when the indexed point was already processed; see this example:

std::size_t j = d;
//retrieve index from lookup table if sampling in already switched element
if(std::size_t(d)<idx)
   j = mapidx[d];

However, we don't know whether mapidx[d] doesn't again point to an index that was also processed. What actually needs to be done, is to propagate all the changes that were made on all previously used indexes of mapidx. So in this PR, I replaced the above code example and similar by

std::size_t j = d;
//retrieve index from lookup table if sampling in already switched element
while(j < idx)
   j = mapidx[j];

I haven't had time to check other Octree implementations and see how they deal with this situation.
Naturally, the use of the while cycle slows down the execution. There's probably a more clever solution using pointers?

@boxanm boxanm added the Bug label Dec 4, 2023
@boxanm boxanm changed the base branch from master to develop December 4, 2023 22:37
@simonpierredeschenes
Copy link
Collaborator

simonpierredeschenes commented Dec 12, 2023

@boxanm Could you please confirm that this is the same issue as #454?

@boxanm
Copy link
Collaborator Author

boxanm commented Dec 13, 2023

The bug I described shouldn't make random parts of the cloud disappear. Instead, the bug causes incorrect sampling inside individual cells. So points, that should be sampled are preserved and the output contains more points, than the number of cells in the octree. Therefore I don't think #454 is related.

@boxanm boxanm merged commit c12d141 into norlab-ulaval:develop Dec 15, 2023
5 checks passed
@boxanm boxanm deleted the fix_octree_sampling branch December 15, 2023 22:33
@RedLeader962 RedLeader962 mentioned this pull request Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants