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

Removing items requires a bounding box #95

Open
DanPen opened this issue Aug 13, 2019 · 5 comments
Open

Removing items requires a bounding box #95

DanPen opened this issue Aug 13, 2019 · 5 comments

Comments

@DanPen
Copy link

DanPen commented Aug 13, 2019

tree.remove({ id: '1234' }, (a, b) => {
    console.log(a, b);
    return a.id === b.id;
});

I know the callback is never called because I never see the log statement.

@DanPen
Copy link
Author

DanPen commented Aug 14, 2019

After experimenting a little, it seems that remove doesn't work without a matching bbox.

DanPen@845e8bd

@eeeeenchanted
Copy link

eeeeenchanted commented Aug 15, 2019

We experienced the same issue when removing, updating, and re-inserting the same item quickly.

We looked at the code and the issue is that contains(node, bbox) is returning false.

One thing that got us confused at first is why there needs to be a spatial search prior to removing an element given that the docs say elements are removed by reference. Would it be possible/make sense to have a Map() and only reference elements in the tree? This way removing would be dead simple, e.g., map.delete(item). Anyway, just a random idea.

@mourner mourner changed the title Removing items by id doesn't work Removing items requires a bounding box Nov 6, 2019
@thisredone
Copy link

Actually probably the simplest way would be to keep a reference to parent for all nodes and iitems. This also helps greatly with item updates and reinsertions.
I have rewritten this library (https://github.com/thisredone/rbush/tree/dev) to support fast updates and removals; providing predicate function similar to #74; raycasting with stack-based, ordered ray traversal algorithm from this paper; finding collisions with the help of box-intersect for cross-leaf overlaps; and polling almost everywhere to avoid pressuring GC.
It's a very opinionated fork but it's there if you need any of those things.

@photonstorm
Copy link

@thisredone thank you for this fork, this is exactly what I need right now! Testing it immediately :)

@mizuiren
Copy link

mizuiren commented Mar 30, 2023

Change the code if (!goingUp && !node.leaf && contains(node, bbox)) { // go down to if (!goingUp && !node.leaf && (item.id || contains(node, bbox))) { // go down can solve the problem, but got heavy performance, due to missing range lookup. Suggested use of reference deletion!

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

No branches or pull requests

6 participants