3D stick-ball resolver + a partially functional 3D playground#300
Conversation
- Easier to playtest partial 3D functionality
This reverts commit 591b12e.
WalkthroughThis PR activates 3D ball physics simulation by disabling unimplemented 3D collision detectors, splitting the stick-ball impact model into 2D and 3D variants handling vertical velocity differently, implementing full 3D velocity calculation using cue-strike angles, and demonstrating the capabilities with airborne ball scenarios. Changes3D Ball Physics Activation
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
🚥 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)
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #300 +/- ##
==========================================
+ Coverage 47.61% 47.68% +0.06%
==========================================
Files 158 158
Lines 10626 10642 +16
==========================================
+ Hits 5060 5075 +15
- Misses 5566 5567 +1
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pooltool/evolution/event_based/detect/ball_cushion.py (1)
122-125: ⚡ Quick winAdd docstring for consistency with the linear cushion 3D stub.
The
get_next_ball_linear_cushion_3d_eventstub at line 73 includes a helpful docstring explaining the placeholder behavior, but this circular-cushion variant does not. Adding a similar docstring improves code clarity and maintains consistency within the file.📝 Proposed docstring addition
def get_next_ball_circular_cushion_3d_event( shot: System, collision_cache: CollisionCache ) -> Event: + """3D ball-circular-cushion detection — not vendored yet; emits no event.""" return null_event(np.inf)🤖 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_cushion.py` around lines 122 - 125, Add a docstring to the get_next_ball_circular_cushion_3d_event function mirroring the style and content used in get_next_ball_linear_cushion_3d_event: describe that this is a stub/placeholder for circular cushion collision detection in 3D, explain that it currently returns a null_event(np.inf) (no collision) and note it should be implemented later; place the docstring immediately below the def line for get_next_ball_circular_cushion_3d_event.
🤖 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/stick_ball/core.py`:
- Around line 13-25: final_ball_motion_state currently uses a direct float
equality check rvw[1, 2] != 0.0 which misclassifies near-zero z-velocities;
change this to a tolerance-based test (e.g., use numpy.isclose or compare
abs(rvw[1,2]) > EPS where EPS is a small constant or derived from
np.finfo(float).eps) and keep returning const.airborne / const.sliding
accordingly; also address the unused parameter R by either removing it from
final_ball_motion_state or adding a brief comment explaining it is reserved for
future use.
---
Nitpick comments:
In `@pooltool/evolution/event_based/detect/ball_cushion.py`:
- Around line 122-125: Add a docstring to the
get_next_ball_circular_cushion_3d_event function mirroring the style and content
used in get_next_ball_linear_cushion_3d_event: describe that this is a
stub/placeholder for circular cushion collision detection in 3D, explain that it
currently returns a null_event(np.inf) (no collision) and note it should be
implemented later; place the docstring immediately below the def line for
get_next_ball_circular_cushion_3d_event.
🪄 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: a9c7ca03-e4a9-4a7f-bb31-44fd07259e13
📒 Files selected for processing (9)
pooltool/evolution/event_based/detect/ball_ball.pypooltool/evolution/event_based/detect/ball_cushion.pypooltool/evolution/event_based/detect/ball_pocket.pypooltool/physics/resolve/models.pypooltool/physics/resolve/resolver.pypooltool/physics/resolve/stick_ball/__init__.pypooltool/physics/resolve/stick_ball/core.pypooltool/physics/resolve/stick_ball/instantaneous_point/__init__.pysandbox/airborne_demos.py
| def final_ball_motion_state(rvw: NDArray[np.float64], R: float) -> int: | ||
| """Return the final (post-strike) motion state label. | ||
|
|
||
| If the z-velocity is non-zero the ball is considered airborne, otherwise | ||
| it is sliding (a struck ball is always kinetic). | ||
|
|
||
| Notes: | ||
| - A universal ``final_ball_motion_state`` fn could be a good idea. | ||
| """ | ||
| if rvw[1, 2] != 0.0: | ||
| return const.airborne | ||
|
|
||
| return const.sliding |
There was a problem hiding this comment.
Floating-point equality check is unreliable for classifying motion state.
rvw[1, 2] != 0.0 will fail for near-zero values arising from floating-point arithmetic (e.g., sin(0) producing ~1e-16). Balls intended to be sliding could be misclassified as airborne.
Use a tolerance-based check instead:
Proposed fix
- if rvw[1, 2] != 0.0:
+ if not np.isclose(rvw[1, 2], 0.0, atol=1e-12):
return const.airborneAdditionally, the parameter R is declared but never used in the function body. If it's reserved for future use (e.g., checking if z-position > R), consider removing it until needed, or add a comment explaining the intent.
🤖 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/stick_ball/core.py` around lines 13 - 25,
final_ball_motion_state currently uses a direct float equality check rvw[1, 2]
!= 0.0 which misclassifies near-zero z-velocities; change this to a
tolerance-based test (e.g., use numpy.isclose or compare abs(rvw[1,2]) > EPS
where EPS is a small constant or derived from np.finfo(float).eps) and keep
returning const.airborne / const.sliding accordingly; also address the unused
parameter R by either removing it from final_ball_motion_state or adding a brief
comment explaining it is reserved for future use.
1 participant
Summary
This PR splits the stick-ball resolver into 2D and 3D variants and stands up
enough of the surrounding infrastructure that a 3D-mode
SimulationEnginecan actually be constructed and exercised — at least for trajectories where
the only event types that need to fire are
BALL_TABLEand transitions.It does not vendor real 3D detection algorithms (those land per event type
in future PRs). Instead, the
*_3d_eventdetector stubs that previouslyraised
NotImplementedErrornow returnnull_event(np.inf). The result isa 3D engine that runs without exceptions, with the math-ready event types
firing correctly and the not-yet-ported ones simply contributing no
candidates. Enough to playtest airborne motion end-to-end.
What changed
Stick-ball resolver — 2D and 3D variants. The old
InstantaneousPointbecame
InstantaneousPoint3D: the canonical math, with the previously-zeroed
v·sin(θ)line uncommented so cue elevation produces real verticalvelocity.
InstantaneousPoint2Dis a thin subclass that callssuper().solve()and then clampsvz = 0(and re-classifies state viafinal_ball_motion_state, so the state label stays consistent with theclamped velocity). The
StickBallModelenum splits intoINSTANTANEOUS_POINT_2DandINSTANTANEOUS_POINT_3D;default_resolver()ships the 2D variant; resolver
VERSIONbumps 10 → 11.New sandbox:
sandbox/airborne_demos.py. Three demos:drop— handcrafted airborne ball at z=0.3 m with a small +x nudge.impulse_into— handcrafted airborne ball at table height with astrong downward strike plus +y nudge.
jump— a genuine cue strike withtheta=60°,V0=2,phi=90°,a=b=0. No handcrafted state:InstantaneousPoint3Dproduces theairborne motion from cue elevation.
Summary by CodeRabbit
New Features
Changes