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

Adding a previously removed index results in unexpected behavior. #142

Open
YashBansod opened this issue Mar 20, 2021 · 3 comments
Open
Labels

Comments

@YashBansod
Copy link

I tried to add the removed index at

size_t removePointIndex = N - 1;
index.removePoint(removePointIndex);

by adding the following code:

cloud.pts[N-1] = { 0.5, 0.5, 0.5 };
cout << "N-1 point: (" << cloud.pts[N-1].x << ", " << cloud.pts[N-1].y << ", " << cloud.pts[N-1].z << ")" << endl;
index.addPoints(N-1, N-1);

This should result in the closest point at (0.5, 0.5, 0.5) with a distance value of 0.
However, the code results in a different value (based on the original random closest point).

This behavior only happens if I try to re-add a previously removed index.
Can someone explain why this is happening? Is this a Bug?

@jlblancoc jlblancoc added the bug label Mar 22, 2021
@jlblancoc
Copy link
Owner

It seems so. Thanks for reporting, we'll try to address it, or if in the meanwhile you find a patch, PRs are welcome! ;-)

@Pikecillo
Copy link
Contributor

Pikecillo commented Jun 8, 2022

The problem is that treeIndex[N-1] is not being updated on the re-add of the point after being set to -1 during the deletion.

A fix draft can be found in this PR #172

Thoughts on two points are welcomed (please comments on PR):

  1. treeIndex is now a hash map instead of a vector, so it's expected to be slower. A vector<bool> could be used instead of a hashmap to alleviate this but depending on how points are loaded in the index this may mean extra space. does any other alternative comes to mind folks?
  2. The semantic of the argument void removePoint(size_t idx) changed. Previously that index referred to the order in which points were added to the set, now it's the same index space as the arguments of function void addPoints(AccessorType start, AccessorType end) (in other words, previously if the first bunch of points added are addPoints(100, 200), and I wanted to delete point with index 100, I would have to do remove(0), currently I have to do remove(100) )

@jlblancoc
Copy link
Owner

Oh, I saw this after the PR :-)
Ok, further comments on the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants