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

findAll( ) function #18

Closed
wants to merge 1 commit into from
Closed

findAll( ) function #18

wants to merge 1 commit into from

Conversation

lwthatcher
Copy link

Added a slight variation to the find( ) method by creating a findAll( ) method which returns all points within a specified radius.

This is based off of this example code.

The main difference however is that the code used in the example adds callbacks for searched and selected nodes, which is how it highlights explored nodes as well.

@mbostock mbostock self-assigned this Dec 13, 2016
@mbostock
Copy link
Member

This looks useful. A couple thoughts:

  • I wonder if there is a way to reduce the code duplication with quadtree.find. My guess is that it might be hard to do that without over-generalizing the implementation, but maybe.

  • The name quadtree.findAll feels a little too general, since there could be other ways of finding (or “filtering”?) that return more than one datum. In particular, intersecting the quadtree with another axis-aligned bounding box is a common use case given the popular interaction technique of brushing. There’s even the quadtree brushing example which rolls its own search implementation, but partly that’s because it also wants to record which quads were scanned for explanatory purposes. I suppose you don’t want to over-generalize though, for example, what if you want to find all points within a polygon? (You might compute the bounding box or bounding circle of the polygon first, and use that, though…)

@lwthatcher
Copy link
Author

Good points.

The name findAll does seem a little general, I think I like your suggestion of filter better...

As far as code duplication goes, that might be able to be resolved using some sort of dependency injection? I'm not sure how much that would affect performance, but the differences between routines could be passed in rather than hard coded. This way could possibly also support other search cases such as a bounding box, polygon, or possibly even a k-nearest neighbor search without too much duplication.

I'll take a look at it and see if I can come up with something.

@gka
Copy link

gka commented Jan 25, 2017

if I may add, a quadtree.findInRect function would be useful, too

@Amyantis
Copy link

Hi,
Would it be possible to merge this PR? Besides the too general name findAll, the function itself works as expected.

Thanks!

@Fil
Copy link
Member

Fil commented Jun 5, 2020

It's not clear to me why findAll should be based on find.

The crucial optimisation within find occurs when a point is found within radius of the target: we then adjust x0, x3, y0, y3 to that point's distance to the target, and thus eliminate lots of branches in our visiting tree.

But with findAll, we still have to visit all those quadrants that might contain an acceptable point within radius of the target, so this optimisation is not activated. Meaning that findAll is just a normal visit:

quadtree.visit(function(node, x1, y1, x2, y2) {
    if (!node.length) {
      do {
        const d = node.data,
          dx = +quadtree._x.call(null, d) - x,
          dy = +quadtree._y.call(null, d) - y;
        d.scanned = true;
        if (Math.hypot(dx, dy) < radius) accept(d);
      } while (node = node.next);
    }
    return x1 >= x + radius || y1 >= y + radius || x2 < x - radius || y2 < y - radius;
  });

See the demo: https://observablehq.com/d/ff2450667f13132f

@Fil Fil mentioned this pull request Jun 26, 2020
@Fil
Copy link
Member

Fil commented Jul 7, 2020

Superseded by #32

@Fil Fil closed this Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants