Multisize Ball-Ball Collision#290
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:
WalkthroughThe collision resolver for ChangesParameterized Frictional Inelastic Collision Generalization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 #290 +/- ##
==========================================
+ Coverage 46.36% 46.51% +0.15%
==========================================
Files 144 144
Lines 10314 10332 +18
==========================================
+ Hits 4782 4806 +24
+ Misses 5532 5526 -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:
|
…alls of different sizes
derek-mcblane
force-pushed
the
multi-size-ball-ball
branch
from
b306054 to
dfd7115
Compare
|
Fixed review comments from old PR (#289 (review)) |
|
Thanks @derek-mcblane, I'm going to review it today. |
|
Theory laid out by @derek-mcblane here: https://discord.com/channels/1116210227688255601/1378776579106279434/1502503637107347577 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
To be thorough, I should check that the math works out to be the same when the masses and radii are equal. I should also spot check the simulator visualization. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pooltool/physics/resolve/ball_ball/frictional_inelastic/__init__.py (1)
29-89: ⚡ Quick winAdd an automated unequal-ball regression test.
This block now owns all of the asymmetric impulse math, but the PR notes that the A-14/A-27 plots already moved slightly and the current validation was manual. A small test for one
m1 != m2/R1 != R2case that checks key invariants here—e.g. linear momentum conservation and the expected slip/no-slip contact-velocity behavior—would make future sign or radius regressions much easier to catch.🤖 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 29 - 89, Add a small automated unit test that constructs an unequal-ball collision (set m1 != m2 and R1 != R2), builds representative rvw1/rvw2 input arrays, calls the collision routine in this frictional_inelastic module to get rvw1_f and rvw2_f, and then verifies (1) linear momentum conservation by checking m1*rvw1[1] + m2*rvw2[1] ≈ m1*rvw1_f[1] + m2*rvw2_f[1], and (2) the slip/no-slip behavior by computing pre/post contact velocities with ptmath.surface_velocity and asserting either that relative contact velocity magnitude reduced to zero for no-slip or that the slip-case sign/direction matches the expected D_v?_t logic; choose a single concrete unequal-case that previously showed drift and add it as a regression test (use small tolerances and reference m1, m2, R1, R2, rvw1_f, rvw2_f, and ptmath.surface_velocity in the assertions).
🤖 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.
Nitpick comments:
In `@pooltool/physics/resolve/ball_ball/frictional_inelastic/__init__.py`:
- Around line 29-89: Add a small automated unit test that constructs an
unequal-ball collision (set m1 != m2 and R1 != R2), builds representative
rvw1/rvw2 input arrays, calls the collision routine in this frictional_inelastic
module to get rvw1_f and rvw2_f, and then verifies (1) linear momentum
conservation by checking m1*rvw1[1] + m2*rvw2[1] ≈ m1*rvw1_f[1] + m2*rvw2_f[1],
and (2) the slip/no-slip behavior by computing pre/post contact velocities with
ptmath.surface_velocity and asserting either that relative contact velocity
magnitude reduced to zero for no-slip or that the slip-case sign/direction
matches the expected D_v?_t logic; choose a single concrete unequal-case that
previously showed drift and add it as a regression test (use small tolerances
and reference m1, m2, R1, R2, rvw1_f, rvw2_f, and ptmath.surface_velocity in the
assertions).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 41e2f52a-ead8-4c77-a304-52a4d0173f82
📒 Files selected for processing (1)
pooltool/physics/resolve/ball_ball/frictional_inelastic/__init__.py
|
This is a welcome change, especially if you can establish equivalence to the r1=r2 & m1=m2 behavior. As I'm sure you've also reasoned, a full integration of unequal ball radii would involve at minimum updating ball-ball collision detection logic to account for differing radii. And ideally, the other ball-ball models would also have to account for different radii/mass -- although that could simply be a caveat of those models (they assume equal mass and radii, use at your own risk). That would be a reasonable concession. With what little time I'm finding recently, I'm personally prioritizing shipping 3D trajectories. This kind of matches the order of operations (as I see it), since unequal ball radii break the 2D plane assumption and can lead to some interesting effects in 3D. |
I just quickly checked normal velocity, I'll do the others when I get a chance.
I can take a quick look at what this would require. If it's simple enough, I can take a shot at it. In my own simulator, I do have 3D collision prediction working, and I imagine it's just something like substituting 2R for R_a + R_b somewhere.
The other models are:
I think the frictionless model would be very easy to update. It should be identical to the slip case from this model, but with e = 1, and mu = 0. I'm not really interested in updating the Mathavan model. I think it's feasible to update it to allow for different mass/radii, though. I think, at least for now, it should just be a requirement of the model that the balls are the same size and mass.
I don't think it really matters whether you add unequal radii or 3D first. Even though unequal radii can cause an airborne state, I think it's reasonable to just discard that z component like we do now. It's certainly another reason to motivate 3D trajectories, though! |
|
I just realized this isn't quite valid. For each ball's velocity and angular velocity, I rotate the x-axis around the z-axis to align it with the line of centers between the balls. This is basically a 2D rotation. Then I assume the contact normal direction is the x-axis, and the tangent direction is in the yz-plane. Now that the contact normal can be out of the xy-plane, these assumptions are no longer valid. The two ways to deal with it are:
I think I'll go w/ 2. to be consistent w/ what I did in the ball-cushion collision model. |
…tween balls of different sizes
…alls of different sizes
This is taken care of now. This has the benefit of making support for 3D as simple as not zeroing the velocity z components, here and here |
|
Looks like you've ironed everything else. I wasn't quite clear from your activity whether you established the m1=m2 and r1=r2 equivalence you were hoping to. Are there still discrepancies? And if so, how severe? |
I'm convinced mathematically it's equivalent. But I still get a different result and I don't know why. |
2 participants
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 #258). 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.
TODO:
I'm stumped by this one. I still see slightly different results in the plots compared to the original. I'm convinced that when
m1 == m2andR1 == R2, the result should be equivalent to the original.I also see that with the new version, I get equivalent results whether I use:
R1 == R2so normal is in XY-plane)I have the math worked out for doing this in 3D with different radii balls. It's not a big change. Going to hold off until we have a plan for the 3d branch.
Summary by CodeRabbit
Bug Fixes
Documentation