3D Ball-Linear-Cushion Detection#350
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds 3D ball-linear-cushion collision detection by rotating the ball's 3D position polynomial into the cushion's local frame, reducing the problem to a 2D parabola-vs-circle quartic, and filtering roots by segment bounds and approach direction. New math helpers ( Changes3D Linear-Cushion Collision Detection
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 |
2a6f0f6 to
dc20aa3
Compare
|
dc20aa3 to
274326c
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #350 +/- ##
==========================================
+ Coverage 47.75% 47.77% +0.02%
==========================================
Files 159 159
Lines 10716 10758 +42
==========================================
+ Hits 5117 5140 +23
- Misses 5599 5618 +19
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:
|
274326c to
65bb7c0
Compare
65bb7c0 to
fbddc7b
Compare
fbddc7b to
42923c9
Compare
|
@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: 3
🤖 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/evolution/event_based/detect/ball_cushion.py`:
- Around line 67-114: The new 3D collision path in
ball_linear_cushion_segment_collison_time ignores cushion.direction, so
one-sided cushions become hittable from both sides. Add the same direction-side
filtering used by the legacy linear-cushion solver after selecting the collision
root, or before returning from
select_ball_linear_cushion_segment_collision_root, and make sure the check is
applied in the is_3d=True branch for LinearCushionSegment.
- Around line 91-95: The first 3D detection path in ball_cushion uses
parabola_circle_distance_2d_quartic_coefficients with only a single rotated
coordinate from p_rotated.T and cushion_origin_rotated, but that helper indexes
both xy[1] and center[1]. Update the call site to pass both rotated XY
components (not a 1-element slice) so the quartic builder receives valid 2D
inputs, and keep the change localized to the ball_cushion detection logic around
the parabola_circle_distance_2d_quartic_coefficients call.
- Around line 53-61: The approach check in ball_cushion.py is using an incorrect
velocity expression from the polynomial coefficients, which can misclassify
collisions. Update the collision test in the detection logic around
ball_position_polynomial() / the root loop so the velocity used for the dot
product reflects the true time derivative, using the same p coefficients that
define the position polynomial. Keep the existing root filtering and normal
calculation, but replace the current v / v_collision computation with the
correct derivative-based velocity before evaluating the approach check.
🪄 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
Run ID: 7d02582d-9a63-4191-a131-909a04bea39d
📒 Files selected for processing (5)
pooltool/evolution/event_based/detect/ball_cushion.pypooltool/evolution/event_based/detect/detector.pypooltool/evolution/event_based/detect/quartic_coefficients.pypooltool/ptmath/roots/__init__.pypooltool/ptmath/roots/core.py
- I have no idea why this worked before
select_ball_linear_cushion_segment_collision_root used v1 = 0.5 * p[2] for the t-dependent term of the velocity, but p[2] is the t^2 coefficient of the position polynomial (half the acceleration), so the velocity is v(t) = p[1] + 2 * p[2] * t. The factor was off by 4x, which could flip the sign of the dot(xy_normal, v_collision) "moving toward cushion" gate and cause a valid approaching collision to be skipped or a receding root to be wrongly accepted.
ekiefl
left a comment
ekiefl
left a comment
There was a problem hiding this comment.
Looks good @derek-mcblane!
I found some bugs that I'm 99% sure on, so I went ahead and fixed them--but I'd appreciate a second look.
142c3e2 sends x and y explicitly. I'm very surprised it worked with [0:1], and Claude thinks it was due to a slice memory vs contiguous array.
b29efe7 I'm not sure if you purposefully used np.isclose, but I made it exact to match ball-ball detection. We may need to add some tolerance in the future, but I'd rather introduce tolerance as needed and not relying on np.isclose defaults for absolute/relative tolerance.
|
Oh, and also, thanks for your patience. Time has been hard to come by recently. |
Ooops, I fixed these in another branch and they didn't make it to this one. Sorry for the trouble. |
Don't sweat it |
|
|
||
| v0 = p[1] | ||
| v1 = 0.5 * p[2] | ||
| v1 = 2 * p[2] |
There was a problem hiding this comment.
This is correct. I guess I forgot how to take a derivative.
| v = np.array([p[1], 0.5 * p[2]]) | ||
| v0 = p[1] | ||
| v1 = 0.5 * p[2] |
There was a problem hiding this comment.
Could put v together outside of this function, instead. It might also make sense to construct it similar to ball_position_polynomial instead of taking a derivative.
|
Hey I'm phone-bound until next week but it looks like this is good to merge. Agreed? |
I'd like to find the branch I made those fixes on and see if there's anything else I missed so they don't get lost. Should be able to this weekend. If there's anything being held up by this feel free to merge and I can do the checks after. Otherwise, let's leave it open. |
|
That sounds good to me! Was just checking in to make sure I wasn't blocking anything |
2 participants
Adding 3D detection support for ball-linear-cushion to resolve #303.
Summary by CodeRabbit
New Features
Bug Fixes