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

filter: Add FilterSphere #16

Merged
merged 6 commits into from
Jan 14, 2022
Merged

Conversation

ctbur
Copy link
Contributor

@ctbur ctbur commented Jan 5, 2022

Changes

Add the FilterSphere filter than can filter points for a sphere given a center and a radius.

Unfortunately I had to use a scalar converter that needs to be hand-picked and match the multidimensional converter used in the tree. Please let me know how it can be improved.

Verification

Added a test case.

@improbable-til improbable-til self-assigned this Jan 6, 2022
@improbable-til
Copy link
Contributor

The normal converters can only convert PhPoint instances. I think it would be good if you could use the same converter that is used for the PhTree itself. I see two options:

  1. Extend one of the existing converters and add pre()/post() functions for scalars. This could then be used to also create the PhTree itself.
  2. Adapt the filter to work with PhPoints.
    • Convert the sphere center to an internal point
    • Use < / > to identify/create the closest corner point of the node to the sphere center. The result is an internal point. (It is safe to compare internal scalars, but it is not safe to add/substract/multiply/... them).
    • Convert the internal corner point to an external point to calculate the distance to the sphere center.

@ctbur
Copy link
Contributor Author

ctbur commented Jan 8, 2022

Thanks. Approach 2. worked and gave a much cleaner result. We actually just need to clamp the center point in the node's AABB to get the closest distance, so only comparisons are needed, no calculations.

@improbable-til
Copy link
Contributor

LGTM.
Nice contribution, thanks!

@improbable-til improbable-til merged commit 3e65a36 into improbable-eng:master Jan 14, 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