Multisize Ball-Ball Collision#289
Conversation
…alls of different sizes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 #289 +/- ##
==========================================
- Coverage 46.36% 46.33% -0.03%
==========================================
Files 144 144
Lines 10314 10320 +6
==========================================
Hits 4782 4782
- Misses 5532 5538 +6
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pooltool/physics/resolve/ball_ball/frictional_inelastic/__init__.py (2)
107-107:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate class docstring — model now handles unequal masses.
The docstring still claims this is for equal-mass balls, which contradicts the new per-ball
(m1, R1, m2, R2)support added in this PR.📝 Proposed wording
- """A simple ball-ball collision model including ball-ball friction, and coefficient of restitution for equal-mass balls + """A simple ball-ball collision model including ball-ball friction and a coefficient of restitution, supporting balls of unequal mass and radius.🤖 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_ball/frictional_inelastic/__init__.py` at line 107, Update the module/class docstring to reflect that the frictional_inelastic collision model supports unequal masses and radii by accepting per-ball parameters (m1, R1, m2, R2); replace the phrase "for equal-mass balls" with wording that it handles unequal masses and radii, briefly state it models ball-ball friction and a coefficient of restitution for two possibly different masses, and mention the expected input parameters (m1, R1, m2, R2) so the docstring matches the implemented behavior.
76-83:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winNo-slip path ignores the new per-ball parameters — defeats the purpose of this PR.
Lines 78–79 correctly compute
D_v2_tandD_w2, but lines 82–83 still apply the old equal-mass / equal-radius forms (- D_v1_tandD_w1) to ball 2. Concretely:
- Line 82:
rvw2[1] - D_v1_tequalsrvw2[1] + D_v2_tonly whenm1 == m2(sinceD_v2_t = -(m1/m2)*D_v1_t).- Line 83:
rvw2[2] + D_w1equalsrvw2[2] + D_w2only whenm1 == m2andR1 == R2(sinceD_w2 = (m1/m2) * (R1/R2) * D_w1).For any unequal-mass or unequal-radius collision that falls into the no-slip branch (including the
not has_relative_velocitycase), ball 2's tangential velocity and angular velocity updates will violate Newton's third law and the no-slip contact condition. The newly computedD_v2_t/D_w2are also dead values as written.🐛 Proposed fix
# velocity tangent component for no-slip condition D_v1_t = -(2.0 / 7.0) * v12_c / (1 + m1 / m2) D_w1 = 2.5 / R1 * ptmath.cross(unit_x, D_v1_t) D_v2_t = -(m1 / m2) * D_v1_t D_w2 = 2.5 / R2 * ptmath.cross(-unit_x, D_v2_t) rvw1_f[1] = rvw1[1] + D_v1_t rvw1_f[2] = rvw1[2] + D_w1 - rvw2_f[1] = rvw2[1] - D_v1_t - rvw2_f[2] = rvw2[2] + D_w1 + rvw2_f[1] = rvw2[1] + D_v2_t + rvw2_f[2] = rvw2[2] + D_w2It would be worth adding a regression test that exercises the no-slip branch with
m1 != m2andR1 != R2(e.g., zero relative surface velocity at contact, or a largeu_bthat drives the slip-direction-flip fallback) and asserts conservation of linear/angular momentum about the contact point.🤖 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_ball/frictional_inelastic/__init__.py` around lines 76 - 83, The no-slip branch incorrectly applies D_v1_t and D_w1 to ball 2, ignoring the computed D_v2_t and D_w2; update the rvw2_f assignments so rvw2_f[1] = rvw2[1] + D_v2_t and rvw2_f[2] = rvw2[2] + D_w2 (mirror of rvw1_f using the D_*_2 values) wherever the no-slip path / not has_relative_velocity branch runs, ensuring momentum/torque consistency for unequal m1/m2 and R1/R2; also add a regression test that triggers the no-slip branch with m1 != m2 and R1 != R2 (e.g., zero relative surface velocity or large u_b) and asserts conservation about the contact point.
🤖 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.
Outside diff comments:
In `@pooltool/physics/resolve/ball_ball/frictional_inelastic/__init__.py`:
- Line 107: Update the module/class docstring to reflect that the
frictional_inelastic collision model supports unequal masses and radii by
accepting per-ball parameters (m1, R1, m2, R2); replace the phrase "for
equal-mass balls" with wording that it handles unequal masses and radii, briefly
state it models ball-ball friction and a coefficient of restitution for two
possibly different masses, and mention the expected input parameters (m1, R1,
m2, R2) so the docstring matches the implemented behavior.
- Around line 76-83: The no-slip branch incorrectly applies D_v1_t and D_w1 to
ball 2, ignoring the computed D_v2_t and D_w2; update the rvw2_f assignments so
rvw2_f[1] = rvw2[1] + D_v2_t and rvw2_f[2] = rvw2[2] + D_w2 (mirror of rvw1_f
using the D_*_2 values) wherever the no-slip path / not has_relative_velocity
branch runs, ensuring momentum/torque consistency for unequal m1/m2 and R1/R2;
also add a regression test that triggers the no-slip branch with m1 != m2 and R1
!= R2 (e.g., zero relative surface velocity or large u_b) and asserts
conservation about the contact point.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 45e6c0e7-807c-4c9a-be4d-ca40cde02e7d
📒 Files selected for processing (1)
pooltool/physics/resolve/ball_ball/frictional_inelastic/__init__.py
|
Might be worth profiling this. There's probably a slight performance hit, but I'm not sure how significant. |
|
Branch name has typo, going to re-open the PR |
Good catch |
1 participant
Update ball_ball/frictional_inelastic to support collisions between balls of different sizes.
I checked the A-14 and A-27 plots before and after this change (See my other PR). They appear slightly different, which is pretty annoying. It might be floating point error, but I'm surprised it's noticeable at all on the plots.