SimulationEngine infrastructure for 2D/3D modes#294
Merged
Conversation
- The simulation (event_based/) should not know about details like the shape of the collision surface
- Moved to evolution/
- Matches `resolver`
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
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:
WalkthroughThis PR refactors pooltool's event-based simulation architecture from function-centric event handling to a strategy-based ChangesCore SimulationEngine and Physics Foundations
Physics Strategy Dimensionality Declarations
Event-Based Simulation Refactor
Physics Utilities Migration and Module Updates
Tests and Documentation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 #294 +/- ##
==========================================
+ Coverage 46.36% 47.45% +1.09%
==========================================
Files 144 153 +9
Lines 10314 10479 +165
==========================================
+ Hits 4782 4973 +191
+ Misses 5532 5506 -26
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: 2
🧹 Nitpick comments (1)
pooltool/physics/utils.py (1)
128-151: ⚡ Quick winRename the inverse-transform input to match semantics.
Line 129 and Line 141 describe different inputs; this helper should take a contact-point offset and return cue-center offset. Aligning the parameter/doc names will make call sites less error-prone.
♻️ Proposed fix
`@jit`(nopython=True, cache=const.use_numba_cache) def tip_center_offset( - tip_center_offset: NDArray[np.float64], tip_radius: float, ball_radius: float + contact_point_offset: NDArray[np.float64], tip_radius: float, ball_radius: float ) -> NDArray[np.float64]: @@ - Args: - cue_center_offset: - A 2D vector (e.g., [a, b]) representing the offset of the cue tip center - relative to the ball center (normalized by the ball's radius). + Args: + contact_point_offset: + A 2D vector (e.g., [a, b]) representing the contact point offset on the + ball's surface (normalized by the ball's radius). @@ - return tip_center_offset * (1 + tip_radius / ball_radius) + return contact_point_offset * (1 + tip_radius / ball_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/utils.py` around lines 128 - 151, The function tip_center_offset currently names its input tip_center_offset but actually expects a contact-point offset and returns the cue tip center offset; rename the input parameter to reflect this (e.g., contact_offset or tip_contact_offset) and update the docstring Arg name and description to match, keeping the computation tip_center = contact_offset * (1 + tip_radius/ball_radius) and the function name tip_center_offset unchanged so callers remain clear; ensure the return docstring describes the cue tip center offset (normalized) and update any internal references to the old parameter name.
🤖 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 `@docs/resources/custom_physics.md`:
- Line 202: Update the broken markdown anchor for the SimulationEngine reference
used in the Dim.TWO line: replace the old anchor target
"`#pooltool.evolution.engine.SimulationEngine`" with the exported API path anchor
"`#pooltool.evolution.SimulationEngine`" (so the link from the Dim.TWO description
points to pooltool.evolution.SimulationEngine). Locate the text mentioning
Dim.TWO and adjust the link target accordingly to the exported SimulationEngine
symbol.
In `@pooltool/evolution/engine.py`:
- Around line 43-51: The current code bypasses dimensionality checks for
non-attrs strategies by using attrs.has(type(strategy)); remove that attrs.has
gate so validation always runs: in the block referencing strategy, bundle, field
and Dim, first ensure the strategy exposes a dim attribute (keep the
hasattr(strategy, "dim") check and its AttributeError using bundle and field),
then validate that strategy.dim is in (required, Dim.BOTH) and raise the same
error if not; do not rely on attrs.has(type(strategy)) so custom non-attrs
strategies can't skip the dim checks.
---
Nitpick comments:
In `@pooltool/physics/utils.py`:
- Around line 128-151: The function tip_center_offset currently names its input
tip_center_offset but actually expects a contact-point offset and returns the
cue tip center offset; rename the input parameter to reflect this (e.g.,
contact_offset or tip_contact_offset) and update the docstring Arg name and
description to match, keeping the computation tip_center = contact_offset * (1 +
tip_radius/ball_radius) and the function name tip_center_offset unchanged so
callers remain clear; ensure the return docstring describes the cue tip center
offset (normalized) and update any internal references to the old parameter
name.
🪄 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: 1614b63d-681c-4b55-bae3-92a9dcb9a6b8
📒 Files selected for processing (52)
docs/examples/30_degree_rule.ipynbdocs/include_exclude.pydocs/meta/developer_guide.mddocs/resources/custom_physics.mdpooltool/ani/hud.pypooltool/ani/modes/aim.pypooltool/ani/modes/view.pypooltool/evolution/__init__.pypooltool/evolution/engine.pypooltool/evolution/event_based/_utils.pypooltool/evolution/event_based/cache.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/detector.pypooltool/evolution/event_based/detect/stick_ball.pypooltool/evolution/event_based/introspection.pypooltool/evolution/event_based/simulate.pypooltool/objects/cue/render.pypooltool/objects/table/components.pypooltool/physics/__init__.pypooltool/physics/dimensionality.pypooltool/physics/engine.pypooltool/physics/evolve/__init__.pypooltool/physics/motion/__init__.pypooltool/physics/motion/solve.pypooltool/physics/resolve/ball_ball/core.pypooltool/physics/resolve/ball_ball/friction.pypooltool/physics/resolve/ball_ball/frictional_inelastic/__init__.pypooltool/physics/resolve/ball_ball/frictional_mathavan/__init__.pypooltool/physics/resolve/ball_ball/frictionless_elastic/__init__.pypooltool/physics/resolve/ball_cushion/core.pypooltool/physics/resolve/ball_cushion/han_2005/model.pypooltool/physics/resolve/ball_cushion/impulse_frictional_inelastic/model.pypooltool/physics/resolve/ball_cushion/mathavan_2010/model.pypooltool/physics/resolve/ball_cushion/stronge_compliant/model.pypooltool/physics/resolve/ball_cushion/unrealistic/__init__.pypooltool/physics/resolve/ball_pocket/__init__.pypooltool/physics/resolve/sphere_half_space_collision.pypooltool/physics/resolve/stick_ball/core.pypooltool/physics/resolve/stick_ball/instantaneous_point/__init__.pypooltool/physics/resolve/transition/__init__.pypooltool/physics/utils.pypooltool/ptmath/__init__.pypooltool/ptmath/utils.pytests/evolution/event_based/test_simulate.pytests/evolution/test_engine.pytests/physics/resolve/ball_ball/test_ball_ball.pytests/physics/resolve/ball_ball/test_frictional_mathavan.pytests/physics/resolve/ball_cushion/test_ball_cushion.pytests/physics/test_utils.py
💤 Files with no reviewable changes (3)
- pooltool/physics/engine.py
- pooltool/ptmath/init.py
- pooltool/ptmath/utils.py
|
|
||
| `Dim` is a capability declaration consumed at engine construction: | ||
|
|
||
| - `Dim.TWO` — your model is safe only when [](#pooltool.evolution.engine.SimulationEngine)'s `is_3d` is `False`. Use this if your model assumes balls are on the table surface (z=R, vz=0). |
There was a problem hiding this comment.
Fix broken SimulationEngine cross-reference target.
This anchor points to pooltool.evolution.engine.SimulationEngine, but that module is now excluded from AutoAPI. Link to the exported API path (pooltool.evolution.SimulationEngine) instead.
🔧 Suggested patch
-- `Dim.TWO` — your model is safe only when [](`#pooltool.evolution.engine.SimulationEngine`)'s `is_3d` is `False`. Use this if your model assumes balls are on the table surface (z=R, vz=0).
+- `Dim.TWO` — your model is safe only when [](`#pooltool.evolution.SimulationEngine`)'s `is_3d` is `False`. Use this if your model assumes balls are on the table surface (z=R, vz=0).📝 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
| - `Dim.TWO` — your model is safe only when [](#pooltool.evolution.engine.SimulationEngine)'s `is_3d` is `False`. Use this if your model assumes balls are on the table surface (z=R, vz=0). | |
| - `Dim.TWO` — your model is safe only when [](`#pooltool.evolution.SimulationEngine`)'s `is_3d` is `False`. Use this if your model assumes balls are on the table surface (z=R, vz=0). |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 202-202: Link fragments should be valid
(MD051, link-fragments)
🤖 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 `@docs/resources/custom_physics.md` at line 202, Update the broken markdown
anchor for the SimulationEngine reference used in the Dim.TWO line: replace the
old anchor target "`#pooltool.evolution.engine.SimulationEngine`" with the
exported API path anchor "`#pooltool.evolution.SimulationEngine`" (so the link
from the Dim.TWO description points to pooltool.evolution.SimulationEngine).
Locate the text mentioning Dim.TWO and adjust the link target accordingly to the
exported SimulationEngine symbol.
Comment on lines
+43
to
+51
| if not attrs.has(type(strategy)): | ||
| continue | ||
| if not hasattr(strategy, "dim"): | ||
| raise AttributeError( | ||
| f"{type(bundle).__name__}.{field.name} " | ||
| f"({type(strategy).__name__}) is missing required " | ||
| f"'dim' attribute" | ||
| ) | ||
| if strategy.dim not in (required, Dim.BOTH): |
There was a problem hiding this comment.
Dimensionality validation is bypassable for non-attrs strategy implementations.
Line 43 skips validation when a strategy instance is not an attrs class. That allows incompatible custom strategies to pass without dim checks, despite the engine contract.
Suggested fix
- if not attrs.has(type(strategy)):
- continue
if not hasattr(strategy, "dim"):
raise AttributeError(
f"{type(bundle).__name__}.{field.name} "
f"({type(strategy).__name__}) is missing required "
f"'dim' attribute"
)📝 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
| if not attrs.has(type(strategy)): | |
| continue | |
| if not hasattr(strategy, "dim"): | |
| raise AttributeError( | |
| f"{type(bundle).__name__}.{field.name} " | |
| f"({type(strategy).__name__}) is missing required " | |
| f"'dim' attribute" | |
| ) | |
| if strategy.dim not in (required, Dim.BOTH): | |
| if not hasattr(strategy, "dim"): | |
| raise AttributeError( | |
| f"{type(bundle).__name__}.{field.name} " | |
| f"({type(strategy).__name__}) is missing required " | |
| f"'dim' attribute" | |
| ) | |
| if strategy.dim not in (required, Dim.BOTH): |
🤖 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/engine.py` around lines 43 - 51, The current code bypasses
dimensionality checks for non-attrs strategies by using
attrs.has(type(strategy)); remove that attrs.has gate so validation always runs:
in the block referencing strategy, bundle, field and Dim, first ensure the
strategy exposes a dim attribute (keep the hasattr(strategy, "dim") check and
its AttributeError using bundle and field), then validate that strategy.dim is
in (required, Dim.BOTH) and raise the same error if not; do not rely on
attrs.has(type(strategy)) so custom non-attrs strategies can't skip the dim
checks.
This was referenced May 18, 2026
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.
SimulationEngine infrastructure for 2D/3D modes
Summary
This is the first concrete step against the plan I'd settled on: stop trying to rebase the
3dbranch, vendor pieces intomainbehind a flag instead. The flag's semantic boundary is whether the airborne motion state is reachable — if no model can produce vz, balls just never leave the table, BALL_TABLE events never fire, airborne branches are dead code. That collapses 2D vs 3D to one boolean on the engine that already wraps the resolver and gets passed tosimulate().This PR delivers the infrastructure for that flag. No 3D physics yet — but the slot is built, tagged, and validated.
What's in this PR
The flag itself (
SimulationEngine.is_3d: bool = False):Dimcapability —TWO,THREE, orBOTH.Dim.BOTHis a strict promise: the strategy behaves the same for both (it doesn't know what mode the simulator is in). If a strategy would behave differently across modes, it's two strategies, not oneDim.BOTH.SimulationEngine.__attrs_post_init__validates that every bundled strategy is compatible withis_3d. Incompatible pairings raise with a message naming the offending strategy.EventDetector as a peer to Resolver in
pooltool/evolution/event_based/detect/:stick_ball,ball_ball,ball_linear_cushion,ball_circular_cushion,ball_pocket).EventDetector.get_next_event()is the orchestration method. The old free-functionget_next_eventandget_next_*collection insimulate.pyare deleted._SimulationState.step→self.engine.detector.get_next_event(...).Architectural moves falling out of the above:
PhysicsEngine→SimulationEngine, moved frompooltool/physics/engine.pytopooltool/evolution/engine.py. It bundles detection now, so "physics" is no longer accurate.solve.py→pooltool/physics/motion/solve.py. Low-level numerics belong in physics, not in the event loop.rel_velocity,get_slide_time,get_ball_energy, etc.) moved frompooltool/ptmath/utils.pytopooltool/physics/utils.py. They consumervwarrays — they're physics, not pure math. This was already done in the 3d branch._system_has_energylives inpooltool/evolution/event_based/_utils.py— the layer that depends on both system and physics.What's NOT in this PR
Dim.THREEstrategy implementationsBALL_TABLEevent type or its detector/resolverairbornemotion state inpooltool/constants.py3dbranch beyond architectural decisionsEach of those is to be delivered in follow-up PRs that lands independently against the infrastructure in this one.
Things refined vs. the original plan
A couple of design points evolved during implementation:
default_2d()/default_3d()factories on SimulationEngine. I'd originally pitched these; rejected because Resolver is user-owned via~/.config/pooltool/physics/resolver.yaml, and a factory would override user customization. Users pick resolver models independently; SimulationEngine just validates the bundle is internally consistent with itsis_3dsetting.evolve/__init__.pyandsolve.pyinto per-state modules (sliding/rolling/spinning each owning both forward-step and collision-time math). Rejected for this PR — high-cost change with relatively weak payoff.solve.pywas relocated intophysics/motion/to leave room for the unification later if desired.Summary by CodeRabbit
New Features
SimulationEnginefor pluggable strategy management with automatic dimensionality validationDim) system to declare physics strategy compatibilityDocumentation
SimulationEngineusageRefactor