Delete detector strategy design#299
Merged
Merged
Conversation
WalkthroughThis PR refactors the collision detection system from an ChangesEvent Detection Architectural Refactor
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 |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #299 +/- ##
==========================================
- Coverage 47.87% 47.61% -0.27%
==========================================
Files 158 158
Lines 10658 10626 -32
==========================================
- Hits 5103 5060 -43
- Misses 5555 5566 +11
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: 5
🧹 Nitpick comments (1)
tests/evolution/test_engine.py (1)
77-83: ⚡ Quick winDocstring claims both modes, but only 2D is exercised.
This test says ball-table dim skipping is validated “in either mode”, but it only constructs
SimulationEngine(..., is_3d=False). Add a 3D construction assertion (or narrow the docstring) to keep intent and coverage aligned.Suggested update
def test_ball_table_exempt_from_dim_validation(): """Ball-table resolver strategies don't carry a `dim` attribute. The validator skips this field in either mode via SKIP_DIMENSION.""" resolver = SimulationEngine().resolver assert not hasattr(resolver.ball_table, "dim") SimulationEngine(resolver=resolver, is_3d=False) + SimulationEngine(resolver=resolver, is_3d=True)🤖 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 `@tests/evolution/test_engine.py` around lines 77 - 83, The docstring claims both modes are checked but the test only constructs a 2D engine; update the test to exercise 3D as well by constructing SimulationEngine(resolver=resolver, is_3d=True) and asserting the same behavior (e.g., assert not hasattr(resolver.ball_table, "dim")) so the "either mode" claim is true, or alternatively change the docstring to explicitly state it only checks 2D; locate the assertions around SimulationEngine, resolver, and ball_table.dim to apply the change.
🤖 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_cushion.py`:
- Around line 114-120: The code calls min(cache, key=...) without guarding for
an empty cache which will raise ValueError; update the circular cushion
detection (the block using variable cache and calling
ball_circular_cushion_collision) to first check if cache is empty (e.g. if not
cache: return None or the same no-collision sentinel used by the linear cushion
path), and only then compute ball_id, cushion_id = min(cache, key=lambda k:
cache[k]) and call ball_circular_cushion_collision with shot.balls[ball_id],
shot.table.cushion_segments.circular[cushion_id], and time=cache[(ball_id,
cushion_id)].
- Around line 61-67: The code assumes cache has entries and calls min(cache,
...) which will raise on empty shots; add a guard like in
get_next_ball_ball_2d_event: immediately return null_event(np.inf) if not cache
before computing obj_ids. Update the block that computes obj_ids and returns
ball_linear_cushion_collision (referencing variables obj_ids, cache,
ball_linear_cushion_collision, shot.balls, shot.table.cushion_segments.linear)
so it first checks "if not cache: return null_event(np.inf)" to avoid ValueError
on empty shots.
In `@pooltool/evolution/event_based/detect/ball_pocket.py`:
- Around line 48-54: The code calls min(cache, key=...) without checking for an
empty cache which raises ValueError when shot.balls is empty; update the logic
in ball_pocket.py (around where min(...) is used and the ball_pocket_collision
return occurs) to first check if cache is empty and return an appropriate
sentinel (e.g., None or the same "no collision" value used elsewhere) instead of
calling min; ensure the guard references the same variables (cache, shot.balls,
shot.table.pockets) and preserves the existing behavior when cache is non-empty
by then computing ball_id, pocket_id and calling ball_pocket_collision as
before.
In `@pooltool/evolution/event_based/detect/ball_table.py`:
- Around line 29-34: The code calls min(cache, key=...) without checking for an
empty cache, which raises ValueError when shot.balls is empty; add the same
empty-cache guard used in get_next_ball_ball_2d_event at the start of this
function (check if not cache or not shot.balls) and return the same sentinel
(e.g., None or the existing no-event value) instead of proceeding to min; ensure
the guard appears before computing obj_ids and before calling
ball_table_collision so the function safely handles tables with no balls.
In `@pooltool/evolution/event_based/introspection.py`:
- Around line 51-55: get_prospective_events currently ignores BALL_TABLE entries
in the CollisionCache so 3D prospective events are under-reported; update
get_prospective_events to detect cache keys for BALL_TABLE (keys like
(ball_id,)) and reconstruct events by calling or building
ball_table_collision(ball, time) for each cached (ball_id,) entry before
returning the event list. Locate the logic in get_prospective_events and the
CollisionCache handling, iterate cached entries where the bucket equals
BALL_TABLE, look up the corresponding Ball object by id and reconstruct the same
event shape used elsewhere (e.g., via ball_table_collision or equivalent
constructor), and include those reconstructed events in the returned prospective
events collection so BALL_TABLE candidates are not missed in 3D mode.
---
Nitpick comments:
In `@tests/evolution/test_engine.py`:
- Around line 77-83: The docstring claims both modes are checked but the test
only constructs a 2D engine; update the test to exercise 3D as well by
constructing SimulationEngine(resolver=resolver, is_3d=True) and asserting the
same behavior (e.g., assert not hasattr(resolver.ball_table, "dim")) so the
"either mode" claim is true, or alternatively change the docstring to explicitly
state it only checks 2D; locate the assertions around SimulationEngine,
resolver, and ball_table.dim to apply the change.
🪄 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: 475a2c42-60ad-4a23-a8d0-0e010cc4b2d1
📒 Files selected for processing (12)
pooltool/evolution/engine.pypooltool/evolution/event_based/detect/__init__.pypooltool/evolution/event_based/detect/ball_ball.pypooltool/evolution/event_based/detect/ball_cushion.pypooltool/evolution/event_based/detect/ball_pocket.pypooltool/evolution/event_based/detect/ball_table.pypooltool/evolution/event_based/detect/detector.pypooltool/evolution/event_based/detect/stick_ball.pypooltool/evolution/event_based/introspection.pytests/evolution/event_based/test_ball_table.pytests/evolution/event_based/test_simulate.pytests/evolution/test_engine.py
Comment on lines
+61
to
+67
| obj_ids = min(cache, key=lambda k: cache[k]) | ||
|
|
||
| return ball_linear_cushion_collision( | ||
| ball=shot.balls[obj_ids[0]], | ||
| cushion=shot.table.cushion_segments.linear[obj_ids[1]], | ||
| time=cache[obj_ids], | ||
| ) |
There was a problem hiding this comment.
Missing guard for empty cache will crash on tables with no balls.
If shot.balls is empty, the loop on lines 30-59 never executes and cache remains empty. Calling min() on an empty dict raises ValueError. This is inconsistent with get_next_ball_ball_2d_event which guards with if not cache: return null_event(np.inf).
🐛 Proposed fix
cache[obj_ids] = shot.t + dtau_E
+ if not cache:
+ return null_event(np.inf)
+
obj_ids = min(cache, key=lambda k: cache[k])
return ball_linear_cushion_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_cushion.py` around lines 61 - 67,
The code assumes cache has entries and calls min(cache, ...) which will raise on
empty shots; add a guard like in get_next_ball_ball_2d_event: immediately return
null_event(np.inf) if not cache before computing obj_ids. Update the block that
computes obj_ids and returns ball_linear_cushion_collision (referencing
variables obj_ids, cache, ball_linear_cushion_collision, shot.balls,
shot.table.cushion_segments.linear) so it first checks "if not cache: return
null_event(np.inf)" to avoid ValueError on empty shots.
Comment on lines
+114
to
+120
| ball_id, cushion_id = min(cache, key=lambda k: cache[k]) | ||
|
|
||
| return ball_circular_cushion_collision( | ||
| ball=shot.balls[ball_id], | ||
| cushion=shot.table.cushion_segments.circular[cushion_id], | ||
| time=cache[(ball_id, cushion_id)], | ||
| ) |
There was a problem hiding this comment.
Same missing guard for empty cache in circular cushion detection.
Same issue as linear cushion: min(cache, ...) on line 114 will raise ValueError if shot.balls is empty.
🐛 Proposed fix
cache[obj_ids] = shot.t + dtau_E
+ if not cache:
+ return null_event(np.inf)
+
ball_id, cushion_id = min(cache, key=lambda k: cache[k])
return ball_circular_cushion_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.
Suggested change
| ball_id, cushion_id = min(cache, key=lambda k: cache[k]) | |
| return ball_circular_cushion_collision( | |
| ball=shot.balls[ball_id], | |
| cushion=shot.table.cushion_segments.circular[cushion_id], | |
| time=cache[(ball_id, cushion_id)], | |
| ) | |
| if not cache: | |
| return null_event(np.inf) | |
| ball_id, cushion_id = min(cache, key=lambda k: cache[k]) | |
| return ball_circular_cushion_collision( | |
| ball=shot.balls[ball_id], | |
| cushion=shot.table.cushion_segments.circular[cushion_id], | |
| time=cache[(ball_id, cushion_id)], | |
| ) |
🤖 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 114 -
120, The code calls min(cache, key=...) without guarding for an empty cache
which will raise ValueError; update the circular cushion detection (the block
using variable cache and calling ball_circular_cushion_collision) to first check
if cache is empty (e.g. if not cache: return None or the same no-collision
sentinel used by the linear cushion path), and only then compute ball_id,
cushion_id = min(cache, key=lambda k: cache[k]) and call
ball_circular_cushion_collision with shot.balls[ball_id],
shot.table.cushion_segments.circular[cushion_id], and time=cache[(ball_id,
cushion_id)].
Comment on lines
+48
to
+54
| ball_id, pocket_id = min(cache, key=lambda k: cache[k]) | ||
|
|
||
| dtau_E = ball_pocket_collision_time( | ||
| rvw=state.rvw, | ||
| s=state.s, | ||
| a=pocket.a, | ||
| b=pocket.b, | ||
| r=pocket.radius, | ||
| mu=(params.u_s if state.s == const.sliding else params.u_r), | ||
| m=params.m, | ||
| g=params.g, | ||
| R=params.R, | ||
| ) | ||
| cache[obj_ids] = shot.t + dtau_E | ||
| return ball_pocket_collision( | ||
| ball=shot.balls[ball_id], | ||
| pocket=shot.table.pockets[pocket_id], | ||
| time=cache[(ball_id, pocket_id)], | ||
| ) |
There was a problem hiding this comment.
Missing guard for empty cache will crash on tables with no balls.
Same pattern as cushion detection: if shot.balls is empty, min(cache, ...) on line 48 raises ValueError.
🐛 Proposed fix
cache[obj_ids] = shot.t + dtau_E
+ if not cache:
+ return null_event(np.inf)
+
ball_id, pocket_id = min(cache, key=lambda k: cache[k])
return ball_pocket_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.
Suggested change
| ball_id, pocket_id = min(cache, key=lambda k: cache[k]) | |
| dtau_E = ball_pocket_collision_time( | |
| rvw=state.rvw, | |
| s=state.s, | |
| a=pocket.a, | |
| b=pocket.b, | |
| r=pocket.radius, | |
| mu=(params.u_s if state.s == const.sliding else params.u_r), | |
| m=params.m, | |
| g=params.g, | |
| R=params.R, | |
| ) | |
| cache[obj_ids] = shot.t + dtau_E | |
| return ball_pocket_collision( | |
| ball=shot.balls[ball_id], | |
| pocket=shot.table.pockets[pocket_id], | |
| time=cache[(ball_id, pocket_id)], | |
| ) | |
| if not cache: | |
| return null_event(np.inf) | |
| ball_id, pocket_id = min(cache, key=lambda k: cache[k]) | |
| return ball_pocket_collision( | |
| ball=shot.balls[ball_id], | |
| pocket=shot.table.pockets[pocket_id], | |
| time=cache[(ball_id, pocket_id)], | |
| ) |
🤖 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_pocket.py` around lines 48 - 54,
The code calls min(cache, key=...) without checking for an empty cache which
raises ValueError when shot.balls is empty; update the logic in ball_pocket.py
(around where min(...) is used and the ball_pocket_collision return occurs) to
first check if cache is empty and return an appropriate sentinel (e.g., None or
the same "no collision" value used elsewhere) instead of calling min; ensure the
guard references the same variables (cache, shot.balls, shot.table.pockets) and
preserves the existing behavior when cache is non-empty by then computing
ball_id, pocket_id and calling ball_pocket_collision as before.
Comment on lines
+29
to
+34
| obj_ids = min(cache, key=lambda k: cache[k]) | ||
|
|
||
| return ball_table_collision( | ||
| ball=shot.balls[obj_ids[0]], | ||
| time=cache[obj_ids], | ||
| ) | ||
| return ball_table_collision( | ||
| ball=shot.balls[obj_ids[0]], | ||
| time=cache[obj_ids], | ||
| ) |
There was a problem hiding this comment.
Missing guard for empty cache will crash on tables with no balls.
Same pattern: if shot.balls is empty, min(cache, ...) on line 29 raises ValueError. Add an empty-cache guard for consistency with get_next_ball_ball_2d_event.
🐛 Proposed fix
cache[obj_ids] = shot.t + dtau_E
+ if not cache:
+ from pooltool.events import null_event
+ import numpy as np
+ return null_event(np.inf)
+
obj_ids = min(cache, key=lambda k: cache[k])
return ball_table_collision(Alternatively, add the imports at module level:
from pooltool.events import Event, EventType, ball_table_collision
+from pooltool.events import null_event
+import numpy as npThen the guard simplifies to:
+ if not cache:
+ 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_table.py` around lines 29 - 34,
The code calls min(cache, key=...) without checking for an empty cache, which
raises ValueError when shot.balls is empty; add the same empty-cache guard used
in get_next_ball_ball_2d_event at the start of this function (check if not cache
or not shot.balls) and return the same sentinel (e.g., None or the existing
no-event value) instead of proceeding to min; ensure the guard appears before
computing obj_ids and before calling ball_table_collision so the function safely
handles tables with no balls.
Comment on lines
+51
to
+55
| # TODO: BALL_TABLE entries in the cache are not reconstructed here. In 2D | ||
| # mode this is harmless (the detector doesn't populate the BALL_TABLE | ||
| # bucket). When 3D activation lands, prospective BALL_TABLE events will be | ||
| # silently missed from get_prospective_events(). Add a branch that builds | ||
| # ball_table_collision(ball, time) from each (ball_id,) key. |
There was a problem hiding this comment.
Reconstruct BALL_TABLE cache entries so prospective events stay complete in 3D.
get_prospective_events() currently omits cached BALL_TABLE candidates, so introspection silently under-reports events when 3D mode is used.
Proposed fix
from pooltool.events import (
Event,
EventType,
ball_ball_collision,
+ ball_table_collision,
ball_circular_cushion_collision,
ball_linear_cushion_collision,
ball_pocket_collision,
stick_ball_collision,
)
@@
if EventType.BALL_POCKET in cache.times:
for (ball_id, pocket_id), time in cache.times[EventType.BALL_POCKET].items():
events.append(
ball_pocket_collision(
ball=system.balls[ball_id],
pocket=system.table.pockets[pocket_id],
time=time,
)
)
+
+ if EventType.BALL_TABLE in cache.times:
+ for (ball_id,), time in cache.times[EventType.BALL_TABLE].items():
+ events.append(
+ ball_table_collision(
+ ball=system.balls[ball_id],
+ time=time,
+ )
+ )If you want, I can also draft a focused test that asserts BALL_TABLE appears in get_prospective_events() when present in CollisionCache.
🤖 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/introspection.py` around lines 51 - 55,
get_prospective_events currently ignores BALL_TABLE entries in the
CollisionCache so 3D prospective events are under-reported; update
get_prospective_events to detect cache keys for BALL_TABLE (keys like
(ball_id,)) and reconstruct events by calling or building
ball_table_collision(ball, time) for each cached (ball_id,) entry before
returning the event list. Locate the logic in get_prospective_events and the
CollisionCache handling, iterate cached entries where the bucket equals
BALL_TABLE, look up the corresponding Ball object by id and reconstruct the same
event shape used elsewhere (e.g., via ball_table_collision or equivalent
constructor), and include those reconstructed events in the returned prospective
events collection so BALL_TABLE candidates are not missed in 3D mode.
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.
Detection layer redesign — collapse strategy classes to functions, dispatch on
is_3dSummary
This PR rewrites the detection layer of the event-based simulator. The
*Detectionstrategy classes and their Protocols are gone. In their place areplain module-level functions, dispatched from a single
EventDetectorclassthat knows whether the engine is in 2D or 3D mode. The math layer
(
physics/motion/solve.py) is untouched, simulation behavior in 2D isbyte-identical to before, and the full test suite stays green.
Why
The previous architecture wrapped detection in the same strategy-pattern
machinery as resolution: Protocols, attrs strategy classes,
dimcapabilitydeclarations, pluggable fields on a bundle. But the two layers serve very
different design pressures, and applying one pattern to both was costing more
than it was earning.
Resolution genuinely has model variety. There's a Han 2005 cushion model and a
Mathavan 2010 cushion model. There's a frictional inelastic table model and a
frictionless inelastic one. Users plug different models in, swap them via a
YAML config, and write their own. The strategy pattern earns its weight there.
Detection doesn't work like that. There is exactly one canonical algorithm per
(event type, simulation mode). The strategy pattern was just
ceremony: one implementer per Protocol, classes that held no state beyond a
dimtag, registries with one entry. None of the extensibility it promisedwas being used or was even meaningful to use.
The strain became visible when planning 3D vendoring. The 3d branch has
substantively different detection algorithms for several event types — most
notably ball-pocket, which uses a cylinder-plus-height-threshold approach when
the incoming ball is airborne, versus the friction-driven quartic in 2D. Under
the old architecture, supporting that meant either two strategy classes per
forking event type (
BallPocketDetection2DwithDim.TWO, thenBallPocketDetection3DwithDim.THREE, bundled selectively by the engine),or threading an
is_3dflag down through every strategy method. Neither feltright. The first doubles the strategy count for an extension point that
doesn't exist; the second smears mode-awareness throughout the detection
layer rather than concentrating it.
What changed
The detection layer is now plain functions. For event types whose algorithm
depends on simulation mode (
ball_ball,ball_cushion,ball_pocket),each module exports
get_next_*_2d_eventandget_next_*_3d_event. Formode-invariant types (
stick_ball,ball_table), the module exports asingle
get_next_*_event. The function name carries the mode contract; thefunction body assumes its mode is correct and doesn't second-guess it.
EventDetectorbecomes a single-field class — justis_3d: bool. Itsget_next_eventmethod is the one place where mode branching happens indetection: an explicit
if self.is_3d / elseblock that picks the rightfunction for each candidate. After that branch, every function call is
mode-pure. Reading the body of
get_next_ball_pocket_2d_event, you don'tneed to think about whether the engine is 3D — by name and by guarantee,
it isn't.
The
SimulationEngineAPI simplifies as a side effect:detectorisinit=False, built automatically fromself.is_3d. Users can no longerconstruct a detector with a mismatched mode, because they can't construct
one at all — they specify
is_3don the engine and the detector is derived.The
Dimtaxonomy andSKIP_DIMENSIONconstant survive, but they now applyonly to the resolver layer, where strategy variety is real and the
"is this strategy safe in this mode?" question has teeth. The validator's
inner loop over
EventDetectorfields disappears.Summary by CodeRabbit
Refactor
Tests