Add ball-table resolver strategies#297
Merged
Merged
Conversation
WalkthroughThis PR introduces ball-table collision physics to the pooltool simulator. It adds a 2D-aware dimensionality validation framework, defines collision model types and core physics helpers, implements two collision strategies (frictionless and frictional inelastic), integrates collision handling into the event resolver, and provides comprehensive validation tests. ChangesBall-Table Collision System
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #297 +/- ##
==========================================
+ Coverage 47.45% 47.81% +0.36%
==========================================
Files 153 157 +4
Lines 10493 10631 +138
==========================================
+ Hits 4979 5083 +104
- Misses 5514 5548 +34
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
🤖 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/ball_table/core.py`:
- Around line 14-19: The function bounce_height currently divides by g without
validation; add an explicit guard in bounce_height to ensure g is positive (g >
0) and raise a clear ValueError (e.g., "gravity must be positive") when g is
zero or negative so the function cannot return non-physical results or crash
with a ZeroDivisionError.
🪄 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: d7ca49f9-0ed7-44d8-94de-a0da556e2334
📒 Files selected for processing (17)
pooltool/evolution/engine.pypooltool/evolution/event_based/config.pypooltool/objects/ball/params.pypooltool/physics/__init__.pypooltool/physics/dimensionality.pypooltool/physics/resolve/ball_table/__init__.pypooltool/physics/resolve/ball_table/core.pypooltool/physics/resolve/ball_table/frictional_inelastic/__init__.pypooltool/physics/resolve/ball_table/frictionless_inelastic/__init__.pypooltool/physics/resolve/models.pypooltool/physics/resolve/resolver.pypooltool/physics/resolve/serialize.pypooltool/physics/utils.pytests/evolution/test_engine.pytests/physics/resolve/ball_table/__init__.pytests/physics/resolve/ball_table/test_ball_table.pytests/physics/resolve/ball_table/test_frictionless_inelastic.py
Comment on lines
+14
to
+19
| def bounce_height(vz: float, g: float) -> float: | ||
| """Return how high a ball with outgoing positive z-velocity will bounce. | ||
|
|
||
| Measured as distance from table to bottom of ball. | ||
| """ | ||
| return 0.5 * vz**2 / g |
There was a problem hiding this comment.
Guard bounce_height against non-positive gravity.
bounce_height divides by g directly; g == 0 crashes and g < 0 yields non-physical output. Add an explicit validation guard.
Suggested fix
def bounce_height(vz: float, g: float) -> float:
@@
- return 0.5 * vz**2 / g
+ if g <= 0:
+ raise ValueError(f"g must be > 0, got {g}")
+ return 0.5 * vz**2 / g📝 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.
Suggested change
| def bounce_height(vz: float, g: float) -> float: | |
| """Return how high a ball with outgoing positive z-velocity will bounce. | |
| Measured as distance from table to bottom of ball. | |
| """ | |
| return 0.5 * vz**2 / g | |
| def bounce_height(vz: float, g: float) -> float: | |
| """Return how high a ball with outgoing positive z-velocity will bounce. | |
| Measured as distance from table to bottom of ball. | |
| """ | |
| if g <= 0: | |
| raise ValueError(f"g must be > 0, got {g}") | |
| return 0.5 * vz**2 / g |
🤖 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_table/core.py` around lines 14 - 19, The
function bounce_height currently divides by g without validation; add an
explicit guard in bounce_height to ensure g is positive (g > 0) and raise a
clear ValueError (e.g., "gravity must be positive") when g is zero or negative
so the function cannot return non-physical results or crash with a
ZeroDivisionError.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Introduce the ball-table event resolver. After
this PR,
Resolvercarries aball_tablestrategy field,BALL_TABLEeventshave a dispatch branch, and two ball-table collision models are importable. The
2D simulation is byte-identical to before — nothing currently generates
BALL_TABLEevents, so the new dispatch branch is dormant. A detection strategy would activate that.This builds on the previous PRs today (
SimulationEngineinfrastructure for 2D/3D modes, and vendored airborne primitives — motion state,BALL_TABLEevent type,parabolic evolution).
What's in this PR
Resolver package (
pooltool/physics/resolve/ball_table/):core.py—BallTableCollisionStrategyprotocol,CoreBallTableCollisionABC with
make_kiss+resolve, and the helpersbounce_heightandfinal_ball_motion_state. Ported verbatim from 3d except for the additionof
dim: Dimon the protocol.frictionless_inelastic/__init__.py—FrictionlessInelasticTable, thesimple
vz' = -e_t · vzmodel with a bounce-height cutoff to avoid thedichotomy paradox.
frictional_inelastic/__init__.py—FrictionalInelasticTable, therealistic frictional model (TP_A-14). Numba-jitted core with the slip /
no-slip branch.
__init__.py— registry and re-exports.Both strategies declare
dim = Dim.THREE— they only ever fire in 3D mode.The associated machinery (
DORMANT_IN_2D) is described below.New
BallTableModelenum (pooltool/physics/resolve/models.py):FRICTIONLESS_INELASTICandFRICTIONAL_INELASTIC. (Note: the 3d branch hada typo —
FRICTIONAL_ELASTICfor the frictionless variant — fixed here.)e_tonBallParams(pooltool/objects/ball/params.py):e_t: float = 0.5(ball-table coefficient of restitution), matching3d's default. Inserted between
e_bande_c.on_tablepredicate (pooltool/physics/utils.py):on_table(rvw, R) -> boolreturnsrvw[0, 2] == R. Numba-jitted. Consumedby
final_ball_motion_stateto distinguish "settled on table" from "stillairborne". Exact float equality is intentional —
make_kissexplicitly setsrvw[0, 2] = Rimmediately before this predicate runs.Resolver wiring (
pooltool/physics/resolve/resolver.py):ball_table: BallTableCollisionStrategy.BALL_TABLEdispatch branch inResolver.resolve.default_resolver()shipsFrictionalInelasticTable(min_bounce_height=0.005).VERSIONbumped 9 → 10. Existingresolver.yamlfiles will fail structure(missing
ball_tablekey) and be cleanly regenerated by the existingrecovery path.
Serialize hooks (
pooltool/physics/resolve/serialize.py):BallTableCollisionStrategy: ball_table_modelsadded to_model_map.Re-exports (
pooltool/physics/__init__.py):BallTableModel,ball_table_modelsadded to the public surface.Dim validator:
DORMANT_IN_2Dexclusion(
pooltool/evolution/event_based/config.py,pooltool/evolution/engine.py):DORMANT_IN_2D: frozenset[str] = frozenset({"ball_table"})—Resolver/EventDetector field names whose
dimis not validated against theengine's
is_3dwhenis_3d=False. These fields are dormant in 2Dbecause the detection layer doesn't emit their associated event types.
_validate_dimensionalitygains a single asymmetric skip:if not self.is_3d and field.name in DORMANT_IN_2D: continue. In 3D modethe field is still validated normally, so a misdeclared (e.g.
Dim.TWO)ball-table strategy is still caught.
INCLUDED_EVENTSbecause both answer "what does theevent-based simulator engage with?"
Dimdocstring refactor (pooltool/physics/dimensionality.py):("BOTH means the strategy behaves identically in either mode") was
misleading — a
Dim.BOTHstrategy may still take different code pathsdepending on its input (e.g. a branch on
state == const.airborneis deadin 2D and live in 3D). What matters is that no path is incorrect for the
mode it runs under. Strategies don't see
is_3ddirectly; they see onlythe inputs they're handed. The new docstring makes that explicit.
Why
Dim.THREE+DORMANT_IN_2Dfor ball-tableBall-table collisions only physically occur in 3D mode (no airborne state in
2D means no ball ever falls back to the table). The honest
dimtag istherefore
Dim.THREE. But theResolverdataclass requires aball_tablefield, which would force
SimulationEngine(is_3d=False)to fail validationon a
Dim.THREEstrategy.Two ways out were considered:
Dim.BOTHtag. Sidesteps validation by declaring the strategy is safein either mode. Works, but contrived: ball_table isn't actually "behaves
the same in either mode"; it's "doesn't run in 2D mode." The Dim contract
is the wrong level to encode that.
Dim.THREEtag + per-field exclusion in the validator. Keeps Dimhonest and captures the "mode-conditional" fact at the right layer —
in the validator, alongside the rest of the engine's mode logic.
We went with the second. The exclusion is asymmetric on purpose: in 2D the
slot is genuinely dormant and its tag is irrelevant; in 3D the tag matters
and is checked, catching
Dim.TWOmisdeclarations on what is fundamentallya 3D-only strategy.
Tests added (62 cases across 3 files)
tests/physics/resolve/ball_table/test_ball_table.py(53 cases, 8 tests):Pure helpers:
test_bounce_height(4 cases) —0.5·vz²/gfor various positivevzandg.test_bounce_height_negative_vz— symmetry undervz → -vz.Resolution invariants (parameterized over both models × 10 incoming speeds
spanning
−logspace(−5, 4, 10)):test_non_negative_output_velocity— outgoingvz ≥ 0after resolve.test_decaying_velocity— outgoing speed ≤ incoming speed (noe_t > 1).test_positive_incoming_velocity_fails—vz = +1raisesValueError.test_zero_incoming_velocity_fails—vz = 0raisesValueError.Dichotomy-paradox cutoff:
test_non_airborne_outgoing_state—vz = -0.001(belowmin_bounce_height)→ outgoing state is not
airborneandvzis exactly0. Validates thebounce-height floor that prevents perpetual bouncing.
tests/physics/resolve/ball_table/test_frictionless_inelastic.py(7 cases,2 tests):
Targets the private scalar
_resolve_ball_table(vz0, e_t) -> floatto lockin the exact arithmetic
−vz·e_t, independent of the strategy wrapper.tests/evolution/test_engine.py(2 new tests) — pin the asymmetricDORMANT_IN_2Dbehavior:test_dormant_ball_table_skipped_in_2d— a default 2D engine constructssuccessfully even though
ball_table.dim == Dim.THREE. The validatorskips the slot.
test_misdeclared_ball_table_still_caught_in_3d— flipping the ball_tablestrategy to
Dim.TWOin a 3D engine raisesValueErrornamingball_table.The exclusion is 2D-only.
Summary by CodeRabbit
New Features
Improvements
Tests