Vision: Refactored project, Added body tracking, Fixed constraints, Improved performance #15
Conversation
Added updates section with recent changes and links.
Add YouTube link for project demonstration
📝 WalkthroughWalkthroughThis pull request refactors the SharpPhysics physics engine from a static, window-bound architecture to an instance-based, game-loop abstraction system, and introduces a comprehensive demo project with multiple playable games, pose-tracking integration, and a complete UI framework. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Program.cs
participant Engine as GameEngine
participant Game as IGame
participant Physics as PhysicsSystem
participant Renderer
Main->>Engine: new GameEngine(width, height, title, MenuGame)
Main->>Engine: Run()
Engine->>Game: Initialize(engine)
activate Game
Game->>Physics: Create objects/constraints
Game->>Renderer: Setup UI
deactivate Game
loop Main Game Loop
Engine->>Engine: Check pending game switches
alt Game Switch Pending
Engine->>Game: Shutdown()
Engine->>Physics: Clear all objects
Engine->>Game: Initialize(engine) [new game]
end
Engine->>Physics: Update(deltaTime * TimeScale)
Engine->>Game: Update(deltaTime, keyState)
Engine->>Renderer: Render physics objects
Engine->>Game: Render(renderer) [overlays]
Engine->>Renderer: Display()
end
Note over Engine,Renderer: On window close or ESC
Engine->>Game: Shutdown()
Engine->>Renderer: Cleanup
sequenceDiagram
participant User as User Input
participant InputMgr as InputManager
participant KeyState as KeyState
participant Game as IGame
User->>InputMgr: Mouse/Keyboard events
activate InputMgr
InputMgr->>InputMgr: Buffer key presses with timer
InputMgr->>InputMgr: Track edge states
deactivate InputMgr
Game->>InputMgr: GetKeyState()
activate InputMgr
InputMgr->>KeyState: Create snapshot
KeyState-->>InputMgr: Held + Pressed states
deactivate InputMgr
InputMgr-->>Game: KeyState with buffered presses
Game->>Game: Process input per buffered key
Game->>InputMgr: ConsumePress(key) [optional]
InputMgr->>InputMgr: Remove from buffer
sequenceDiagram
participant PersonCollider as PersonColliderBridge
participant YoloDetector as YOLO Model
participant Camera as Camera/MJPEG
participant Physics as PhysicsSystem
participant MainThread as Main Thread
PersonCollider->>Camera: Start background frame capture
loop Background Detection Thread
PersonCollider->>Camera: Grab frame
PersonCollider->>YoloDetector: Detect pose (17 keypoints)
PersonCollider->>PersonCollider: Enqueue detection result
end
MainThread->>PersonCollider: ProcessPendingUpdates()
activate PersonCollider
PersonCollider->>PersonCollider: Match detections to TrackedPerson
loop Per Person
PersonCollider->>PersonCollider: Apply smoothing to keypoints
PersonCollider->>Physics: Update head/hand ball positions
end
PersonCollider-->>MainThread: OnPersonBodyUpdated event
deactivate PersonCollider
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
physics/Engine/PhysicsSystem.cs (1)
229-239: Potential NullReferenceException inRemoveActiveObject.
ActiveObjectcould be null when this method is called, but it's passed toContains()andEnqueue()without null checks. This will throw if called when no object is active.🐛 Suggested fix
public void RemoveActiveObject() { + if (ActiveObject == null) + return; + if (ListGravityObjects.Contains(ActiveObject)) { ListGravityObjects.Remove(ActiveObject); } // Add to removal queue for proper removal RemovalQueue.Enqueue(ActiveObject); ActiveObject = null; }
🤖 Fix all issues with AI agents
In `@physics/Engine/Classes/CollisionPair.cs`:
- Around line 33-37: CollisionPair.Reset() sets A and B to null but Equals() and
GetHashCode() call A.Equals(...) and A.GetHashCode() directly, which can throw
NullReferenceException; update CollisionPair.Equals(...) and
CollisionPair.GetHashCode() to defensively handle nulls by checking A and B for
null (use ReferenceEquals or explicit null checks) before calling
Equals/GetHashCode, return appropriate fallback values (e.g., false for
comparisons when the other operand isn't matching or 0/combined hash when fields
are null) and keep equality symmetrical and consistent with GetHashCode.
In `@physics/Engine/Classes/Objects/PhysicsObject.cs`:
- Line 29: The Friction property was made static which forces a global value and
prevents per-object tuning; revert to an instance property on PhysicsObject and
keep a static default constant for new instances. Replace the static Friction
with an instance property named Friction (get/set) initialized to a static
DefaultFriction (or DefaultFrictionValue) set to 0.5f, and update
constructors/usages that referenced PhysicsObject.Friction to use the instance
property (obj.Friction) or the static DefaultFriction when creating new objects.
In `@physics/Engine/Classes/Templates/ActionTemplates.cs`:
- Around line 35-46: PopAndMultiply iterates _physicsSystem.ListStaticObjects
while calling _physicsSystem.RemoveActiveObject(), which can mutate the
collection; fix by iterating a snapshot copy instead of the live collection:
capture the objects into a separate list/array (e.g., call ToList() or copy into
a new List<PhysicsObject>) and loop over that snapshot in PopAndMultiply,
keeping the existing ActivateAtPoint/RemoveActiveObject/SetVelocity calls
(referencing PopAndMultiply, _physicsSystem.ListStaticObjects,
RemoveActiveObject, ActivateAtPoint, SetVelocity, and
_objectTemplates.CreateSmallBall to locate the code).
- Around line 19-25: The Launch method's physObj parameter is unused; either
remove the parameter from Launch(PhysicsObject physObj, Vector2 StartPointF,
Vector2 EndPointF) if activation is meant to be location-only, or update the
method to activate and apply velocity to the provided PhysicsObject instead of
activating by StartPointF: replace the call to
_physicsSystem.ActivateAtPoint(StartPointF) with the appropriate activation call
for the object (e.g., activate/mark physObj in the physics system) and ensure
_physicsSystem.AddVelocityToActive applies the computed delta*2 to that
activated physObj; keep the existing delta calculation (from StartPointF to
EndPointF) and pass or associate the velocity with physObj so physObj is
actually used.
In `@physics/Engine/Collision.cs`:
- Around line 338-344: The penetration calculation for an inside-circle case is
wrong: in the block guarded by "if (!circleIsOutside)" (where you already flip
normal = -normal), change the penetration assignment from m.Penetration = radius
+ dist to m.Penetration = radius - dist so penetration equals radius minus
distance (same magnitude as the outside case) with the flipped normal; keep the
normal flip and variable names (circleIsOutside, normal, radius, dist,
m.Penetration) as-is.
In `@physics/Engine/Rendering/UI/UiCheckbox.cs`:
- Around line 34-44: The RectangleShape created in UiCheckbox.DrawSelf is not
disposed (same leak as in UiSlider); update DrawSelf to dispose the
RectangleShape after drawing (e.g., wrap the new RectangleShape(...) in a using
or call Dispose() after target.Draw(rect)) so the temporary
SFML.Graphics.RectangleShape is properly released.
In `@physics/Engine/Rendering/UI/UiElement.cs`:
- Around line 73-95: HandleClick currently forwards the world clickPos to
Children in insertion order, causing incorrect hit tests for positioned children
and clicks hitting elements under overlapping siblings; modify HandleClick to
iterate Children in reverse order and for each child convert the incoming
clickPos into the child's local coordinate space before calling
child.HandleClick (e.g. compute childLocal = clickPos - child.Position or call
child.WorldToLocal/Transform method if available), preserving the existing
IUiClickable check for this element after children are checked.
In `@physics/Engine/Rendering/UI/UiManager.cs`:
- Around line 58-75: UiManager.HandleClick currently treats any element in
_elements that returns true from UiElement.HandleClick as the clicked element,
which causes _draggedElement to be set to the container instead of the actual
child that handled the click; change the contract so UiElement.HandleClick
returns the actual UiElement that handled the click (or null) instead of bool,
update UiManager.HandleClick to call element.HandleClick(clickPos) and if it
returns a non-null handledElement use that handledElement (and check
handledElement is IUiDraggable) to set _draggedElement, and update any other
callers of UiElement.HandleClick to the new signature accordingly.
In `@physics/Engine/Rendering/UI/UiMenuButton.cs`:
- Around line 11-20: UiMenuButton currently holds SFML resources (_background,
_border, _titleText, _descriptionText) but does not implement IDisposable; add
the same disposal pattern used by UiTextLabel: implement IDisposable on the
UiMenuButton class, add a public void Dispose() that calls a protected virtual
void Dispose(bool disposing), in Dispose(bool) if disposing call Dispose() on
_background, _border, _titleText and _descriptionText (null-checking them and
then setting fields to null), call GC.SuppressFinalize(this) in the public
Dispose, and (optionally) add a finalizer that calls Dispose(false) if
UiTextLabel uses one — this ensures SFML shapes and texts are properly cleaned
up when the button is destroyed.
In `@physics/Engine/Rendering/UI/UiRoundedRectangle.cs`:
- Around line 136-140: The override of UiRoundedRectangle.HandleClick currently
returns false unconditionally which blocks child click traversal; change the
implementation to delegate to the base method (i.e., call
base.HandleClick(clickPos) and return its result) or remove the override
entirely so children can receive clicks—update the HandleClick method in class
UiRoundedRectangle to call base.HandleClick(clickPos) instead of returning
false.
In `@physics/Engine/Rendering/UI/UiSlider.cs`:
- Around line 48-51: GetNormalizedPosition() can divide by zero when MinValue ==
MaxValue; update the UiSlider class to either validate inputs in the constructor
(throw ArgumentException if MaxValue == MinValue) or make
GetNormalizedPosition() defensive by checking if (MaxValue - MinValue) == 0 and
returning a sensible default (e.g., 0f or 0.5f) instead of performing the
division; locate the GetNormalizedPosition method and the UiSlider constructor
to apply one of these fixes and ensure callers still get a well-defined
normalized value or a clear validation exception.
- Around line 109-142: In DrawSelf (UiSlider.DrawSelf) the three RectangleShape
instances (background, filled, handle) are created each frame but not disposed,
leaking native SFML resources; wrap each RectangleShape's lifetime in a using
(or call Dispose after target.Draw) so background, filled and handle are
disposed immediately after drawing (retain GetNormalizedPosition and target.Draw
calls but ensure Dispose/using around the RectangleShape creation).
In `@physics/SharpPhysics.csproj`:
- Around line 4-12: The project file SharpPhysics.csproj declares RootNamespace
and AssemblyName as SharpPhysics.Engine which conflicts with existing source
files using the physics.Engine namespace; fix by making them consistent: either
change all source file namespaces from physics.Engine to SharpPhysics.Engine
(update namespace declarations in classes across physics, Demo, and Tests) or
revert RootNamespace and AssemblyName in SharpPhysics.csproj to physics.Engine
so they match current code; additionally pin the SFML.Net package version in the
project file to a specific known-good version to ensure compatibility across
projects.
In `@PhysicsEngine.sln`:
- Around line 10-15: The solution contains external project references
(ProjectorSegmentation.Vision and ProjectorSegmentation.Demo) pointing to
sibling repo paths which break builds for contributors who only clone this repo;
update docs and/or solution so builds are reproducible: add clear setup
instructions in the README/Installation section describing that
ProjectorSegmentation must be cloned as a sibling directory (example structure
and git clone commands), or make the external projects optional by
unloading/guarding them in the solution (remove or conditionally include
ProjectorSegmentation.Vision and ProjectorSegmentation.Demo from
PhysicsEngine.sln and add a note how to re-add them), and include a brief
troubleshooting note about build errors referencing those project GUIDs
({58D568AA-7BA6-90BF-B217-BF1497DB529A}, {C3112F8F-13A2-AC9D-988A-397C4B3B3A44})
so contributors know how to proceed.
In `@SharpPhysics.Demo/DemoGame.cs`:
- Around line 493-499: The right-mouse deletion currently runs in the
OnMouseMoved path (when _isMousePressedRight && !_isCreatingBox) calling
_engine.PhysicsSystem.RemoveActiveObject() repeatedly; change this so deletion
happens only once per press: either remove the deletion logic from the
OnMouseMoved block and keep removal in OnMouseButtonPressed, or add a
press-cycle flag (e.g. _rightDeletedThisPress) that you set when
RemoveActiveObject() is called and check/reset on button release; update the
handlers OnMouseButtonPressed (where initial deletion currently occurs),
OnMouseMoved (remove or gate the call to
_engine.PhysicsSystem.RemoveActiveObject()), and OnMouseButtonReleased to reset
the flag if you choose the press-cycle approach.
In `@SharpPhysics.Demo/RainCatcherGame.cs`:
- Around line 494-504: The Space key handling in HandleDebugInput currently
spawns 5 balls every frame while keyState.Space is true, causing uncontrolled
spawning; change it to only trigger on a key-down transition or implement a
cooldown to respect MAX_BALLS: add state (e.g., a private bool lastSpaceDown or
a float spaceCooldown) to detect when Space goes from false to true (or
decrement cooldown each update and only spawn when <=0), call SpawnBall up to
remaining capacity (respecting MAX_BALLS) and update the state/cooldown after
spawning; reference HandleDebugInput, keyState.Space, SpawnBall and MAX_BALLS to
locate where to apply the change.
In `@SharpPhysics.Demo/SharpPhysics.Demo.csproj`:
- Around line 13-15: SharpPhysics.Demo.csproj currently contains a hard-coded
ProjectReference to
..\..\..\ProjectorSegmentation\src\ProjectorSegmentation.Vision\ProjectorSegmentation.Vision.csproj
which forces a specific sibling-repo layout; change this to either (A) make the
reference conditional by wrapping the ProjectReference with an MSBuild Condition
that only includes it when a property like ProjectorSegmentationPath is set
(e.g., use Condition="'$(ProjectorSegmentationPath)' != ''" and reference via
that property), or (B) replace the ProjectReference with a PackageReference to a
published ProjectorSegmentation.Vision NuGet package, and add clear README setup
instructions for contributors to either set ProjectorSegmentationPath or install
the package; update SharpPhysics.Demo.csproj and project documentation
accordingly.
🟡 Minor comments (16)
physics/Engine/Shaders/SFMLPolyShader.cs-16-17 (1)
16-17: Remove the unusedDrawSleepSupportsproperty.
DrawSleepSupportsis declared in bothSFMLPolyShader.csandSFMLPolyRainbowShader.csbut is never read or referenced anywhere in the codebase. UnlikeDrawNormalswhich is actively used in conditional rendering logic, this property serves no purpose and should be removed to avoid dead code.physics/Engine/Collision.cs-276-309 (1)
276-309: Normal can be wrong when closest point ends up on a different edge.
WhenclosestPointis updated from an edge,closestIndexis not updated. In thedist < 0.0001fbranch (Lines 323-327), the edge normal is derived fromclosestIndex, which may not correspond to the actual closest edge, producing an incorrect normal.🔧 Suggested approach
Track the edge endpoints whenever
closestPointis updated from a segment, and use that edge to compute the normal in the near-zero-distance branch.- Vector2 closestPoint = Vector2.Zero; - int closestIndex = 0; + Vector2 closestPoint = Vector2.Zero; + int closestIndex = 0; + Vector2 closestEdgeA = Vector2.Zero; + Vector2 closestEdgeB = Vector2.Zero; ... - closestPoint = ClosestPointOnSegment(va, vb, circleCenter); + closestPoint = ClosestPointOnSegment(va, vb, circleCenter); + closestEdgeA = va; + closestEdgeB = vb; ... - closestPoint = ClosestPointOnSegment(vb, vc, circleCenter); + closestPoint = ClosestPointOnSegment(vb, vc, circleCenter); + closestEdgeA = vb; + closestEdgeB = vc; ... - closestPoint = ClosestPointOnSegment(poly[i], poly[nextI], circleCenter); + closestPoint = ClosestPointOnSegment(poly[i], poly[nextI], circleCenter); + closestEdgeA = poly[i]; + closestEdgeB = poly[nextI]; ... - Vector2 edge = poly[closestIndex] - poly[prevI]; + Vector2 edge = closestEdgeB - closestEdgeA;Also applies to: 323-327
physics/Engine/Collision.cs-372-375 (1)
372-375: Guard against zero-length segments in ClosestPointOnSegment.
If two consecutive vertices are identical,ab.LengthSquared()becomes zero and causes a divide-by-zero (NaN propagation).✅ Proposed fix
Vector2 ab = b - a; - float t = Vector2.Dot(p - a, ab) / ab.LengthSquared(); + float abLenSq = ab.LengthSquared(); + if (abLenSq < 1e-8f) + return a; + float t = Vector2.Dot(p - a, ab) / abLenSq;SharpPhysics.Tests/SharpPhysics.Tests.csproj-3-3 (1)
3-3: Addglobal.jsonto enforce .NET 9.0 SDK version across development and CI environments.All projects consistently target
net9.0, but without aglobal.jsonfile pinning the SDK version, developers and build systems could inadvertently use different SDK versions (e.g., .NET 8.0 or 10.0), causing environment inconsistencies. Create aglobal.jsonat the repository root with:{ "sdk": { "version": "9.0.0", "rollForward": "latestMinor" } }This enforces the SDK version for all developers. Consider also adding CI/CD configuration (e.g., GitHub Actions) to verify builds succeed automatically.
SharpPhysics.Demo/gamesettings.json-1-25 (1)
1-25: Avoid hard‑coded LAN camera URL in default settings.
Defaulting to a private LAN address with URL capture will fail for most users unless they edit the file. Consider a safer default (e.g., device capture or empty URL) and let users opt into URL mode.💡 Suggested default tweak
- "cameraSourceType": "url", - "cameraUrl": "http://192.168.1.161:8080", + "cameraSourceType": "device", + "cameraUrl": "",physics/Engine/Rendering/UI/UiButton.cs-61-69 (1)
61-69: Pressed color never resets after click.
HandleClicksets_background.FillColorbut nothing restores it, so the button stays in the pressed color indefinitely. If that isn’t intended, consider resetting after the click or tracking a short-lived pressed state.SharpPhysics.Demo/PlatformerGame.cs-617-623 (1)
617-623: Reset debug UI visibility on shutdown.
Initializedisables debug UI; without restoring it, subsequent games may unexpectedly keep it hidden. Consider resetting inShutdown(or ensure every game sets it explicitly).🔧 Suggested fix
public void Shutdown() { + _engine.Renderer.ShowDebugUI = true; _coins.Clear(); _hazards.Clear(); _enemies.Clear(); Console.WriteLine($"🎮 Platformer ended. Final Score: {_score}"); }SharpPhysics.Demo/MenuGame.cs-145-151 (1)
145-151: The exit hint label is created but never added to the UI or rendered.Similar to
SettingsGame, theexitHintlabel is instantiated but not added to_uiManager, so it won't be displayed.🐛 Suggested fix
// Hint at bottom var exitHint = new UiTextLabel("Press ESC to return to menu from any game", font) { Position = new Vector2(centerX - 200, _engine.WindowHeight - 50), CharacterSize = 14 }; + _uiManager.Add(exitHint);SharpPhysics.Demo/SettingsGame.cs-360-380 (1)
360-380: Fix inconsistent indentation in the Render method.Lines 360-380 have broken indentation - the
switchblock and closing braces are misaligned. This hinders readability and may cause issues with future edits.🔧 Suggested fix for indentation
- // Category-specific hints - float hintY = startY + _currentSettings.Count * rowHeight + 35f; - switch (_currentCategory) - { - case "Camera": - renderer.DrawText("Common URL formats:", labelX, hintY, 14, new Color(180, 180, 200)); - renderer.DrawText("- MJPEG Stream: http://<ip>:8080", labelX + 20f, hintY + 22f, 12, new Color(140, 140, 160)); - renderer.DrawText("- ESP32-CAM: http://<ip>:81/stream", labelX + 20f, hintY + 40f, 12, new Color(140, 140, 160)); - break; - case "Detection": - renderer.DrawText("Larger ball radius = easier tracking, smaller = more precise", labelX, hintY, 14, new Color(180, 180, 200)); - break; - case "Display": - renderer.DrawText("WARNING: Display changes require application restart", labelX, hintY, 14, new Color(255, 200, 100)); - break; - } - - // Draw UI elements (category buttons) - renderer.Window.SetView(renderer.UiView); - _uiManager.Draw(renderer.Window); - } + // Category-specific hints + float hintY = startY + _currentSettings.Count * rowHeight + 35f; + switch (_currentCategory) + { + case "Camera": + renderer.DrawText("Common URL formats:", labelX, hintY, 14, new Color(180, 180, 200)); + renderer.DrawText("- MJPEG Stream: http://<ip>:8080", labelX + 20f, hintY + 22f, 12, new Color(140, 140, 160)); + renderer.DrawText("- ESP32-CAM: http://<ip>:81/stream", labelX + 20f, hintY + 40f, 12, new Color(140, 140, 160)); + break; + case "Detection": + renderer.DrawText("Larger ball radius = easier tracking, smaller = more precise", labelX, hintY, 14, new Color(180, 180, 200)); + break; + case "Display": + renderer.DrawText("WARNING: Display changes require application restart", labelX, hintY, 14, new Color(255, 200, 100)); + break; + } + + // Draw UI elements (category buttons) + renderer.Window.SetView(renderer.UiView); + _uiManager.Draw(renderer.Window); + }SharpPhysics.Demo/MenuGame.cs-233-243 (1)
233-243: Fix inconsistent indentation in the Render method.Lines 236-243 have misaligned indentation compared to the rest of the method.
🔧 Suggested fix
// Draw animated background particles DrawParticles(renderer); - // Draw version/info at bottom - renderer.DrawText("Use your body to interact! • Body tracking powered by YOLO Pose", - _engine.WindowWidth / 2f - 250, _engine.WindowHeight - 30, 14, new Color(120, 120, 140)); + // Draw version/info at bottom + renderer.DrawText("Use your body to interact! • Body tracking powered by YOLO Pose", + _engine.WindowWidth / 2f - 250, _engine.WindowHeight - 30, 14, new Color(120, 120, 140)); - // Draw UI elements (menu buttons) - renderer.Window.SetView(renderer.UiView); - _uiManager.Draw(renderer.Window); - } + // Draw UI elements (menu buttons) + renderer.Window.SetView(renderer.UiView); + _uiManager.Draw(renderer.Window); + }SharpPhysics.Demo/Helpers/SkeletonRenderer.cs-79-82 (1)
79-82: Missing bounds check on confidences in keypoint loop.Similarly, accessing
confidences[i]should verifyi < confidences.Length.🐛 Proposed fix
for (int i = 0; i < keypoints.Length; i++) { + if (i >= confidences.Length) + continue; + if (confidences[i] < confidenceThreshold) continue;SharpPhysics.Demo/Helpers/SkeletonRenderer.cs-61-67 (1)
61-67: Missing bounds check on confidences array.The code checks if indices are within
keypoints.Lengthbut accessesconfidences[idx1]andconfidences[idx2]without verifying they're within bounds of the confidences array.🐛 Proposed fix
foreach (var (idx1, idx2) in connections) { - if (idx1 >= keypoints.Length || idx2 >= keypoints.Length) + if (idx1 >= keypoints.Length || idx2 >= keypoints.Length || + idx1 >= confidences.Length || idx2 >= confidences.Length) continue;SharpPhysics.Demo/RainCatcherGame.cs-455-458 (1)
455-458: GravityFlip toggles on activation but always resets to false on deactivation.When GravityFlip is activated, it toggles
_isGravityReversed. ButDeactivatePowerUp(line 482-484) always sets it tofalse. If gravity was already reversed before the power-up, this creates inconsistent behavior.🔧 Proposed fix - store pre-powerup state
+ private bool _preGravityFlipState = false; + case "GravityFlip": + _preGravityFlipState = _isGravityReversed; _isGravityReversed = !_isGravityReversed; _physics.Gravity = new Vector2(0, _isGravityReversed ? -6f : 6f); break;And in deactivation:
case "GravityFlip": - _isGravityReversed = false; - _physics.Gravity = new Vector2(0, 6f); + _isGravityReversed = _preGravityFlipState; + _physics.Gravity = new Vector2(0, _isGravityReversed ? -6f : 6f); break;SharpPhysics.Demo/DemoGame.cs-345-366 (1)
345-366: Inconsistent indentation in Render and subsequent methods.Lines 345-524 use 8-space indentation while the rest of the file uses 4-space. This appears to be a formatting inconsistency that should be corrected.
🔧 Fix indentation
- public void Render(Renderer renderer) - { + public void Render(Renderer renderer) + {Apply similar 4-space indentation to all methods from line 345 onwards.
SharpPhysics.Demo/RainCatcherGame.cs-135-146 (1)
135-146: Unusedangleparameter inCreateAngledRamp.The
angleparameter is accepted but never applied to the ramp. The comment mentions "rotation would need polygon" - either remove the parameter or implement rotation with a polygon shape.🔧 Remove unused parameter or add TODO
- private void CreateAngledRamp(float x, float y, int width, int height, float angle) + private void CreateAngledRamp(float x, float y, int width, int height) { - // Create a simple angled platform using a box (rotation would need polygon) + // TODO: Implement rotation using polygon shapes for actual angled platforms var ramp = _physics.CreateStaticBox(Also update the call sites on lines 119-120.
SharpPhysics.Demo/Integration/PersonColliderBridge.cs-216-217 (1)
216-217: Thewidth,height, andfpsparameters are not used for MJPEG streams.The
MjpegCameraFrameSourceconstructor at line 216 doesn't use thewidth,height, orfpsparameters passed toStart(). This is inconsistent with the OpenCV camera version (line 257) where these parameters are used. Either remove the unused parameters from the MJPEG overload signature, or pass them to the frame source if supported.
🧹 Nitpick comments (40)
physics/Engine/Player/PlayerController.cs (3)
5-5: Unused import.
using System;does not appear to be used in this file. Consider removing it.
37-37: Remove commented-out code.If this velocity reset is no longer needed, delete the line rather than leaving it commented. Version control preserves history if you need to restore it later.
Suggested diff
if (normal.Y > 0.5f) { _groundObjects.Add(obj); - //_player.Velocity = new Vector2(_player.Velocity.X, 0); return; }
77-81: Extract magic number to a constant.The value
2000should be a named constant (e.g.,_slamForce) for consistency with_speedand_jumpForce, and to make tuning easier.Suggested refactor
Add a field near the other movement constants:
private float _speed = 150.0f; private float _jumpForce = 180.0f; + private float _slamForce = 2000.0f;Then update
Slam():private void Slam() { - _player.Velocity = new Vector2(_player.Velocity.X, 2000); + _player.Velocity = new Vector2(_player.Velocity.X, _slamForce); }physics/Engine/Shaders/SFMLPolyShader.cs (1)
57-65: Consider preallocatingCircleShapeto reduce GC pressure.Given the PR's GC optimization goals, the
CircleShapecreated on everyPostDrawcall could be preallocated as a class field (likevertexesandcontactLines), then updated in place with the current position and color.♻️ Suggested refactor
public class SFMLPolyShader : SFMLShader { // Stores the polygon's outline points. private VertexArray vertexes = new VertexArray(PrimitiveType.LineStrip); // Preallocated VertexArray for drawing contact normals. private VertexArray contactLines = new VertexArray(PrimitiveType.Lines); + // Preallocated circle for drawing the object's center. + private CircleShape centerCircle = new CircleShape(5f) + { + Origin = new Vector2f(5f, 5f), + OutlineThickness = 1f, + FillColor = Color.Transparent + };Then in
PostDraw:- float radius = 5f; - CircleShape circle = new CircleShape(radius) - { - Origin = new Vector2f(radius, radius), - Position = obj.Center.ToSfml(), - OutlineThickness = 1f, - OutlineColor = obj.Sleeping ? Color.Blue : Color.Red, - FillColor = Color.Transparent - }; - target.Draw(circle); + centerCircle.Position = obj.Center.ToSfml(); + centerCircle.OutlineColor = obj.Sleeping ? Color.Blue : Color.Red; + target.Draw(centerCircle);physics/Engine/Shaders/SFMLPolyRainbowShader.cs (5)
13-14: Misleading comment: TriangleFan renders a filled polygon, not an outline.The comment states "polygon's outline points" but
PrimitiveType.TriangleFanrenders a filled polygon using the first vertex as the fan origin. If an outline is intended, useLineStriplikeSFMLPolyShader. If filled rendering is correct, update the comment.Suggested comment fix
- // Stores the polygon's outline points. + // Stores the polygon's vertices for filled rendering. private VertexArray vertexes = new VertexArray(PrimitiveType.TriangleFan);
54-58: Unnecessary vertex append for TriangleFan.With
PrimitiveType.TriangleFan, the fan automatically connects back to the first vertex. Appendingpoints[0]again creates a degenerate triangle. This pattern is needed forLineStrip(as inSFMLPolyShader) but not forTriangleFan.Suggested fix
- // Close the loop. - if (points.Length > 0) - { - vertexes.Append(new Vertex(points[0].ToSfml(), _color)); - }
69-69: Stale comment: "3)" has no corresponding "1)" or "2)".This appears to be a copy-paste artifact from
SFMLPolyShader.PostDrawwhere steps 1, 2, and 3 exist. Here, only the normal-drawing step is present.Suggested fix
- // 3) Draw a line from each contact point along its normal. + // Draw a line from each contact point along its normal.
60-61: Misleading comment: TriangleFan draws a filled polygon, not an outline.Update the comment to reflect the actual rendering behavior.
Suggested fix
- // 1) Draw the polygon outline. + // Draw the filled polygon. target.Draw(vertexes);
18-18: Remove unused propertyDrawSleepSupports.This static property is declared but never used anywhere in the codebase. An identical declaration also exists in
SFMLPolyShader.cs. Remove this property unless there is a planned feature that requires it..gitignore (1)
241-243: Consider moving the*.onnxrule to a more appropriate section.The
*.onnxignore rule is placed under the "FAKE - F# Make" section, which is misleading. Consider adding a dedicated section for ML/ONNX assets or placing it under "Others" (around line 179).Also, since the demo project references ONNX models (e.g.,
models/yolo26s_pose.onnxpergamesettings.json), ensure the README or setup instructions document how users should obtain these model files.Suggested reorganization
# Others ClientBin/ [Ss]tyle[Cc]op.* ~$* *~ *.dbmdl *.dbproj.schemaview *.pfx *.publishsettings node_modules/ orleans.codegen.cs + +# ML/ONNX Models (large binaries, download separately) +*.onnxAnd remove line 243 from under the FAKE section.
README.md (1)
12-12: Track or address the TODO to update the readme.The TODO comment indicates pending documentation work. Consider creating an issue to track this or completing the documentation update as part of this PR.
Would you like me to help draft updated documentation sections based on the new features introduced in this PR?
PhysicsEngine.sln (1)
3-4: Visual Studio Version 18 header may cause compatibility warnings.The solution format references Visual Studio Version 18, which corresponds to a preview/future version. Developers using VS 2022 (Version 17) may see compatibility warnings when opening the solution, though it should still load correctly.
physics/Engine/Rendering/UI/IUiClickable.cs (1)
8-9: Consider documenting theOnClickevent parameter semantics.The
boolparameter inAction<bool> OnClickhas different meanings across implementers:
UiCheckbox: passes the newIsCheckedstate after toggleUiSlider: always passestruewhen clickedThis semantic inconsistency could confuse consumers. Consider either:
- Adding XML documentation to clarify the expected usage
- Using a more descriptive event signature (e.g., a custom
ClickEventArgs)Suggested documentation
public interface IUiClickable { - // Move the event to the interface + /// <summary> + /// Raised when the element is clicked. + /// The bool parameter meaning is implementation-specific: + /// for toggleable controls it indicates the new state, + /// for simple buttons it indicates click occurred (always true). + /// </summary> event Action<bool> OnClick; - // Keep the click handling method + /// <summary> + /// Handles a click at the specified position. + /// </summary> + /// <param name="clickPos">The position of the click in UI coordinates.</param> + /// <returns>True if the click was handled by this element; otherwise, false.</returns> bool HandleClick(Vector2 clickPos); }physics/Engine/Classes/Objects/PhysicsObject.cs (1)
47-47: Consider resetting sleep state whenCanSleepis false.
IfCanSleepis turned off at runtime, a sleeping object stays asleep andsleepTimerkeeps accumulating, so re‑enabling can immediately put it back to sleep. Consider short‑circuitingUpdateSleepStateto wake/reset whenCanSleepis false.💡 Optional guard in UpdateSleepState
public void UpdateSleepState(float dt) { + if (!CanSleep) + { + if (Sleeping) Wake(); + sleepTimer = 0f; + return; + } if (Locked) { sleepTimer = 0f; return; }Also applies to: 203-204
physics/Engine/Classes/CollisionPairPool.cs (1)
21-25: Consider adding null guard inReturn().If a null
CollisionPairis accidentally passed,pair.Reset()will throw. A simple guard prevents this edge case.Suggested fix
public void Return(CollisionPair pair) { + if (pair == null) return; pair.Reset(); _pool.Push(pair); }physics/Engine/Input/InputManager.cs (2)
25-26:prevKeyStateappears unused.The
prevKeyStatefield is updated at lines 272-280 but never read. The "Pressed" states are derived from_pressTimers, not from comparing previous vs current state. This is dead code.Suggested fix
Remove the unused field and the update block:
- // Previous frame's key state for edge detection - private KeyState prevKeyState = new KeyState();And remove lines 271-280 in
GetKeyState():- // Store current key state as previous for next frame (struct copy, no allocation) - prevKeyState.Left = keyState.Left; - prevKeyState.Right = keyState.Right; - prevKeyState.Up = keyState.Up; - prevKeyState.Down = keyState.Down; - prevKeyState.Space = keyState.Space; - prevKeyState.Escape = keyState.Escape; - prevKeyState.Enter = keyState.Enter; - prevKeyState.Tab = keyState.Tab; - prevKeyState.Backspace = keyState.Backspace;Also applies to: 272-280
222-283: Inconsistent indentation inGetKeyState()method.The method body has excessive indentation (appears to be indented 16 spaces instead of 8). This harms readability.
physics/Engine/Helpers/ConstraintHelper.cs (1)
1-9: Unused imports.
System,System.Collections.Generic, andSystem.Linqappear unused in this file.Suggested fix
-using System; -using System.Collections.Generic; using physics.Engine.Classes; using physics.Engine.Objects; using physics.Engine.Shapes; using physics.Engine.Constraints; -using System.Linq; using System.Numerics; using physics.Engine.Core;physics/Engine/Rendering/UI/UiSlider.cs (1)
87-97:newmodifier hides interface methods unexpectedly.Using
newonHandleDragandStopDraghides any base class definitions. Since these are interface implementations, use explicit interface implementation instead for clarity.Suggested change
- public new bool HandleDrag(Vector2 dragPos) + bool IUiDraggable.HandleDrag(Vector2 dragPos) { ... } - public new void StopDrag() + void IUiDraggable.StopDrag() { ... }Also applies to: 99-107
physics/Engine/Constraints/Constraint.cs (1)
45-123: Clamp Baumgarte bias inWeldConstraintfor stability.AxisConstraint clamps bias; WeldConstraint doesn’t, so large errors or small
dtcan produce oversized impulses.♻️ Proposed adjustment
private const float BaumgarteBias = 0.1f; private const float AngularBias = 0.05f; +private const float MaxBiasVelocity = 300f; @@ - float bias = BaumgarteBias * posError / dt; + float bias = BaumgarteBias * posError / dt; + bias = Math.Clamp(bias, -MaxBiasVelocity, MaxBiasVelocity);physics/Engine/Rendering/Renderer.cs (1)
46-54: DrawText leaves the window in UI view.Because DrawText switches to
UiViewwithout restoring the previous view, callers that render viaRenderer.Windowafterward may draw in the wrong coordinate space. Consider saving/restoring the view.♻️ Suggested change
public void DrawText(string text, float x, float y, uint size = 24, Color? color = null) { - Window.SetView(UiView); + var previousView = Window.GetView(); + Window.SetView(UiView); _reusableText.DisplayedString = text; _reusableText.CharacterSize = size; _reusableText.Position = new Vector2f(x, y); _reusableText.FillColor = color ?? Color.White; Window.Draw(_reusableText); + Window.SetView(previousView); }SharpPhysics.Demo/SettingsGame.cs (2)
112-146: Inconsistent boolean parsing logic.Lines 113-114 use
bool.TryParse()for parsing booleans, but lines 133 and 135 use string comparison (v.ToLower() == "true"). This inconsistency means "True", "TRUE", and "false" are handled differently across settings.♻️ Suggested fix for consistent boolean parsing
case "Detection": _currentSettings.Add(new SettingItem("Model Path", _settings.ModelPath, v => _settings.ModelPath = v, "path to ONNX model")); _currentSettings.Add(new SettingItem("Use GPU", _settings.UseGpu.ToString(), - v => _settings.UseGpu = v.ToLower() == "true", "true/false")); + v => { if (bool.TryParse(v, out var b)) _settings.UseGpu = b; }, "true/false")); _currentSettings.Add(new SettingItem("Mirror Mode (FlipX)", _settings.FlipX.ToString(), - v => _settings.FlipX = v.ToLower() == "true", "true/false")); + v => { if (bool.TryParse(v, out var b)) _settings.FlipX = b; }, "true/false"));
386-393: Avoid empty catch blocks that silently swallow exceptions.The empty catch block hides any errors during event unsubscription. At minimum, log exceptions for debugging purposes, or handle only the specific expected exception type.
♻️ Suggested fix
try { _engine.Renderer.Window.TextEntered -= OnTextEntered; } - catch { } + catch (Exception ex) + { + Console.WriteLine($"Warning: Failed to unsubscribe TextEntered: {ex.Message}"); + }physics/Engine/PhysicsSystem.cs (1)
513-521: Consider pooling cell lists to reduce GC pressure.A new
List<PhysicsObject>is allocated for each spatial hash cell every frame. For scenes with many cells, this could generate allocation pressure.Consider maintaining a pool of lists or reusing a single list per cell across frames if profiling shows this as a hotspot.
SharpPhysics.Demo/Program.cs (1)
19-23: Consider logging the full exception for debugging.Only
ex.Messageis logged, which omits the stack trace. During development, the full exception details are valuable for diagnosing issues.♻️ Suggested improvement
catch (Exception ex) { Console.WriteLine("Fatal error starting the game engine:"); - Console.WriteLine(ex.Message); + Console.WriteLine(ex); }SharpPhysics.Demo/Integration/TrackedPerson.cs (1)
36-43: Consider caching or avoiding allocation inGetTrackingBalls().This method allocates a new list on every call. If called frequently (e.g., per frame), consider caching the result or returning an array/span.
♻️ Alternative implementation to avoid per-call allocation
+ private PhysicsObject[]? _trackingBallsCache; + private int _trackingBallsCacheVersion = -1; + private int _ballsVersion = 0; + public IReadOnlyList<PhysicsObject> GetTrackingBalls() { - var balls = new List<PhysicsObject>(); - if (HeadBall != null) balls.Add(HeadBall); - if (LeftHandBall != null) balls.Add(LeftHandBall); - if (RightHandBall != null) balls.Add(RightHandBall); - return balls; + // Simple approach: return array with nulls filtered + var count = (HeadBall != null ? 1 : 0) + + (LeftHandBall != null ? 1 : 0) + + (RightHandBall != null ? 1 : 0); + var result = new PhysicsObject[count]; + int idx = 0; + if (HeadBall != null) result[idx++] = HeadBall; + if (LeftHandBall != null) result[idx++] = LeftHandBall; + if (RightHandBall != null) result[idx++] = RightHandBall; + return result; }Or if the balls rarely change, cache the result and invalidate when setters are called.
SharpPhysics.Demo/BubblePopGame.cs (2)
44-51: Verify spawn weight totals exactly 1.0.The
SpawnWeightvalues (0.4 + 0.3 + 0.2 + 0.08 + 0.02) sum to 1.0, which is correct. However, floating-point arithmetic could introduce precision issues. Consider adding a comment or normalizing at runtime for robustness.
180-180: MoveRandomExtensionsto a shared helpers file.
_random.NextFloat()relies on the extension methodRandomExtensionsdefined inRainCatcherGame.cs(line 575-577), but it's also used inMenuGame.csandBubblePopGame.cs. This creates an implicit cross-file dependency where multiple game files depend on an extension defined in an unrelated game class. The project already has aHelpersdirectory; movingRandomExtensionsthere would make this utility more discoverable and logically organized.physics/Engine/Rendering/UI/UiMenuButton.cs (1)
111-131: Duplicate bounds-checking logic between HandleClick and ContainsPoint.Both
HandleClickandContainsPointperform identical bounds checking. Consider havingHandleClickdelegate toContainsPointfor DRY compliance.♻️ Proposed refactor
bool IUiClickable.HandleClick(Vector2 clickPos) { - var bounds = _background.GetGlobalBounds(); - if (clickPos.X >= bounds.Left && clickPos.X <= bounds.Left + bounds.Width && - clickPos.Y >= bounds.Top && clickPos.Y <= bounds.Top + bounds.Height) + if (ContainsPoint(clickPos)) { OnClick?.Invoke(true); return true; } return false; }SharpPhysics.Demo/DemoGame.cs (2)
76-86: Remove or document commented-out code.The commented ball grid creation code should either be removed if no longer needed or documented with a reason for keeping it (e.g., for future testing). Commented-out code adds noise and maintenance burden.
96-251: Consider breaking downCreateCarDemointo smaller focused methods.At ~155 lines, this method handles car body, wheels, axis constraints, weld constraints, rope chains, and circle formations. Consider extracting logical groups into separate methods for improved readability and testability.
♻️ Suggested structure
private void CreateCarDemo(uint worldWidth, uint worldHeight) { var carBody = CreateCarBody(worldWidth, worldHeight); AttachWheels(carBody, worldWidth, worldHeight); AttachBodyParts(carBody, worldWidth, worldHeight); CreateRopeChains(); CreateCircleOfBalls(); }SharpPhysics.Demo/RainCatcherGame.cs (2)
574-578: MoveRandomExtensionsto a shared helper file.This extension method is used by both
RainCatcherGameandBubblePopGame. Placing it in a shared location (e.g.,SharpPhysics.Demo/Helpers/RandomExtensions.cs) would improve discoverability and prevent duplication.
231-267: UpdateTimers has side effects on physics system.This method modifies
_physics.Gravityduring the gravity pulse effect. Consider renaming toUpdateTimersAndEffectsor extracting the gravity modification to make the side effect explicit.physics/Engine/Classes/Templates/ActionTemplates.cs (1)
21-24: Simplify Vector2 delta calculation.The verbose Vector2 initialization can be simplified.
♻️ Simplified code
- Vector2 delta = (new Vector2 { X = EndPointF.X, Y = EndPointF.Y } - - new Vector2 { X = StartPointF.X, Y = StartPointF.Y }); + Vector2 delta = EndPointF - StartPointF; _physicsSystem.AddVelocityToActive(delta * 2);SharpPhysics.Demo/Settings/GameSettings.cs (2)
14-34: Consider marking_instanceasvolatilefor thread-safe singleton.The double-checked locking pattern requires the instance field to be
volatileto ensure that when a thread sees a non-null_instance, it also sees a fully constructed object. Withoutvolatile, there's a theoretical risk of another thread seeing a non-null but partially initialized object.Suggested fix
- private static GameSettings? _instance; + private static volatile GameSettings? _instance;
2-2: Unused import.
System.Numericsis imported butVector2doesn't appear to be used in this file.`#nullable` enable -using System.Numerics; using System.Text.Json;physics/Engine/Classes/Templates/ObjectTemplates.cs (2)
20-23: Consider adding null check for constructor parameter.If
physicsSystemis null, the error will manifest later when calling factory methods, making debugging harder. Adding an early guard clause improves error reporting.Suggested fix
public ObjectTemplates(PhysicsSystem physicsSystem) { + ArgumentNullException.ThrowIfNull(physicsSystem); _physicsSystem = physicsSystem; }
26-49: Minor: Consider usingTryGetValueinstead ofContainsKey+ indexer.The current pattern performs two dictionary lookups. Using
TryGetValuereduces this to one lookup per access.Suggested refactor
private T GetShader<T>(int diameter) where T : SFMLShader { Type shaderType = typeof(T); - if (!shaderPool.ContainsKey(shaderType)) + if (!shaderPool.TryGetValue(shaderType, out var innerDict)) { - shaderPool[shaderType] = new Dictionary<int, SFMLShader>(); + innerDict = new Dictionary<int, SFMLShader>(); + shaderPool[shaderType] = innerDict; } - var innerDict = shaderPool[shaderType]; - if (!innerDict.ContainsKey(diameter)) + if (!innerDict.TryGetValue(diameter, out var shader)) { // Try to get a constructor that takes an int parameter. var constructor = shaderType.GetConstructor(new Type[] { typeof(int) }); if (constructor != null) { - innerDict[diameter] = (SFMLShader)Activator.CreateInstance(shaderType, diameter); + shader = (SFMLShader)Activator.CreateInstance(shaderType, diameter)!; } else { // If not available, use the default constructor. - innerDict[diameter] = (SFMLShader)Activator.CreateInstance(shaderType); + shader = (SFMLShader)Activator.CreateInstance(shaderType)!; } + innerDict[diameter] = shader; } - return (T)innerDict[diameter]; + return (T)shader; }SharpPhysics.Demo/Integration/PersonColliderBridge.cs (2)
202-279: Significant code duplication between Start overloads.Both
Start(string url, ...)andStart(int cameraIndex, ...)share nearly identical code for model loading, detector initialization, thread creation, and error handling. Only the frame source instantiation differs. Consider extracting the common logic.Suggested refactor
+private void StartInternal(IFrameSource frameSource, string sourceDescription) +{ + try + { + Console.WriteLine($"[PersonBridge] Loading model from: {System.IO.Path.GetFullPath(_modelPath)}"); + + if (!System.IO.File.Exists(_modelPath)) + { + throw new System.IO.FileNotFoundException($"Model file not found: {_modelPath}"); + } + + _poseDetector = new YoloV26PoseDetector(_modelPath, useGpu: true, maxPeople: _maxPeople); + Console.WriteLine("[PersonBridge] Yolo model loaded successfully"); + + _camera = frameSource; + Console.WriteLine($"[PersonBridge] {sourceDescription}"); + Console.WriteLine($"[PersonBridge] Multi-person tracking enabled (max {_maxPeople} people)"); + + _cts = new CancellationTokenSource(); + _isRunning = true; + + _detectionThread = new Thread(() => DetectionLoop(_cts.Token)) + { + Name = "PersonColliderBridge-Detection", + IsBackground = true + }; + _detectionThread.Start(); + + Console.WriteLine($"[PersonBridge] Detection thread started"); + } + catch (Exception ex) + { + Console.WriteLine($"[PersonBridge] Failed to start: {ex.Message}"); + Console.WriteLine($"[PersonBridge] Stack trace: {ex.StackTrace}"); + OnError?.Invoke(this, ex); + } +} + public void Start(string url, int width = 640, int height = 480, int fps = 30) { - // ... current implementation + var source = new MjpegCameraFrameSource(url, 5, true); + StartInternal(source, $"Stream {url} opened at {width}x{height} @ {fps}fps"); } public void Start(int cameraIndex = 0, int width = 640, int height = 480, int fps = 30) { - // ... current implementation + var source = new OpenCvCameraFrameSource(cameraIndex, width, height, fps); + StartInternal(source, $"Camera {cameraIndex} opened at {width}x{height} @ {fps}fps"); }
686-695: Consider whether setting velocity is necessary for locked objects.
MoveBallToPositionunlocks the ball, sets velocity and moves it, then re-locks. Since the ball is immediately locked again, the velocity assignment (line 692) may have no effect—the physics system likely ignores velocity for locked objects. If the velocity serves no purpose here, consider removing it for clarity.
| public void Reset() | ||
| { | ||
| A = null; | ||
| B = null; | ||
| } |
There was a problem hiding this comment.
Potential NullReferenceException after Reset() is called.
After Reset() sets A and B to null, both Equals() and GetHashCode() will throw NullReferenceException since they access this.A.Equals() and A.GetHashCode() without null checks.
While pooled pairs shouldn't typically be used in equality comparisons, defensive null-checks would prevent subtle runtime crashes.
Suggested fix
public bool Equals(CollisionPair other)
{
- if (other == null)
+ if (other == null || A == null || B == null || other.A == null || other.B == null)
return false;
return (this.A.Equals(other.A) && this.B.Equals(other.B)) ||
(this.A.Equals(other.B) && this.B.Equals(other.A));
}
public override int GetHashCode()
{
+ if (A == null || B == null)
+ return 0;
int hashA = A.GetHashCode();
int hashB = B.GetHashCode();
return hashA ^ hashB;
}Also applies to: 44-58
🤖 Prompt for AI Agents
In `@physics/Engine/Classes/CollisionPair.cs` around lines 33 - 37,
CollisionPair.Reset() sets A and B to null but Equals() and GetHashCode() call
A.Equals(...) and A.GetHashCode() directly, which can throw
NullReferenceException; update CollisionPair.Equals(...) and
CollisionPair.GetHashCode() to defensively handle nulls by checking A and B for
null (use ReferenceEquals or explicit null checks) before calling
Equals/GetHashCode, return appropriate fallback values (e.g., false for
comparisons when the other operand isn't matching or 0/combined hash when fields
are null) and keep equality symmetrical and consistent with GetHashCode.
|
|
||
| // Friction, in newtons | ||
| public float Friction = 0.5f; | ||
| public static float Friction { get; set; }= 0.5f; |
There was a problem hiding this comment.
Static Friction makes per‑object tuning impossible.
If any code expects per‑object friction, this change silently globalizes it. Consider keeping an instance property with a static default to preserve configurability.
🛠️ Suggested adjustment
- public static float Friction { get; set; }= 0.5f;
+ public static float DefaultFriction { get; set; } = 0.5f;
+ public float Friction { get; set; } = DefaultFriction;🤖 Prompt for AI Agents
In `@physics/Engine/Classes/Objects/PhysicsObject.cs` at line 29, The Friction
property was made static which forces a global value and prevents per-object
tuning; revert to an instance property on PhysicsObject and keep a static
default constant for new instances. Replace the static Friction with an instance
property named Friction (get/set) initialized to a static DefaultFriction (or
DefaultFrictionValue) set to 0.5f, and update constructors/usages that
referenced PhysicsObject.Friction to use the instance property (obj.Friction) or
the static DefaultFriction when creating new objects.
| public void Launch(PhysicsObject physObj, Vector2 StartPointF, Vector2 EndPointF) | ||
| { | ||
| physSystem.ActivateAtPoint(StartPointF); | ||
| _physicsSystem.ActivateAtPoint(StartPointF); | ||
| Vector2 delta = (new Vector2 { X = EndPointF.X, Y = EndPointF.Y } - | ||
| new Vector2 { X = StartPointF.X, Y = StartPointF.Y }); | ||
| physSystem.AddVelocityToActive(delta * 2); | ||
| _physicsSystem.AddVelocityToActive(delta * 2); | ||
| } |
There was a problem hiding this comment.
Unused physObj parameter in Launch method.
The physObj parameter is passed but never used. The method activates an object at StartPointF instead of using the provided physObj. This appears to be a bug or incomplete refactoring.
🐛 Proposed fix - use the provided object
Option 1: Remove the unused parameter if activation by point is intended:
-public void Launch(PhysicsObject physObj, Vector2 StartPointF, Vector2 EndPointF)
+public void Launch(Vector2 StartPointF, Vector2 EndPointF)
{
_physicsSystem.ActivateAtPoint(StartPointF);Option 2: Use the provided object directly:
public void Launch(PhysicsObject physObj, Vector2 StartPointF, Vector2 EndPointF)
{
- _physicsSystem.ActivateAtPoint(StartPointF);
+ _physicsSystem.SetActiveObject(physObj);
Vector2 delta = (new Vector2 { X = EndPointF.X, Y = EndPointF.Y } -
new Vector2 { X = StartPointF.X, Y = StartPointF.Y });
_physicsSystem.AddVelocityToActive(delta * 2);
}🤖 Prompt for AI Agents
In `@physics/Engine/Classes/Templates/ActionTemplates.cs` around lines 19 - 25,
The Launch method's physObj parameter is unused; either remove the parameter
from Launch(PhysicsObject physObj, Vector2 StartPointF, Vector2 EndPointF) if
activation is meant to be location-only, or update the method to activate and
apply velocity to the provided PhysicsObject instead of activating by
StartPointF: replace the call to _physicsSystem.ActivateAtPoint(StartPointF)
with the appropriate activation call for the object (e.g., activate/mark physObj
in the physics system) and ensure _physicsSystem.AddVelocityToActive applies the
computed delta*2 to that activated physObj; keep the existing delta calculation
(from StartPointF to EndPointF) and pass or associate the velocity with physObj
so physObj is actually used.
| public void PopAndMultiply() | ||
| { | ||
| foreach(PhysicsObject obj in PhysicsSystem.ListStaticObjects) | ||
| foreach(PhysicsObject obj in _physicsSystem.ListStaticObjects) | ||
| { | ||
| physSystem.ActivateAtPoint(new Vector2(obj.Center.X, obj.Center.Y)); | ||
| _physicsSystem.ActivateAtPoint(new Vector2(obj.Center.X, obj.Center.Y)); | ||
| var velocity = obj.Velocity; | ||
| var origin = obj.Center; | ||
| physSystem.RemoveActiveObject(); | ||
| physSystem.SetVelocity(ObjectTemplates.CreateSmallBall(origin.X, origin.Y), velocity); | ||
| physSystem.SetVelocity(ObjectTemplates.CreateSmallBall(origin.X, origin.Y), velocity); | ||
| _physicsSystem.RemoveActiveObject(); | ||
| _physicsSystem.SetVelocity(_objectTemplates.CreateSmallBall(origin.X, origin.Y), velocity); | ||
| _physicsSystem.SetVelocity(_objectTemplates.CreateSmallBall(origin.X, origin.Y), velocity); | ||
| } | ||
| } |
There was a problem hiding this comment.
Iterating over collection while potentially modifying it.
PopAndMultiply iterates over _physicsSystem.ListStaticObjects while calling RemoveActiveObject(), which may modify the underlying collection. This could cause a InvalidOperationException or skip elements.
🐛 Proposed fix - snapshot the collection first
public void PopAndMultiply()
{
- foreach(PhysicsObject obj in _physicsSystem.ListStaticObjects)
+ var objectsToProcess = _physicsSystem.ListStaticObjects.ToList();
+ foreach(PhysicsObject obj in objectsToProcess)
{
_physicsSystem.ActivateAtPoint(new Vector2(obj.Center.X, obj.Center.Y));
var velocity = obj.Velocity;
var origin = obj.Center;
_physicsSystem.RemoveActiveObject();
_physicsSystem.SetVelocity(_objectTemplates.CreateSmallBall(origin.X, origin.Y), velocity);
_physicsSystem.SetVelocity(_objectTemplates.CreateSmallBall(origin.X, origin.Y), velocity);
}
}📝 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.
| public void PopAndMultiply() | |
| { | |
| foreach(PhysicsObject obj in PhysicsSystem.ListStaticObjects) | |
| foreach(PhysicsObject obj in _physicsSystem.ListStaticObjects) | |
| { | |
| physSystem.ActivateAtPoint(new Vector2(obj.Center.X, obj.Center.Y)); | |
| _physicsSystem.ActivateAtPoint(new Vector2(obj.Center.X, obj.Center.Y)); | |
| var velocity = obj.Velocity; | |
| var origin = obj.Center; | |
| physSystem.RemoveActiveObject(); | |
| physSystem.SetVelocity(ObjectTemplates.CreateSmallBall(origin.X, origin.Y), velocity); | |
| physSystem.SetVelocity(ObjectTemplates.CreateSmallBall(origin.X, origin.Y), velocity); | |
| _physicsSystem.RemoveActiveObject(); | |
| _physicsSystem.SetVelocity(_objectTemplates.CreateSmallBall(origin.X, origin.Y), velocity); | |
| _physicsSystem.SetVelocity(_objectTemplates.CreateSmallBall(origin.X, origin.Y), velocity); | |
| } | |
| } | |
| public void PopAndMultiply() | |
| { | |
| var objectsToProcess = _physicsSystem.ListStaticObjects.ToList(); | |
| foreach(PhysicsObject obj in objectsToProcess) | |
| { | |
| _physicsSystem.ActivateAtPoint(new Vector2(obj.Center.X, obj.Center.Y)); | |
| var velocity = obj.Velocity; | |
| var origin = obj.Center; | |
| _physicsSystem.RemoveActiveObject(); | |
| _physicsSystem.SetVelocity(_objectTemplates.CreateSmallBall(origin.X, origin.Y), velocity); | |
| _physicsSystem.SetVelocity(_objectTemplates.CreateSmallBall(origin.X, origin.Y), velocity); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@physics/Engine/Classes/Templates/ActionTemplates.cs` around lines 35 - 46,
PopAndMultiply iterates _physicsSystem.ListStaticObjects while calling
_physicsSystem.RemoveActiveObject(), which can mutate the collection; fix by
iterating a snapshot copy instead of the live collection: capture the objects
into a separate list/array (e.g., call ToList() or copy into a new
List<PhysicsObject>) and loop over that snapshot in PopAndMultiply, keeping the
existing ActivateAtPoint/RemoveActiveObject/SetVelocity calls (referencing
PopAndMultiply, _physicsSystem.ListStaticObjects, RemoveActiveObject,
ActivateAtPoint, SetVelocity, and _objectTemplates.CreateSmallBall to locate the
code).
| // If circle is inside, reverse the normal | ||
| if (!circleIsOutside) | ||
| { | ||
| normal = -normal; | ||
| // Penetration is radius plus distance to closest edge | ||
| m.Penetration = radius + dist; | ||
| } |
There was a problem hiding this comment.
Inside-circle penetration uses the wrong formula.
Line 343 adds the distance to the radius, which over-penetrates when the circle center is inside the polygon. Penetration should be radius - dist (same as outside, but with flipped normal).
✅ Proposed fix
- m.Penetration = radius + dist;
+ m.Penetration = Math.Max(radius - dist, 0f);🤖 Prompt for AI Agents
In `@physics/Engine/Collision.cs` around lines 338 - 344, The penetration
calculation for an inside-circle case is wrong: in the block guarded by "if
(!circleIsOutside)" (where you already flip normal = -normal), change the
penetration assignment from m.Penetration = radius + dist to m.Penetration =
radius - dist so penetration equals radius minus distance (same magnitude as the
outside case) with the flipped normal; keep the normal flip and variable names
(circleIsOutside, normal, radius, dist, m.Penetration) as-is.
| <OutputType>Library</OutputType> | ||
| <TargetFramework>net9.0</TargetFramework> | ||
| <RootNamespace>SharpPhysics.Engine</RootNamespace> | ||
| <AssemblyName>SharpPhysics.Engine</AssemblyName> | ||
| <GenerateAssemblyInfo>false</GenerateAssemblyInfo> | ||
| <UseWindowsForms>false</UseWindowsForms> | ||
| <Platforms>AnyCPU</Platforms> | ||
| <PlatformTarget>AnyCPU</PlatformTarget> | ||
| <Nullable>enable</Nullable> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all .csproj files to identify project structure
find . -name "*.csproj" -type f | head -20Repository: dankelley2/sharpPhysics
Length of output: 188
🏁 Script executed:
# Check for any usage of old "physics" namespace in C# files
rg -n 'using\s+physics\b|namespace\s+physics\b' --type csRepository: dankelley2/sharpPhysics
Length of output: 12540
🏁 Script executed:
# Search for SFML.Net references and versions
rg -n 'SFML\.Net' --type csproj --type propsRepository: dankelley2/sharpPhysics
Length of output: 97
🏁 Script executed:
# Check all target frameworks in the repository
rg -n 'TargetFramework' --type csproj | head -20Repository: dankelley2/sharpPhysics
Length of output: 97
🏁 Script executed:
# Look at the actual SharpPhysics.csproj file in full
cat physics/SharpPhysics.csprojRepository: dankelley2/sharpPhysics
Length of output: 819
🏁 Script executed:
cat SharpPhysics.Tests/SharpPhysics.Tests.csprojRepository: dankelley2/sharpPhysics
Length of output: 1517
🏁 Script executed:
cat SharpPhysics.Demo/SharpPhysics.Demo.csprojRepository: dankelley2/sharpPhysics
Length of output: 898
🏁 Script executed:
# Check for any namespace changes in git (if this is a git repo)
git log --oneline -n 20 -- physics/SharpPhysics.csproj 2>/dev/null || echo "Not a git repo or limited history"Repository: dankelley2/sharpPhysics
Length of output: 106
Fix the RootNamespace/AssemblyName mismatch—code still uses physics.Engine namespace, not SharpPhysics.Engine
The csproj declares RootNamespace and AssemblyName as SharpPhysics.Engine, but all C# files in the physics, Demo, and Tests projects still use the physics.Engine namespace. This creates a mismatch where the assembly name doesn't match its contained code namespaces. Either update all source files to use the new SharpPhysics.Engine namespace, or revert the csproj changes to keep the existing physics root namespace. Additionally, consider pinning the SFML.Net version to ensure compatibility across projects.
🤖 Prompt for AI Agents
In `@physics/SharpPhysics.csproj` around lines 4 - 12, The project file
SharpPhysics.csproj declares RootNamespace and AssemblyName as
SharpPhysics.Engine which conflicts with existing source files using the
physics.Engine namespace; fix by making them consistent: either change all
source file namespaces from physics.Engine to SharpPhysics.Engine (update
namespace declarations in classes across physics, Demo, and Tests) or revert
RootNamespace and AssemblyName in SharpPhysics.csproj to physics.Engine so they
match current code; additionally pin the SFML.Net package version in the project
file to a specific known-good version to ensure compatibility across projects.
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "ProjectorSegmentation.Vision", "..\..\ProjectorSegmentation\src\ProjectorSegmentation.Vision\ProjectorSegmentation.Vision.csproj", "{58D568AA-7BA6-90BF-B217-BF1497DB529A}" | ||
| EndProject | ||
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "ProjectorSegmentation.Demo", "..\..\ProjectorSegmentation\src\ProjectorSegmentation.Demo\ProjectorSegmentation.Demo.csproj", "{C3112F8F-13A2-AC9D-988A-397C4B3B3A44}" | ||
| EndProject | ||
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "SharpPhysics.Demo", "SharpPhysics.Demo\SharpPhysics.Demo.csproj", "{DC24BC1D-93C6-4062-B20C-A7C34BF60C88}" | ||
| EndProject |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if README.md exists and search for documentation about external dependencies
if [ -f "README.md" ]; then
echo "=== README.md exists ==="
echo "Searching for ProjectorSegmentation mentions:"
rg -i "ProjectorSegmentation" README.md
echo -e "\n=== Searching for clone instructions ==="
rg -i "clone" README.md
else
echo "README.md not found"
fi
# Check actual project structure to understand dependencies
echo -e "\n=== Repository structure ==="
git ls-files | grep -E "(\.sln|\.csproj)" | head -20Repository: dankelley2/sharpPhysics
Length of output: 455
🏁 Script executed:
# Get the full context around ProjectorSegmentation mentions in README
echo "=== Full context around ProjectorSegmentation in README ==="
rg -B 3 -A 3 "ProjectorSegmentation" README.md
# Also check for setup/build/installation sections
echo -e "\n=== Looking for setup/build/installation sections ==="
rg -i "(setup|installation|build|requirements|dependencies)" README.md | head -20
# Get full README to understand its structure
echo -e "\n=== README.md full content ==="
wc -l README.md
head -100 README.mdRepository: dankelley2/sharpPhysics
Length of output: 4677
External project references lack setup documentation.
The ProjectorSegmentation.Vision and ProjectorSegmentation.Demo projects reference paths outside this repository (..\..\ProjectorSegmentation\...), which assumes both repositories are cloned as siblings. While the README mentions the ProjectorSegmentation project exists, the Installation and Setup section provides no guidance on this multi-repository structure. Contributors cloning only sharpPhysics will encounter build failures.
Add explicit setup instructions documenting how to organize both repositories, or make these projects optional/conditionally included in the solution.
🤖 Prompt for AI Agents
In `@PhysicsEngine.sln` around lines 10 - 15, The solution contains external
project references (ProjectorSegmentation.Vision and ProjectorSegmentation.Demo)
pointing to sibling repo paths which break builds for contributors who only
clone this repo; update docs and/or solution so builds are reproducible: add
clear setup instructions in the README/Installation section describing that
ProjectorSegmentation must be cloned as a sibling directory (example structure
and git clone commands), or make the external projects optional by
unloading/guarding them in the solution (remove or conditionally include
ProjectorSegmentation.Vision and ProjectorSegmentation.Demo from
PhysicsEngine.sln and add a note how to re-add them), and include a brief
troubleshooting note about build errors referencing those project GUIDs
({58D568AA-7BA6-90BF-B217-BF1497DB529A}, {C3112F8F-13A2-AC9D-988A-397C4B3B3A44})
so contributors know how to proceed.
| else if (_isMousePressedRight && !_isCreatingBox) | ||
| { | ||
| if (_engine.PhysicsSystem.ActivateAtPoint(_mousePosition)) | ||
| { | ||
| _engine.PhysicsSystem.RemoveActiveObject(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's search for RemoveActiveObject usage as suggested
rg -n "RemoveActiveObject" --type=cs -B3 -A3Repository: dankelley2/sharpPhysics
Length of output: 2225
🏁 Script executed:
# Also search for the file and understand the broader context
find . -name "DemoGame.cs" -type fRepository: dankelley2/sharpPhysics
Length of output: 98
🏁 Script executed:
# Get the full context of the problematic code section
rg -n "_isMousePressedRight" --type=cs -B5 -A5Repository: dankelley2/sharpPhysics
Length of output: 3139
🏁 Script executed:
# Get more context around line 424
sed -n '410,435p' SharpPhysics.Demo/DemoGame.csRepository: dankelley2/sharpPhysics
Length of output: 948
🏁 Script executed:
# Get the full OnMouseMoved method and surrounding context
sed -n '474,510p' SharpPhysics.Demo/DemoGame.csRepository: dankelley2/sharpPhysics
Length of output: 1478
🏁 Script executed:
# Check the ActivateAtPoint method to understand what it does
rg -n "ActivateAtPoint" --type=cs -B2 -A8Repository: dankelley2/sharpPhysics
Length of output: 5301
Right-mouse deletion logic causes unintended continuous object removal during mouse drag.
When right-clicking on an object, RemoveActiveObject() is called immediately in OnMouseButtonPressed (line 424). However, _isMousePressedRight remains true throughout the drag, causing OnMouseMoved to repeatedly attempt deletion on any objects under the cursor. This differs from left-click behavior, which only grabs the initially targeted object and doesn't repeat during drag motion. If the cursor moves over other objects while dragging, they will be deleted as well.
Consider either: (1) removing the deletion logic from OnMouseMoved, (2) tracking whether an object was already deleted in this press cycle, or (3) only deleting on button release instead of press.
🤖 Prompt for AI Agents
In `@SharpPhysics.Demo/DemoGame.cs` around lines 493 - 499, The right-mouse
deletion currently runs in the OnMouseMoved path (when _isMousePressedRight &&
!_isCreatingBox) calling _engine.PhysicsSystem.RemoveActiveObject() repeatedly;
change this so deletion happens only once per press: either remove the deletion
logic from the OnMouseMoved block and keep removal in OnMouseButtonPressed, or
add a press-cycle flag (e.g. _rightDeletedThisPress) that you set when
RemoveActiveObject() is called and check/reset on button release; update the
handlers OnMouseButtonPressed (where initial deletion currently occurs),
OnMouseMoved (remove or gate the call to
_engine.PhysicsSystem.RemoveActiveObject()), and OnMouseButtonReleased to reset
the flag if you choose the press-cycle approach.
| private void HandleDebugInput(KeyState keyState) | ||
| { | ||
| // Space to spawn extra balls (for testing) | ||
| if (keyState.Space) | ||
| { | ||
| for (int i = 0; i < 5; i++) | ||
| { | ||
| SpawnBall(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Space key spawns balls continuously every frame while held.
keyState.Space is true for every frame the key is held, spawning 5 balls per frame. This could quickly exceed MAX_BALLS. Consider adding a cooldown or checking for key-down transition only.
🐛 Proposed fix with cooldown
+ private bool _spaceWasPressed = false;
+
private void HandleDebugInput(KeyState keyState)
{
// Space to spawn extra balls (for testing)
- if (keyState.Space)
+ if (keyState.Space && !_spaceWasPressed)
{
for (int i = 0; i < 5; i++)
{
SpawnBall();
}
}
+ _spaceWasPressed = keyState.Space;
}🤖 Prompt for AI Agents
In `@SharpPhysics.Demo/RainCatcherGame.cs` around lines 494 - 504, The Space key
handling in HandleDebugInput currently spawns 5 balls every frame while
keyState.Space is true, causing uncontrolled spawning; change it to only trigger
on a key-down transition or implement a cooldown to respect MAX_BALLS: add state
(e.g., a private bool lastSpaceDown or a float spaceCooldown) to detect when
Space goes from false to true (or decrement cooldown each update and only spawn
when <=0), call SpawnBall up to remaining capacity (respecting MAX_BALLS) and
update the state/cooldown after spawning; reference HandleDebugInput,
keyState.Space, SpawnBall and MAX_BALLS to locate where to apply the change.
| <ProjectReference Include="..\physics\SharpPhysics.csproj" /> | ||
| <ProjectReference Include="..\..\..\ProjectorSegmentation\src\ProjectorSegmentation.Vision\ProjectorSegmentation.Vision.csproj" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
External project reference depends on specific directory structure.
The reference to ProjectorSegmentation.Vision.csproj uses a relative path that navigates outside this repository. This creates a hard dependency on a sibling repository being present at a specific location.
As noted in the solution file review, this will cause build failures for contributors who only clone this repository. Consider documenting the required setup or making this dependency optional.
🤖 Prompt for AI Agents
In `@SharpPhysics.Demo/SharpPhysics.Demo.csproj` around lines 13 - 15,
SharpPhysics.Demo.csproj currently contains a hard-coded ProjectReference to
..\..\..\ProjectorSegmentation\src\ProjectorSegmentation.Vision\ProjectorSegmentation.Vision.csproj
which forces a specific sibling-repo layout; change this to either (A) make the
reference conditional by wrapping the ProjectReference with an MSBuild Condition
that only includes it when a property like ProjectorSegmentationPath is set
(e.g., use Condition="'$(ProjectorSegmentationPath)' != ''" and reference via
that property), or (B) replace the ProjectReference with a PackageReference to a
published ProjectorSegmentation.Vision NuGet package, and add clear README setup
instructions for contributors to either set ProjectorSegmentationPath or install
the package; update SharpPhysics.Demo.csproj and project documentation
accordingly.
See https://github.com/dankelley2/ProjectorSegmentation for second project in solution; Will merge them eventually
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.