improbable-eng / phtree-cpp

PH-Tree C++ implementation by Improbable.
Apache License 2.0
22 stars 15 forks source link

filter: Add FilterParaboloid #19

Open ctbur opened 2 years ago

ctbur commented 2 years ago

Changes

Add the FilterParaboloid filter than can filter points for an axis-aligned paraboloid given a vertex and a scale factor. Useful for range-checks on projectile with a fixed launch speed and arbitrary launch direction.

Verification

Added a test case.

improbable-til commented 2 years ago

I am not really sure about this PR. Unlike the sphere filter (https://github.com/improbable-eng/phtree-cpp/pull/16), this seems like very specialised use case that adds quite a bit of code. Do I understand correctly that this represents a paraboloid (like a roundish cone) rather than a parabolic trajectory? Some questions:

ctbur commented 2 years ago

My use case for it is to check whether a given entity can hit a target with a projectile launched at a fixed speed in any direction. For this I need the volume that represents all points where it is possible to launch the projectile to, i.e., the envelope over all possible projectile trajectories. In 3D this is a paraboloid. There is a better explanation in the Wikipedia article.

I don't quite understand the constraint of "fixed launch speed", where does the speed play a role and why does it need to fixed, and fixed to what value?

It's fixed at the maximum speed the entity can launch the projectile at. If you want to maximize range and minimize time then the launch speed is always the maximum speed.

Do you have any benchmarks that demonstrate that it actually improves performance for collision detection?

No there is no performance benchmark. This is to make the query possible in the first place. The alternative would be to approximate the paraboloid with either a cylinder, which would also have to be implemented, or an AABB, and then removing the points not in the paraboloid from the query results.

improbable-til commented 2 years ago

Ah, I had misunderstood the purpose, I thought you were simulating the shape of a projectile, rather than the boundary shape of it's potential flight paths.

Performance: The naive way to use the tree is to use a query bounding box (or a sphere, after your other contribution) and then filter all results with a is-inside-paraboloid check. Your PR adds a filter that moves the is-inside-paraboloid check into the PH-tree. The problem is that doing this check inside the PH-tree comes at a cost.

If you filter externally (without this PR), you have to filter every entry that is inside a given box. If you filter internally, you may have to filter fewer entries (it could still be a similar amount of entries). However, you also have to filter every node. If filtering the additional nodes internally is more expensive than what we avoid by a smaller query shape, then the internal filter does actually slow down query performance.

It basically depends on two factors:

Maybe to understand why I am a bit reluctant (performance aside): if we accept such an PR then we have to maintain the code for basically as long as this project exists, which may be (hopefully) many years. Obviously maintaining code (fixing bugs, adapting to C++20/23/..., fix warnings with new compilers, improve documentation, ...) is not for free, so any additional code should provide a worthwhile benefit for users. So, despite having worked for ~10 years with spatial indexes, this is probably the first time I heard of this paraboloid problem being used in this context. It sounds like a sensible problem + solution, but I want to make sure that it really has a strong benefit for users. Unfortunately I am not convinced yet. I also haven't seen such a paraboloid i any other spatial index yet. I am thinking about leaving it open, maybe wait whether we get other users to upvote it?

ctbur commented 2 years ago

Hi Til, sorry for the delay.

I didn't really consider performance to that level, I mostly added the filter because it's generally applicable and thus makes the library easy to use. Without a parabola you need to choose a finitely sized box appropriate for the specific application and then filter the points with the code in IsEntryValid.

How much smaller is the volume of the paraboloid compared to it's bounding box? This ratio determines how many entity-check you can expect to avoid with an internal filter (= the dedicated filter function in this PR)

Given the circular shape of the parabola it can be quite a difference. Using the formula from this Quora question, the volume of a parabola is (π/2)hr², while the enclosing box has a volume of 4hr², so the box' volume is 8/π ≈ 2.5 times larger. Of course, how relevant this is depends heavily on how the points are distributed.

How expensive is the filter function for nodes? If it is much more expensive than the bounding-box check then it becomes more likely that the additional node filter outweighs the reduced filter calls on entries.

Comparing IsEntryValid with IsNodeValid I wouldn't expect a huge difference in runtime on a modern architecture (2x or 3x, but not 10x). But yes, a benchmark would be needed.

if we accept such an PR then we have to maintain the code for basically as long as this project exists ... I am thinking about leaving it open, maybe wait whether we get other users to upvote it?

Yes, I get your point. My assumption was that projectile range checks would be a relatively common use-case. But if Improbable does not have a need for it then maybe it's not as common as I expected. We can wait and see if someone else could make use of it.

improbable-til commented 2 years ago

My assumption was that projectile range checks would be a relatively common use-case.

I think there are two points here:

Anyway, thanks again for putting your time and effort into this.

We can wait and see if someone else could make use of it.

@all Please like this if you want this to become part of the library.