Skip to content

When dividing the rectangle, now its child-points will be moved to the new child-rectangles #54

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

Closed
wants to merge 2 commits into from

Conversation

uWynell
Copy link

@uWynell uWynell commented Feb 27, 2021

I may be wrong and would like to hear objective criticism about this pull request

Why?

Reason number 1

Imagine a QuadTree with a capacity of 1.
When you insert a point, everything's fine, right?
image
But if you add a second point, the tree will probably look like this:
image
Because the red point is counted as a child of the top-right rectangle, while the black point is still counted as a child of the big main rectangle. So that's you would visually see these two points in the same rectangle. Remember that the capacity of this tree is 1. For me, it looks pretty wrong.

Reason number 2

Now in the query() function, the loop runs only on the smallest squares (that are not divided), I believe this increases the searching performance.

@uWynell
Copy link
Author

uWynell commented Feb 27, 2021

The tests need to be fixed too 😨

@crhallberg
Copy link
Collaborator

Hey! Thank you for the contribution! We had some discussion about this a few years but never really reached a conclusion.

The basic points are:

  • We need to account for an edge case where adding points in identical or near identical positions can cause an infinite loop.
  • Whether or not the added complexity means this repository is less useful as a teaching tool, which is its primary function.

Either way I think your implementation is clean and easy to understand! While you're figuring out the issues with the tests, I'd recommend expanding this line to match the code style and be a little less of a magic one-liner. Thank you!

Base automatically changed from master to main March 2, 2021 18:33
@crhallberg
Copy link
Collaborator

Covered by #61.

@crhallberg crhallberg closed this Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants