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

Add line-circle and circle-circle closest points functions; improve cylinder-cylinder collisions #71775

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rburing
Copy link
Member

@rburing rburing commented Jan 21, 2023

This PR implements Vranek's Fast and Accurate Circle-Circle and Circle-Line 3D Distance Computation with the following API:

line-circle_and_circle-circle_api

The line-circle implementation is based on the one in webots and the circle-circle implementation is based on the one in MinuteTorus, though both have been significantly modified. The original article is sketchy regarding the line-circle case; I've added details in comments in the code, and answering my question about the choice of bracketing triplet could improve the implementation.

To improve cylinder collisions still further (and implement cylinder-cylinder distance), disk-circle and segment-circle closest points functions should be implemented.

@lawnjelly
Copy link
Member

Just a note on the allocations - I usually hold my nose at the amount of dynamic allocations Godot does, but here you are using push_back() / append() to add points to a return Vector, for a function that seems designed to be called realtime.

If possible it would be nice to reduce this to a single allocation.
Some ideas :

  • use reserve to pre-reserve the vector with a reasonable number of points
  • if you know the maximum possible points, store in a local array or use alloca (on stack), then finally reserve an output vector of the correct size and copy

On the other hand if it is nearly always returning one value, this may not be so important.

@rburing
Copy link
Member Author

rburing commented Jan 21, 2023

@lawnjelly Thanks, I'll fix that (it's only in the binding, not in the core code that is used by the physics).

Currently the code always returns only one pair (unlike #68958 which returned more pairs in special cases), which seems to be standard for this particular algorithm. I've indicated in some comments in the code how this could be improved if needed.

@Riteo
Copy link
Contributor

Riteo commented Jan 21, 2023

How does licensing work in this situation? I think that there should be credits to both authors, if not the whole license somewhere.

@rburing
Copy link
Member Author

rburing commented Jan 21, 2023

@Riteo Yes, definitely. I meant to post this as a draft; indeed more detailed copyright notices / credits still need to be added.

@rburing rburing marked this pull request as draft January 21, 2023 16:59
@peastman
Copy link
Contributor

It looks like some of this code came from Numerical Recipes. They don't allow using it in open source software. http://numerical.recipes/licenses/redistribute.html

@rburing
Copy link
Member Author

rburing commented Jan 22, 2023

@peastman Yes, the minimization and bracketing code in minimization.cpp, I've noticed it too.

I'm looking for a solution.

@YuriSizov YuriSizov modified the milestones: 4.0, 4.1 Feb 17, 2023
@hpvb
Copy link
Member

hpvb commented Feb 18, 2023

@rburing I have an implementation on a branch that does not use the code from Numerical Recipes here: hpvb@0952643

Feel free to use it, if you do please add an authored-by line for me or something to the PR :)

As for where this code came from: I had ChatGPT explain the algorithm to me step by step and I implemented the C++ code based on its explanation. There was not a lot of creativity involved, mostly because there's really only so many ways to do this. But this code can't possibly be copyrighted by anyone but me, if anyone at all.

@rburing
Copy link
Member Author

rburing commented Mar 2, 2023

@hpvb Thanks a lot for your help! With your latest commit I still run into the issue of getting caught (stopped) when sliding along the cap of a cylinder (which this PR is supposed to fix), see the demo attached above.

There is also a test like that in https://github.com/fabriceci/Godot-Physics-Tests, but it's already passing in master, so I should add a test that fails like the demo.

Could you give the minimization code another look? If needed we could generate some unit tests for the minimization. Once it's ready I'd be happy to rebase my commits on top of yours (and add more detailed license info to COPYRIGHT.txt).

@rburing
Copy link
Member Author

rburing commented Mar 17, 2023

@hpvb My branch test_line_circle_distance_minimization allows testing the minimization code on the function $$f(t) = A_0 + t\cdot (A_1 + t) + A_2 \cdot \sqrt{A_3 + t\cdot (A_4 + t\cdot A_5)}$$ which (for some $A_i$ ) is the squared distance between a circle and a point $P(t)$ on a line, for $t$ in some interval $[a,b]$.

To test alternative minimization code (like yours), check out my branch, replace minimization.cpp, and run the test project line_circle_distance_minimization_test.zip. It compares calculated minima with the pre-computed minima found using this PR (stored in minimization_data.json), and prints the data where the error in (squared) distance is largest.

Running the test project with your minimization code, I get

max_error_t = 3.79414415359497
max_error_dist = 14.3955173492432
max_error_data = { "A": [22.0049724578857, -7.20710611343384, -3, 4.50648736953735, 0.53945195674896, 0.01614552922547], "B": [-0.80917793512344, -0.04843658953905], "a": 2.00355315208435, "b": 5.20355272293091, "loc_min_dist": 1.24090766906738, "loc_min_t": 3.79414415359497 }
max_error_result = (0, 15.63643)

The above output corresponds to the following plot:

to_be_minimal_or_not_to_be

This shows the correct minimum (max_error_data['loc_min_t'], max_error_data['loc_min_dist']) in green (calculated using this PR), and the output max_error_result of your minimization code in red. The red point is in fact on the graph of the function, but it is not in the interval where the minimum is sought, and it is also not a (local) minimum. I haven't yet examined your code in detail, but I suggest we first find out why it is not respecting the interval, and after that we can re-run the test project to iron out any remaining issues.

Based on David Vranek's Fast and Accurate Circle-Circle and Circle-Line
3D Distance Computation.
Implementation based on the one in MinuteTorus by Sang Hyun Son, but
significantly modified.
Based on David Vranek's Fast and Accurate Circle-Circle and Circle-Line
3D Distance Computation.
Implementation based on the one once included in ODE by Russell L.
Smith, but heavily modified.
@rburing
Copy link
Member Author

rburing commented Apr 12, 2023

I've updated/extended the copyright notices/licenses/credits for the circle-circle and line-circle closest points code.

I haven't touched the numerical minimization code yet (see the issues mentioned above).

@rburing rburing mentioned this pull request Apr 23, 2023
8 tasks
@Gamepro5
Copy link
Contributor

Looking forward to this; I noticed that the current implementation for cylinder collisions struggle both on trimesh collision shapes and the edges of a lot of shapes. For instance, if you are moving onto a different angled slope (hits the edge), it may misreport the collision normal. If a pointy spike shape's vertex pokes the rounded part of the cylinder, it can get a wrong collision normal reported and essentially become stuck. Would like to help find edge cases with this implementation by doing extensive testing with it in my character controller.

* I found that testing it in a character controller environment is quite effective, as any misrepresented move_and_collide() normal will immediately be obvious as the character will not move as expected, and will chain off into a noticeable error. It obviously isn't as credible as a unit test, but I've found a lot of collision detection bugs this way.

@Gamepro5
Copy link
Contributor

So I did some testing. It seems like all this improves is the collision detection along the non-circular-cap parts of the cylinder.
Here's a video of cylinder v cylinder collisions before on 4.0.2 stable, and here's a video of cylinder v cylinder collisions on this build.

I have a feeling that the errors still present on the edges of the cylinder mesh might be the same issue as #70508

@rburing rburing modified the milestones: 4.1, 4.2 Jun 7, 2023
@AThousandShips AThousandShips modified the milestones: 4.2, 4.3 Oct 26, 2023
@AThousandShips AThousandShips modified the milestones: 4.3, 4.4 Jul 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants