3D Ball-Ball Detection#342
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:
WalkthroughThis PR implements full 3D ball-ball collision detection by adding supporting math modules, a real 3D collision-time solver, and consolidating the 2D/3D event detection into a unified dispatcher with backward-compatible test coverage. Changes3D Ball-Ball Collision Detection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 #342 +/- ##
==========================================
+ Coverage 47.72% 47.75% +0.02%
==========================================
Files 157 159 +2
Lines 10670 10716 +46
==========================================
+ Hits 5092 5117 +25
- Misses 5578 5599 +21
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:
|
derek-mcblane
force-pushed
the
3d-ball-ball-detection
branch
from
8bdb40b to
59aaeb7
Compare
derek-mcblane
changed the title
|
Wow, the way you composed these functions is really elegant, @derek-mcblane. Is it ready for review?
I totally agree, it would just involve removing the pooltool/pooltool/evolution/event_based/detect/detector.py Lines 151 to 160 in 3e37285 Once you do this, fingers crossed all the passes test. Then as an extra layer of security to ensure we didn't introduce any regressions in the 2D behavior, I'm going to throw the kitchen sink at the detector and make sure the same shot is simulated across I know this is still a draft, but at this point here are the things I know I'd like to see in this branch. Could you please let me know what you're interested in / have the energy for? Whatever is left over, I'm happy to take over.
|
|
I'll note that I wouldn't mind taking over this branch, just because it's very clear for me what needs to be done. There's several other projects that require your expertise, so now that you've laid down the framework in this PR, I'm happy to clean it up so if you think your time could be spent better elsewhere. |
Thanks! I'm going to do a quick implementation of ball-ball collision resolution so I can spot check this in the simulator.
I might also do something about this.. I'm thinking I will leave the 2d detector as is for now to give time for any bugs in the 3d version to surface, but I can eliminate the code duplication.
I can add some tests for sure. I suspect you might have some to add afterwards.
I'll do this. Like I said I think I'm going to levae the 2d detector as is for now. |
|
Ok, I'll reel back my ask for consolidating 2D with 3D on this branch, and instead leave it up to you how you want to slice and dice this. We can consider that checklist I made an eventuality, rather than specific to this branch. |
| if is_3d: | ||
| dtau_E = ball_ball_collision_time_3d(ball1, ball2) | ||
| else: | ||
| dtau_E = ball_ball_collision_time( |
There was a problem hiding this comment.
Moved the is_3d check to here to avoid code duplication without worrying about altering the 2d behavior quite yet.
derek-mcblane
force-pushed
the
3d-ball-ball-detection
branch
from
02f9ab2 to
d31c055
Compare
derek-mcblane
force-pushed
the
3d-ball-ball-detection
branch
from
027dd7b to
07d4524
Compare
pass is_3d to get_next_ball_ball_event instead of using two completely separate functions so that everything except the collision time prediction can be shared between 2d and 3d
derek-mcblane
force-pushed
the
3d-ball-ball-detection
branch
from
07d4524 to
88ae60c
Compare
|
👀 |
derek-mcblane
force-pushed
the
3d-ball-ball-detection
branch
from
069ad09 to
85800e2
Compare
derek-mcblane
force-pushed
the
3d-ball-ball-detection
branch
from
85800e2 to
eef44ba
Compare
- get_ball_ball_collision_time -> get_ball_ball_collision_time_2d - get_ball_ball_collision_time_3d -> get_ball_ball_collision_time
ekiefl
left a comment
ekiefl
left a comment
There was a problem hiding this comment.
A few requests but it otherwise looks good to goooo!
I added some docstrings and made the following fn name replacements:
- get_ball_ball_collision_time -> get_ball_ball_collision_time_2d
- get_ball_ball_collision_time_3d -> get_ball_ball_collision_time
|
Oh right I'm gonna add some more tests as I mentioned. In that case please leave these comments to me, I'll have a taste of my own medicine |
Oh no, I just addressed them haha.. I hope you didn't start. |
| """Build the position-vs-time polynomial for a ball. | ||
|
|
||
| The trajectory is expressed as | ||
|
|
||
| r(t) = r0 + V0 * t + 1/2 a * t**2 | ||
| = p[0] + p[1] * t + p[2] * t**2 | ||
|
|
||
| where each row of the returned ``(3, 3)`` array is an ``(x, y, z)`` vector. | ||
|
|
||
| Returns: | ||
| A ``(3, 3)`` array ``p`` where rows are the coefficients of the | ||
| position polynomial in increasing power of ``t``: | ||
|
|
||
| * ``p[0]``: initial position (``rvw[0]``). | ||
| * ``p[1]``: initial velocity (``rvw[1]``). | ||
| * ``p[2]``: half the acceleration vector (the coefficient of ``t**2``, not | ||
| the acceleration itself). | ||
| """ |
|
Ok I added a ball falling on ball test, and two balls with uneven radii test. Going to merge this. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_ball.py`:
- Line 138: The docstring incorrectly claims the routine only detects "in 2D
mode" while the function now dispatches both 2D and 3D based on is_3d; update
the function docstring in pooltool/evolution/event_based/detect/ball_ball.py to
describe that it detects the next ball-ball collision for both 2D and 3D,
mention the is_3d flag determines which branch runs, and briefly state the
behavior for each branch (e.g., 2D collision logic vs 3D collision logic) while
keeping it short and accurate; reference the is_3d variable and the function
that contains this docstring when making the change.
In `@pooltool/evolution/event_based/detect/ball_position_polynomial.py`:
- Around line 41-43: tangent_surface_velocity(rvw, -unit_z, R) returns a
velocity vector that must be normalized before applying friction acceleration;
replace the direct use of v_hat_c_0 with a unit vector: compute norm =
np.linalg.norm(v_hat_c_0), if norm > 0 set v_hat_c_0 = v_hat_c_0 / norm
(otherwise set to zero), then use -v_hat_c_0 when computing p[2] so the 0.5 *
u_s * g factor scales correctly; update the assignment involving v_hat_c_0 (the
variable in question) accordingly and keep the rest of the calculation
unchanged.
🪄 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: 1b5124d6-00de-4417-be4c-320a8184ea3e
📒 Files selected for processing (7)
pooltool/evolution/event_based/detect/__init__.pypooltool/evolution/event_based/detect/ball_ball.pypooltool/evolution/event_based/detect/ball_position_polynomial.pypooltool/evolution/event_based/detect/detector.pypooltool/evolution/event_based/detect/quartic_coefficients.pytests/evolution/event_based/test_ball_ball.pytests/evolution/event_based/test_simulate.py
| def get_next_ball_ball_event( | ||
| shot: System, collision_cache: CollisionCache, *, is_3d: bool | ||
| ) -> Event: | ||
| """Detect the next ball-ball collision in 2D mode.""" |
There was a problem hiding this comment.
Docstring is stale after 2D/3D consolidation.
Line 138 says this detects collisions "in 2D mode", but the function now dispatches both 2D and 3D via is_3d.
Suggested fix
- """Detect the next ball-ball collision in 2D mode."""
+ """Detect the next ball-ball collision."""📝 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.
| """Detect the next ball-ball collision in 2D mode.""" | |
| """Detect the next ball-ball collision.""" |
🤖 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/evolution/event_based/detect/ball_ball.py` at line 138, The
docstring incorrectly claims the routine only detects "in 2D mode" while the
function now dispatches both 2D and 3D based on is_3d; update the function
docstring in pooltool/evolution/event_based/detect/ball_ball.py to describe that
it detects the next ball-ball collision for both 2D and 3D, mention the is_3d
flag determines which branch runs, and briefly state the behavior for each
branch (e.g., 2D collision logic vs 3D collision logic) while keeping it short
and accurate; reference the is_3d variable and the function that contains this
docstring when making the change.
| unit_z = np.array([0, 0, 1]) | ||
| v_hat_c_0 = tangent_surface_velocity(rvw, -unit_z, R) | ||
| p[2] = 0.5 * u_s * g * -v_hat_c_0 |
There was a problem hiding this comment.
Normalize sliding contact velocity before applying friction acceleration.
tangent_surface_velocity(...) returns a velocity vector, not a unit direction. Using it directly makes p[2] scale with speed, which breaks the 0.5 * a coefficient and can skew collision-time roots.
Suggested fix
elif s == const.sliding:
- unit_z = np.array([0, 0, 1])
- v_hat_c_0 = tangent_surface_velocity(rvw, -unit_z, R)
- p[2] = 0.5 * u_s * g * -v_hat_c_0
+ unit_z = np.array([0.0, 0.0, 1.0])
+ v_c_0 = tangent_surface_velocity(rvw, -unit_z, R)
+ v_c_0_norm = ptmath.norm3d(v_c_0)
+ if v_c_0_norm == 0.0:
+ p[2] = np.zeros(3)
+ else:
+ p[2] = -0.5 * u_s * g * (v_c_0 / v_c_0_norm)🤖 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/evolution/event_based/detect/ball_position_polynomial.py` around
lines 41 - 43, tangent_surface_velocity(rvw, -unit_z, R) returns a velocity
vector that must be normalized before applying friction acceleration; replace
the direct use of v_hat_c_0 with a unit vector: compute norm =
np.linalg.norm(v_hat_c_0), if norm > 0 set v_hat_c_0 = v_hat_c_0 / norm
(otherwise set to zero), then use -v_hat_c_0 when computing p[2] so the 0.5 *
u_s * g factor scales correctly; update the assignment involving v_hat_c_0 (the
variable in question) accordingly and keep the rest of the calculation
unchanged.
There was a problem hiding this comment.
Whooops. Good catch!
There was a problem hiding this comment.
@derek-mcblane, no worries — easy to miss! The suggested fix normalizes v_c_0 and also guards against the zero-velocity case (ball already at the sliding threshold) to keep the polynomial well-formed. Let me know if you'd like any clarification on the fix.
(=^･ω･^=)
There was a problem hiding this comment.
@derek-mcblane This is a real bug. I fixed it in d668965.
Noteworthy that after removing all the 2D dispatch logic and replacing it with 3D, all the tests pass. But I'm gonna keep the 2D dispatch until I do more tests.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
2 participants
Implement
get_next_ball_ball_3d_eventto resolve #305.There's no reason that this can't just completely replace 2d event detection.
Summary by CodeRabbit
New Features
Tests