Repository navigation
Fix duplicate-entry growth on removePoint/addPoints re-add cycles - #285
Merged
Merged
Conversation
Re-adding a previously-removed point now reactivates the existing entry in place instead of inserting a duplicate. removePoint is a lazy deletion (treeIndex_[idx]=-1) that leaves the point physically present in its sub-tree's vAcc_. Re-adding it via addPoints re-inserted it: incrementing pointCount_, growing treeIndex_, and pushing the index into vAcc_ again. Repeated remove/add cycles therefore caused unbounded growth of the internal vectors, duplicate results in radius/knn searches, and could eventually overflow treeCount_ (the assertion fixed in PR #283 was an earlier casualty of the same growth). treeIndex_'s numeric value is only ever compared to -1, so reactivation only needs to restore a valid tree mark. removedPoints_ becomes a map from index to the sub-tree that still holds it; removePoint records it (and now guards double-removal); addPoints reactivates in place; treeIndex_ is resized incrementally instead of by the batch count. Adds test kdtree.dynamic_readd_no_duplicates (fails before, passes after). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
social4hyq
pushed a commit
to social4hyq/homebrew-core
that referenced
this pull request
Sep 20, 2026
nanoflann 1.10.0 Created-by: HarmonybrewBot Commit-by: HarmonybrewBot Merged-by: HarmonybrewBot Description: Created by `brew bump` --- Created with `brew bump-formula-pr`.<details> <summary>release notes</summary> <pre>## What's Changed * Fix an invalid read. by @drwells in jlblancoc/nanoflann#282 * Fix divideTree assertion failure with KDTreeSingleIndexDynamicAdaptor removePoint/addPoints by @nh2 in jlblancoc/nanoflann#283 * Fix duplicate-entry growth on removePoint/addPoints re-add cycles by @jlblancoc in jlblancoc/nanoflann#285 * Fix loadIndex reading past EOF for empty indices (#286) by @jlblancoc in jlblancoc/nanoflann#287 * fix flaky kdtree.L1_vs_bruteforce_rknn test by @jschueller in jlblancoc/nanoflann#288 * Fix KDTreeSingleIndexDynamicAdaptor_ saveIndex/loadIndex infinite recursion by @jlblancoc in jlblancoc/nanoflann#290 * Remove dead worst_dist branch from L1/L2_Adaptor::evalMetric by @jlblancoc in jlblancoc/nanoflann#291 * Fix three type/correctness bugs (operator=, First0Bit, findWithinBox) by @jlblancoc in jlblancoc/nanoflann#292 * Polish docs warnings attrs by @jlblancoc in jlblancoc/nanoflann#293 * Efficiency: remove middleSplit_ heap alloc (§6.2) + else-if in computeInitialDistances (§6.7) by @jlblancoc in jlblancoc/nanoflann#294 * saveIndex/loadIndex: add magic+version+type-size header, throw on mismatch by @jlblancoc in jlblancoc/nanoflann#295 * CI: extend tested C++ standards, and increase warning levels by @jlblancoc in jlblancoc/nanoflann#296 * Deduplicate addPoint, computeBoundingBox, and searchLevel by @jlblancoc in jlblancoc/nanoflann#297 * Minor internal cleanups: allocateBytes, has_flag(), noexcept by @jlblancoc in jlblancoc/nanoflann#298 * Centralize DIM dispatch in veclen() with if constexpr; widen nodiscard by @jlblancoc in jlblancoc/nanoflann#299 * Refactor tree builders and harden Eigen adaptor ownership by @jlblancoc in jlblancoc/nanoflann#300 * Hoist point-length computation out of search inner loops by @jlblancoc in jlblancoc/nanoflann#301 * Incremental self-balancing dynamic k-d tree index (sync + multithreaded) by @jlblancoc in jlblancoc/nanoflann#302 ## New Contributors * @drwells made their first contribution in jlblancoc/nanoflann#282 * @nh2 made their first contribution in jlblancoc/nanoflann#283 **Full Changelog**: https://github.com/jlblancoc/nanoflann/compare/v1.9.0...v1.10.0</pre> <p>View the full release notes at <a href="https://github.com/jlblancoc/nanoflann/releases/tag/v1.10.0">https://github.com/jlblancoc/nanoflann/releases/tag/v1.10.0</a>.</p> </details> <hr> See merge request: Harmonybrew/homebrew-core!10859
This file contains hidden or 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
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.
Summary
Re-adding a previously-removed point now reactivates the existing entry in place instead of inserting a duplicate.
removePointis a lazy deletion (treeIndex_[idx] = -1) that leaves the point physically present in its sub-tree'svAcc_. Re-adding it viaaddPointsre-inserted it — incrementingpointCount_, growingtreeIndex_, and pushing the index intovAcc_again. Repeated remove/add cycles therefore caused unbounded growth of the internal vectors, duplicate results in radius/knn searches, and could eventually overflowtreeCount_(the assertion fixed in #283 was an earlier casualty of the same growth).Key observation:
treeIndex_'s numeric value is only ever compared to-1, so reactivation just needs to restore a valid tree mark — no re-insertion or rebuild.Changes
removedPoints_:unordered_set<int>→unordered_map<IndexType,int>(index → sub-tree that still holds it).removePoint: records the holding tree before marking-1; guards against double-removal.addPoints: if the index is currently removed, reactivate in place and skip re-insertion; the merge loop keeps a moved tombstone's stored tree current;treeIndex_is resized incrementally instead of by the batch count.kdtree.dynamic_readd_no_duplicates: asserts stored-entry count stays at N and a full-coverage radius search returns each index exactly once. Fails before this change (120 vs 20), passes after.Verification
add_and_remove_pointsand Fix divideTree assertion failure with KDTreeSingleIndexDynamicAdaptor removePoint/addPoints #283'sdynamic_repeated_remove_readd.dynamic_pointcloud_example(forward-only 1M-point usage) unchanged.clang-format-14applied (idempotent).Fixes #284.