Improve ball-ball collision handling for edge cases#257
Conversation
- Tests not passing
- This is made possible by improvements to the make_kiss function - In brief, a spacer between balls is enforced by translating colliding balls along their trajectories until the spacer is achieved.
- Breaks when range(5)
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #257 +/- ##
==========================================
+ Coverage 44.22% 45.98% +1.76%
==========================================
Files 127 144 +17
Lines 8554 10231 +1677
==========================================
+ Hits 3783 4705 +922
- Misses 4771 5526 +755
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Saves newtons cradle
ekiefl
changed the title
make_kiss)![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@pooltool/physics/resolve/ball_ball/core.py`:
- Around line 205-208: The line "theft_fraction = theft_fraction" is a no-op and
should be removed; update the block in resolve/ball_ball/core.py (around where
theft_fraction, stolen_loc_velocity, and chaser_loc_vel are used) so that
theft_fraction is either taken from the correct source (e.g., a parameter or
self.theft_fraction) or computed before using it, then compute
stolen_loc_velocity = chaser_loc_vel * theft_fraction; remove the redundant
self-assignment to eliminate the dead code.
- Around line 109-113: The code computing `keep` can divide by zero and may
produce an empty `roots` array; fix by guarding the division and handling the
empty-case: compute imag_mag and real_mag as before but build a safe denominator
like `safe_real = np.where(np.isclose(real_mag, 0.0), np.finfo(float).eps,
real_mag)` (or explicitly mask zero-real entries), then compute `ratio =
imag_mag / safe_real` and `keep = ratio < 1e-3`; after `roots =
roots_complex[keep].real` add a check `if roots.size == 0:` and either pick a
fallback (e.g., use `roots = roots_complex.real` and select the element with
minimal absolute value) or raise a clear exception; finally assign `t` from the
chosen `roots` as `t = roots[np.abs(roots).argmin()]` to avoid ValueError.
In `@pooltool/ptmath/roots/quadratic.py`:
- Around line 36-37: Replace exact zero checks on polynomial coefficients with
an EPS tolerance like const.EPS to match solve's behavior: change conditions
such as "if abs(_a) != 0" to "if abs(_a) > const.EPS" (and similarly use >
const.EPS for checks involving _b and _c). Ensure const.EPS is
imported/available in this module and apply the same threshold in all
corresponding branches (the quadratic-case check around _a and the
degenerate-case checks around _b/_c) so near‑zero coefficients are treated
consistently with solve.
In `@tests/evolution/event_based/test_simulate.py`:
- Around line 409-417: The docstring for
test_ball_ball_collision_for_intersecting_balls is outdated and contradicts the
test assertions; update the docstring to state that already-intersecting balls
are detected as an immediate collision (at shot.t or t==0) thanks to the
improved make_kiss behavior, and remove the line claiming they "don't collide"
so the description matches the test's expectation of a collision at t==0.
🧹 Nitpick comments (5)
pooltool/ptmath/roots/_quartic_numba.py (1)
639-642: Consider a regression test for tiny‑discriminant cases.
This threshold change alters case selection; a targeted test (near‑zero non‑zero d2) would guard the Newton’s‑cradle edge case.pooltool/physics/resolve/transition/__init__.py (1)
18-19: Consider making this constant private and document the rationale for the tolerance value.The constant is only used internally for assertions within this module, so prefixing with an underscore (
_TOLERANCE) would better communicate its scope. Additionally, this tolerance (1e-12) is 1000× stricter than the previousconst.EPS_SPACE(1e-9). A brief comment explaining why this tighter tolerance is appropriate for velocity checks would aid future maintainability.Suggested change
-TOLERANCE = 1e-12 +# Tolerance for velocity assertions during state transitions. +# Using a tight tolerance (1e-12) to ensure velocities are effectively zero +# before snapping them to exactly zero. +_TOLERANCE = 1e-12sandbox/newtons_cradle.py (1)
43-49: Intentionalspacing_factor=0should be documented.The
generate_layoutdocstring warns "Setting this to 0 is not recommended," but for Newton's cradle, touching balls are required. Consider adding a brief comment explaining this intentional choice to prevent future confusion.balls = generate_layout( blueprint, table, ballset=pt.objects.BallSet("pooltool_pocket"), ball_params=ball_params, + # spacing_factor=0 is intentional: Newton's cradle requires touching balls spacing_factor=0, )pooltool/physics/resolve/ball_ball/core.py (1)
30-46: Minor: Avoid redundant unit vector computation.
ptmath.unit_vector(r2 - r1)is computed twice. Consider caching it:♻️ Suggested optimization
def _apply_fallback_positioning( self, ball1: Ball, ball2: Ball, r1: np.ndarray, r2: np.ndarray, spacer: float, ) -> tuple[np.ndarray, np.ndarray]: """Apply fallback positioning by moving balls along line of centers.""" correction = 2 * ball1.params.R - ptmath.norm3d(r2 - r1) + spacer - r1_corrected = r1 - correction / 2 * ptmath.unit_vector(r2 - r1) - r2_corrected = r2 + correction / 2 * ptmath.unit_vector(r2 - r1) + direction = ptmath.unit_vector(r2 - r1) + half_correction = correction / 2 + r1_corrected = r1 - half_correction * direction + r2_corrected = r2 + half_correction * direction return r1_corrected, r2_correctedpooltool/physics/resolve/ball_cushion/core.py (1)
59-78: Align cushion spacer with the shared distance tolerance.Hard-coding
1e-9in both paths can drift from the rest of the engine’s separation tolerances. Consider using the shared minimum-distance constant (or a module-level constant) so ball–ball and ball–cushion resolution stay consistent and tunable.♻️ Suggested refactor
- spacer = 1e-9 + spacer = MIN_DIST ... - spacer = 1e-9 + spacer = MIN_DIST(Requires importing
MIN_DISTonce at the top.)Also applies to: 109-125
| imag_mag = np.abs(roots_complex.imag) | ||
| real_mag = np.abs(roots_complex.real) | ||
| keep = (imag_mag / real_mag) < 1e-3 | ||
| roots = roots_complex[keep].real | ||
| t = roots[np.abs(roots).argmin()] |
There was a problem hiding this comment.
Potential division by zero and empty array access.
Two edge cases may cause runtime errors:
- Line 111:
real_magcould be zero if a root has zero real part, causing division by zero. - Line 113: If all roots are filtered out by the
keepmask,rootsbecomes empty andargmin()will raise aValueError.
🐛 Proposed fix
imag_mag = np.abs(roots_complex.imag)
real_mag = np.abs(roots_complex.real)
- keep = (imag_mag / real_mag) < 1e-3
+ # Avoid division by zero; treat roots with tiny real part as non-real
+ keep = (real_mag > 1e-14) & ((imag_mag / real_mag) < 1e-3)
roots = roots_complex[keep].real
- t = roots[np.abs(roots).argmin()]
+
+ if len(roots) == 0:
+ # No valid roots found, use fallback
+ r1_corrected, r2_corrected = self._apply_fallback_positioning(
+ ball1, ball2, r1, r2, spacer
+ )
+ ball1.state.rvw[0] = r1_corrected
+ ball2.state.rvw[0] = r2_corrected
+ return ball1, ball2
+
+ t = roots[np.abs(roots).argmin()]🤖 Prompt for AI Agents
In `@pooltool/physics/resolve/ball_ball/core.py` around lines 109 - 113, The code
computing `keep` can divide by zero and may produce an empty `roots` array; fix
by guarding the division and handling the empty-case: compute imag_mag and
real_mag as before but build a safe denominator like `safe_real =
np.where(np.isclose(real_mag, 0.0), np.finfo(float).eps, real_mag)` (or
explicitly mask zero-real entries), then compute `ratio = imag_mag / safe_real`
and `keep = ratio < 1e-3`; after `roots = roots_complex[keep].real` add a check
`if roots.size == 0:` and either pick a fallback (e.g., use `roots =
roots_complex.real` and select the element with minimal absolute value) or raise
a clear exception; finally assign `t` from the chosen `roots` as `t =
roots[np.abs(roots).argmin()]` to avoid ValueError.
| # Chased ball steals fraction of chaser's line of centers momentum | ||
| # FIXME: We assume equal mass, so transfer velocity directly | ||
| theft_fraction = theft_fraction | ||
| stolen_loc_velocity = chaser_loc_vel * theft_fraction |
There was a problem hiding this comment.
Dead code: self-assignment is a no-op.
Line 207 assigns theft_fraction to itself, which has no effect. This appears to be leftover from refactoring.
# Chased ball steals fraction of chaser's line of centers momentum
# FIXME: We assume equal mass, so transfer velocity directly
- theft_fraction = theft_fraction
stolen_loc_velocity = chaser_loc_vel * theft_fraction📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Chased ball steals fraction of chaser's line of centers momentum | |
| # FIXME: We assume equal mass, so transfer velocity directly | |
| theft_fraction = theft_fraction | |
| stolen_loc_velocity = chaser_loc_vel * theft_fraction | |
| # Chased ball steals fraction of chaser's line of centers momentum | |
| # FIXME: We assume equal mass, so transfer velocity directly | |
| stolen_loc_velocity = chaser_loc_vel * theft_fraction |
🤖 Prompt for AI Agents
In `@pooltool/physics/resolve/ball_ball/core.py` around lines 205 - 208, The line
"theft_fraction = theft_fraction" is a no-op and should be removed; update the
block in resolve/ball_ball/core.py (around where theft_fraction,
stolen_loc_velocity, and chaser_loc_vel are used) so that theft_fraction is
either taken from the correct source (e.g., a parameter or self.theft_fraction)
or computed before using it, then compute stolen_loc_velocity = chaser_loc_vel *
theft_fraction; remove the redundant self-assignment to eliminate the dead code.
| if abs(_a) != 0: | ||
| # Quadratic case |
There was a problem hiding this comment.
Use EPS tolerance for near‑zero coefficients (consistency with solve).
Exact-zero checks can misclassify near‑degenerate cases and amplify numeric error compared to solve, which uses const.EPS. Consider aligning the thresholding to avoid unstable roots for small coefficients.
🔧 Suggested fix
- if abs(_a) != 0:
+ if abs(_a) >= const.EPS:
@@
- if abs(_b) != 0:
+ if abs(_b) >= const.EPS:Also applies to: 63-64
🤖 Prompt for AI Agents
In `@pooltool/ptmath/roots/quadratic.py` around lines 36 - 37, Replace exact zero
checks on polynomial coefficients with an EPS tolerance like const.EPS to match
solve's behavior: change conditions such as "if abs(_a) != 0" to "if abs(_a) >
const.EPS" (and similarly use > const.EPS for checks involving _b and _c).
Ensure const.EPS is imported/available in this module and apply the same
threshold in all corresponding branches (the quadratic-case check around _a and
the degenerate-case checks around _b/_c) so near‑zero coefficients are treated
consistently with solve.
| def test_ball_ball_collision_for_intersecting_balls(): | ||
| """Two already intersecting balls don't collide. | ||
|
|
||
| In this instance, no further collision is detected because the balls are already | ||
| intersecting. Otherwise perpetual internal collisions occur, keeping the two balls | ||
| locked. | ||
| Previously, intersecting balls were prevented from colliding to avoid perpetual | ||
| internal collisions. Now, with the improved make_kiss implementation, intersecting | ||
| balls are properly separated and collide normally. | ||
|
|
||
| This test doesn't make sure that balls don't intersect, it tests the safeguard that | ||
| prevents already intersecting balls from colliding with their internal walls, which | ||
| keeps them intersected like links in a chain. | ||
| This test verifies that intersecting balls are detected as a collision at time == | ||
| shot.t |
There was a problem hiding this comment.
Docstring contradicts the new intersecting-ball behavior.
It states intersecting balls “don’t collide,” but the assertions now expect an immediate collision at t=0. Update the wording to avoid confusion.
💡 Suggested fix
- """Two already intersecting balls don't collide.
+ """Two already intersecting balls do collide.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_ball_ball_collision_for_intersecting_balls(): | |
| """Two already intersecting balls don't collide. | |
| In this instance, no further collision is detected because the balls are already | |
| intersecting. Otherwise perpetual internal collisions occur, keeping the two balls | |
| locked. | |
| Previously, intersecting balls were prevented from colliding to avoid perpetual | |
| internal collisions. Now, with the improved make_kiss implementation, intersecting | |
| balls are properly separated and collide normally. | |
| This test doesn't make sure that balls don't intersect, it tests the safeguard that | |
| prevents already intersecting balls from colliding with their internal walls, which | |
| keeps them intersected like links in a chain. | |
| This test verifies that intersecting balls are detected as a collision at time == | |
| shot.t | |
| def test_ball_ball_collision_for_intersecting_balls(): | |
| """Two already intersecting balls do collide. | |
| Previously, intersecting balls were prevented from colliding to avoid perpetual | |
| internal collisions. Now, with the improved make_kiss implementation, intersecting | |
| balls are properly separated and collide normally. | |
| This test verifies that intersecting balls are detected as a collision at time == | |
| shot.t |
🤖 Prompt for AI Agents
In `@tests/evolution/event_based/test_simulate.py` around lines 409 - 417, The
docstring for test_ball_ball_collision_for_intersecting_balls is outdated and
contradicts the test assertions; update the docstring to state that
already-intersecting balls are detected as an immediate collision (at shot.t or
t==0) thanks to the improved make_kiss behavior, and remove the line claiming
they "don't collide" so the description matches the test's expectation of a
collision at t==0.
1 participant
Summary
Major improvements to ball-ball collision detection and resolution, fixing edge cases with nearly-touching and overlapping balls. Introduces priority-based simultaneous event resolution and adds support for complex scenarios like Newton's cradle simulations.
Key Changes
Improved
make_kiss(only for ball-ball)Replaced naive ball positioning (move both balls uniformly along line-of-centers until they are a spacer distance away) with a velocity-based approach. By creating a guaranteed space between balls, we prevent floating point precision errors where balls are evolved into a slightly intersecting state. And by moving the balls along and in proportion with their linear trajectories, we avoid potentially displacing balls into other objects like cushions or other balls:
Continually Touching Resolution
Handles rare edge cases where balls move with nearly identical velocities (e.g., Newton's cradle):
Priority-Based Simultaneous Event Resolution
When multiple events occur at the same (exact) time (which is very rare, but happens often in Newon's cradle-like scenarios), resolve them according to priority tiers:
This prioritizes resolving an energetic event over one between two barely moving balls, for example. The necessity of this is not well understood, however, at the very least, it's nice to resolve simultaneous events in a prescribed order.
Overlapping Ball Collision Detection (pooltool/evolution/event_based/simulate.py:427-234)
Previously, intersecting balls were prevented from colliding (returned np.inf collision time) to avoid perpetual internal collisions. Now:
python sandbox/break.py --spacing-factor 0.0)Quartic Solver Robustness (pooltool/ptmath/roots/_quartic_numba.py:783-809)
Miscellaneous
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.