Ek/make kiss 3d linear cushion - #373
Conversation
get_real_smallest_magnitude_root re-inlined the realness logic that is_real_number now provides, and that its sibling get_real_positive_smallest_root already delegates to.
When the velocity is parallel to the cushion axis there is no perpendicular component, so alpha and beta are both zero and the quadratic degenerates. The solver returns nans, get_real_smallest_magnitude_root returns inf, and t * v evaluates to nan on the axis-aligned components. The displacement guard then silently failed, because a nan comparison is always False, and the nan flowed into the ball position. Fall back explicitly on a non-finite root. Also break the tie in _constrain_to_table when the ball sits exactly on the vertical through the cushion axis. np.sign returned 0 there, zeroing the horizontal component of the new direction and leaving it non-unit, which placed the ball well inside the cushion surface.
WalkthroughLinear and circular cushion kissing now use quadratic root selection, shared fallback displacement handling, cushion nose radius, and table-height constraints. New tests cover geometry, airborne states, degenerate velocities, fallback cases, and randomized inputs. ChangesCushion kissing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 #373 +/- ##
==========================================
- Coverage 47.77% 47.75% -0.03%
==========================================
Files 159 159
Lines 10758 10788 +30
==========================================
+ Hits 5140 5152 +12
- Misses 5618 5636 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@derek-mcblane now that your linear cushion detection is merged, this is ready for review. I imagine the "make kiss" paradigm is relatively bespoke and unique to pooltool, so no pressure to review thoroughly. In truth I don't know if it helps/hurts simulation stability with floating point precision... I do know it's been a major pain in my as* though. Either way, I am pretty satisfied with the solution here in this PR. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pooltool/physics/resolve/ball_cushion/core.py (1)
203-243: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClamp the rotation cosine before
np.sqrtto avoid a silent NaN.
a = (R - c[2]) / arm_len(line 238) can exceed 1 in magnitude wheneverabs(R - cushion.height) > arm_len(arm_lenisR + nose_radius + spacerin the normal call paths). UnderPocketTableSpecsdefaults this stays safely within bounds, butLinearCushionSegmentaccepts arbitraryp1/p2heights andnose_radius, so a custom cushion configuration with an unusually tall or short nose relative toRwould pushabs(a) > 1, making1.0 - a * anegative andnp.sqrtreturn NaN, silently corrupting the returned position with no error raised.Clamp
ato[-1.0, 1.0]before computingbto make this function robust to arbitrary (but still geometrically valid) cushion configurations.🛡️ Proposed defensive clamp
a = (R - c[2]) / arm_len + a = max(-1.0, min(1.0, a)) side = 1.0 if np.dot(direction, h_hat) >= 0 else -1.0 b = side * np.sqrt(1.0 - a * a)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pooltool/physics/resolve/ball_cushion/core.py` around lines 203 - 243, In _constrain_to_table, clamp the computed rotation cosine a to the inclusive range [-1.0, 1.0] before calculating b with np.sqrt. Preserve the existing side selection and returned-position logic while ensuring valid custom cushion configurations cannot produce a negative square-root argument or NaN.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pooltool/physics/resolve/ball_cushion/core.py`:
- Around line 162-169: Update the circular cushion path in the function
containing roots_complex and get_real_smallest_magnitude_root to check
np.isfinite(t) before using t in ptmath.norm3d or updating ball.state.rvw[0].
Route non-finite roots through _apply_fallback_positioning_circular, mirroring
the existing linear cushion guard, and add a regression test in the make_kiss
tests for zero XY velocity.
---
Nitpick comments:
In `@pooltool/physics/resolve/ball_cushion/core.py`:
- Around line 203-243: In _constrain_to_table, clamp the computed rotation
cosine a to the inclusive range [-1.0, 1.0] before calculating b with np.sqrt.
Preserve the existing side selection and returned-position logic while ensuring
valid custom cushion configurations cannot produce a negative square-root
argument or NaN.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d75a77d-5c45-4813-bba3-09e6134b81bd
📒 Files selected for processing (6)
pooltool/physics/resolve/ball_cushion/core.pypooltool/ptmath/roots/__init__.pypooltool/ptmath/roots/core.pytests/physics/resolve/ball_cushion/conftest.pytests/physics/resolve/ball_cushion/test_ball_cushion.pytests/physics/resolve/ball_cushion/test_make_kiss.py
| roots_complex = ptmath.roots.quadratic.solve(alpha, beta, gamma) | ||
| t = ptmath.roots.get_real_smallest_magnitude_root(roots_complex) | ||
|
|
||
| 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()] | ||
|
|
||
| if ptmath.norm3d(t * v) > 5 * spacer: | ||
| ball.state.rvw[0] = self._apply_fallback_positioning_circular( | ||
| ball, cushion, spacer | ||
| ) | ||
| return ball | ||
| if ptmath.norm3d(t * v) > FALLBACK_DISPLACEMENT_FACTOR * spacer: | ||
| return _apply_fallback_positioning_circular(ball, cushion, spacer) | ||
|
|
||
| ball.state.rvw[0] = r + t * v | ||
|
|
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
1 participant
What the algorithm does
The job: the detector hands make_kiss a ball that's slightly wrong — overlapping the cushion by a hair, or
floating a hair away — because floating-point. Before the resolver computes the bounce, the ball needs to
be exactly barely-touching.
The old 2D way
treat the cushion as a flat vertical wall, shove the ball perpendicular to it.
The new 3D way
Five steps:
The ball is just touching when its center sits at distance R + nose_radius from the pipe's centerline.
along its straight-line path is the ball at exactly the right distance from the centerline?" That's a
quadratic in t. Take the root closest to zero (smallest correction) and move to r + t*v. This beats shoving
it sideways because you're following the path the ball actually took, which is more accurate and also reduces
likelihood of intersecting with an object as a result of the nudge.
along the cushion axis, since sliding along a pipe doesn't change your distance from it. That's what makes
it a clean quadratic instead of something ugly.
hovering above it. _constrain_to_table spins the ball around the pipe's axis, like a bead on a wire, until
the height is right: exactly z = R if it's on the table, at least z = R if airborne. Because it's a
rotation about the axis, the distance to the pipe never changes — the spacing from step 2 survives intact.
trick: push straight out perpendicular from the centerline, then apply the same height fix.
The clever part is step 4. Two constraints that would normally fight — distance-from-cushion and
height-above-table — and rotating about the cushion axis satisfies the second for free without disturbing
the first.
This doesn't handle circular cushion.
Summary by CodeRabbit
Bug Fixes
Tests