Demo project refactor, Renderer memory cleanup #16
Conversation
📝 WalkthroughWalkthroughRefactored rendering to a layered pipeline (BeginFrame → RenderBackground → RenderPhysicsObjects → Render → Display); added DemoSceneBuilder, SandboxDebugUI, AnimatedBackground; enabled debug UI by removing explicit hides in several games; introduced SpringConstraint and related helper; removed ObjectTemplates dependency from ActionTemplates; adjusted circle tessellation. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
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
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
physics/Engine/Constraints/Constraint.cs (2)
97-100:⚠️ Potential issue | 🟠 MajorPotential bug:
LengthSquared()compared against non-squared threshold.
posError.LengthSquared()returns the squared distance, but it's compared against30f. If the intent is to break at 30 units distance, this should either useLength() > 30forLengthSquared() > 900f.Currently, the constraint breaks at ~5.5 units (
sqrt(30)), which may not be intended. Note thatSpringConstraint(line 278) correctly usesLength()for its comparison.🐛 Proposed fix (assuming 30 units was intended)
// break - if (CanBreak && (Math.Abs(posError.LengthSquared()) > 30f || Math.Abs(angleError) > 0.5f)) + if (CanBreak && (posError.Length() > 30f || Math.Abs(angleError) > 0.5f)) { IsBroken = true; }
193-196:⚠️ Potential issue | 🟠 MajorMissing
overridekeyword onPrepareForTick.The base
Constraintclass declaresPrepareForTick()asvirtual(line 46), butWeldConstraint.PrepareForTick()doesn't useoverride. This shadows the base method, so polymorphic calls throughConstraintreferences won't invoke this implementation.🐛 Proposed fix
/// <summary> /// Call at the start of each physics tick (before substeps) to enable warm starting. /// </summary> - public void PrepareForTick() + public override void PrepareForTick() { _warmStartAppliedThisTick = false; }
🤖 Fix all issues with AI agents
In @.github/copilot-instructions.md:
- Around line 63-82: The fenced directory listing in
.github/copilot-instructions.md is missing a language tag; update the opening
code fence for the block that starts with "physics/Engine/" to include a
language such as "text" (e.g., change ``` to ```text) so the block satisfies
MD040 and retain the trailing closing fence; no other content changes required.
In `@physics/Engine/Constraints/Constraint.cs`:
- Around line 237-241: The XML summary on the SpringConstraint type is incorrect
(it describes a weld constraint); update the XML documentation for the
SpringConstraint class (the summary block near the top of the type in
Constraint.cs where SpringConstraint is declared) to describe spring behavior:
that it connects two bodies with a spring-damper that applies forces to
maintain/restoring the distance between local anchors and damp relative
velocity, and mention any stiffness/damping parameters used by the
SpringConstraint constructor/fields. Ensure the summary replaces the
weld-specific wording with accurate spring-specific terms and references the
class name SpringConstraint.
In `@physics/Engine/Helpers/ConstraintHelper.cs`:
- Around line 29-35: In AddSpringConstraint (in ConstraintHelper.cs) rename the
misleading local variable (currently named weldConstraint) to springConstraint
and add a canBreak parameter to the method signature (e.g., bool canBreak =
false) so the helper mirrors AddWeldConstraint overloads; pass that canBreak
value into the SpringConstraint constructor when instantiating (new
SpringConstraint(objA, objB, halfdiff, -halfdiff, canBreak)) and add it to the
returned/added constraint path
(engine.PhysicsSystem.Constraints.Add(springConstraint); return
springConstraint;).
In `@physics/Engine/Rendering/Renderer.cs`:
- Around line 16-17: The Renderer class must implement IDisposable and release
SFML native resources: add IDisposable to the Renderer type and implement a
public Dispose() that calls a private Dispose(bool disposing); in Dispose(bool)
when disposing is true call Dispose() or Close() (as appropriate) on
_defaultFont, _reusableText, _lineRenderer, _circleShapeRenderer, and
_rectangleShapeRenderer if they are non-null and then null them; call
GC.SuppressFinalize(this) in public Dispose() and optionally add a finalizer
that calls Dispose(false) if you need a safety net. Ensure you reference the
field names exactly (_defaultFont, _reusableText, _lineRenderer,
_circleShapeRenderer, _rectangleShapeRenderer) and make Dispose idempotent.
- Around line 47-48: The constructor currently instantiates _defaultFont and
_reusableText directly which will throw if "Resources/good_timing_bd.otf" is
missing; update the Renderer constructor (or the code that sets
_defaultFont/_reusableText) to first check
File.Exists("Resources/good_timing_bd.otf") or wrap the Font(...) call in a
try-catch, and on failure fall back to a safe default Font (or log the error and
use SystemFonts.DefaultFont) before creating _reusableText so the renderer still
initializes without throwing.
In `@physics/Engine/Shapes/CirclePhysShape.cs`:
- Line 26: Rename the misspelled local variable mulitplier to multiplier in
CirclePhysShape.cs where it's defined (the expression "var mulitplier =
Math.Max(1,(int)radius / 20);") and update any subsequent usages to the new name
to preserve behavior; ensure the cast and Math.Max logic remain unchanged and
run a build to catch any references that need renaming.
In `@SharpPhysics.Demo/DemoProps/SandboxDebugUI.cs`:
- Around line 84-90: The displayed "num objects" currently uses
_physics.ListStaticObjects.Count (in the renderer.DrawText call inside
SandboxDebugUI.cs), which only shows static objects; update the count expression
to show the total by summing dynamic and static lists (use
_physics.ListGravityObjects.Count + _physics.ListStaticObjects.Count) so the
label "num objects" is correct, or alternatively change the label to "num static
objects" if you intend to keep only the static count.
🧹 Nitpick comments (7)
physics/Engine/Shapes/CirclePhysShape.cs (1)
24-36: Consider extracting the resolution calculation or reusingPolygonShapeHelper.CreateCircleVertices.The
PolygonShapeHelper.CreateCircleVertices(int resolution, float radius)method inphysics/Engine/Helpers/PolygonShapeHelper.csalready generates circle vertices. You could either:
- Calculate the resolution here and delegate to the helper, or
- Add an overload to the helper that auto-calculates resolution from radius.
This would reduce duplication and centralize the circle vertex generation logic.
physics/Engine/Rendering/Renderer.cs (1)
218-224: Remove commented-out code.The old
RectangleShapecreation code is no longer needed since you've switched to reusing_rectangleShapeRenderer.🧹 Proposed fix
public void DrawRectangle(Vector2 position, Vector2 size, Color fillColor, Color? outlineColor = null, float outlineThickness = 0f) { Window.SetView(GameView); - //var rect = new RectangleShape(new Vector2f(size.X, size.Y)) - //{ - // Position = new Vector2f(position.X, position.Y), - // FillColor = fillColor, - // OutlineColor = outlineColor ?? Color.Transparent, - // OutlineThickness = outlineThickness - //}; - _rectangleShapeRenderer.Size = new Vector2f(size.X, size.Y);physics/Engine/Classes/Templates/ActionTemplates.cs (1)
17-23: Simplify Vector2 delta calculation.The Vector2 initialization is unnecessarily verbose.
✨ Proposed simplification
public void Launch(PhysicsObject physObj, Vector2 StartPointF, Vector2 EndPointF) { _physicsSystem.ActivateAtPoint(StartPointF); - 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); }physics/Engine/Constraints/Constraint.cs (1)
198-234: Consider extractingApplyLinearImpulseto the baseConstraintclass.The
ApplyLinearImpulsemethod is nearly identical betweenWeldConstraintandSpringConstraint. The only differences are the Baumgarte constants. Consider moving the method to the base class and making the bias values configurable via protected properties or constructor parameters.Also applies to: 318-354
SharpPhysics.Demo/MenuGame.cs (1)
23-31: Consider making the dictionaryreadonly.
GameFactoriesisprivate staticbut never modified after initialization. Addingreadonlyclarifies intent and prevents accidental reassignment.- private static readonly Dictionary<string, Func<IGame>> GameFactories = new() + private static readonly Dictionary<string, Func<IGame>> GameFactories = new()Actually, the code already has
readonly. LGTM!SharpPhysics.Demo/DemoProps/DemoSceneBuilder.cs (2)
47-48: Magic offset values in wheel positioning.The
-10offsets applied to wheel positions are unexplained. If these compensate for ball creation centering behavior, consider extracting a named constant or adding a brief comment.- var frontWheel = _templates.CreateLargeBall(frontWheelWorldX - 10, wheelWorldY - 10); - var rearWheel = _templates.CreateLargeBall(rearWheelWorldX - 10, wheelWorldY - 10); + // Offset to account for ball creation using top-left corner positioning + const float ballOffset = 10f; + var frontWheel = _templates.CreateLargeBall(frontWheelWorldX - ballOffset, wheelWorldY - ballOffset); + var rearWheel = _templates.CreateLargeBall(rearWheelWorldX - ballOffset, wheelWorldY - ballOffset);
88-88: Consistent magic offset in ball creation.Similar to
CreateCar, this-10offset appears inCreateMedBall. If this is a pattern for the template methods, consider documenting it or adjusting the template API.
| ``` | ||
| physics/Engine/ | ||
| ├── Core/ # GameEngine, IGame interface | ||
| ├── Rendering/ # Renderer, UI components | ||
| ├── Input/ # InputManager | ||
| ├── Objects/ # PhysicsObject | ||
| ├── Shapes/ # CirclePhysShape, PolygonPhysShape, etc. | ||
| ├── Shaders/ # SFML shader implementations | ||
| ├── Constraints/ # Physics constraints (Weld, Axis, Spring) | ||
| ├── Helpers/ # Math utilities, extensions | ||
| └── Classes/ # Templates, collision data | ||
|
|
||
| SharpPhysics.Demo/ | ||
| ├── DemoProps/ # Demo-specific components (SandboxDebugUI, DemoSceneBuilder) | ||
| ├── Helpers/ # Demo utilities (AnimatedBackground, SkeletonRenderer) | ||
| ├── Integration/ # PersonColliderBridge for pose tracking | ||
| ├── Designer/ # Prefab designer game | ||
| ├── Settings/ # GameSettings configuration | ||
| └── [Game].cs # Individual game implementations | ||
| ``` |
There was a problem hiding this comment.
Add a language to the fenced code block to satisfy MD040.
This block should specify a language (e.g., text) to avoid markdownlint warnings and keep formatting consistent.
🔧 Proposed fix
-```
+```text
physics/Engine/
├── Core/ # GameEngine, IGame interface
├── Rendering/ # Renderer, UI components
├── Input/ # InputManager
├── Objects/ # PhysicsObject
├── Shapes/ # CirclePhysShape, PolygonPhysShape, etc.
├── Shaders/ # SFML shader implementations
├── Constraints/ # Physics constraints (Weld, Axis, Spring)
├── Helpers/ # Math utilities, extensions
└── Classes/ # Templates, collision data
SharpPhysics.Demo/
├── DemoProps/ # Demo-specific components (SandboxDebugUI, DemoSceneBuilder)
├── Helpers/ # Demo utilities (AnimatedBackground, SkeletonRenderer)
├── Integration/ # PersonColliderBridge for pose tracking
├── Designer/ # Prefab designer game
├── Settings/ # GameSettings configuration
└── [Game].cs # Individual game implementations
-```
+```📝 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.
| ``` | |
| physics/Engine/ | |
| ├── Core/ # GameEngine, IGame interface | |
| ├── Rendering/ # Renderer, UI components | |
| ├── Input/ # InputManager | |
| ├── Objects/ # PhysicsObject | |
| ├── Shapes/ # CirclePhysShape, PolygonPhysShape, etc. | |
| ├── Shaders/ # SFML shader implementations | |
| ├── Constraints/ # Physics constraints (Weld, Axis, Spring) | |
| ├── Helpers/ # Math utilities, extensions | |
| └── Classes/ # Templates, collision data | |
| SharpPhysics.Demo/ | |
| ├── DemoProps/ # Demo-specific components (SandboxDebugUI, DemoSceneBuilder) | |
| ├── Helpers/ # Demo utilities (AnimatedBackground, SkeletonRenderer) | |
| ├── Integration/ # PersonColliderBridge for pose tracking | |
| ├── Designer/ # Prefab designer game | |
| ├── Settings/ # GameSettings configuration | |
| └── [Game].cs # Individual game implementations | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 63-63: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In @.github/copilot-instructions.md around lines 63 - 82, The fenced directory
listing in .github/copilot-instructions.md is missing a language tag; update the
opening code fence for the block that starts with "physics/Engine/" to include a
language such as "text" (e.g., change ``` to ```text) so the block satisfies
MD040 and retain the trailing closing fence; no other content changes required.
| /// <summary> | ||
| /// A weld constraint holds two objects together so that the world-space positions | ||
| /// of their respective local anchors remain coincident, and their relative angle remains constant. | ||
| /// Uses proper impulse-based constraint solving with Baumgarte stabilization. | ||
| /// </summary> |
There was a problem hiding this comment.
Incorrect documentation: copy-pasted from WeldConstraint.
The XML documentation describes a "weld constraint" but this is SpringConstraint. Update the summary to describe the spring behavior.
📝 Proposed fix
/// <summary>
- /// A weld constraint holds two objects together so that the world-space positions
- /// of their respective local anchors remain coincident, and their relative angle remains constant.
- /// Uses proper impulse-based constraint solving with Baumgarte stabilization.
+ /// A spring constraint is a soft constraint that holds two objects together with
+ /// lower stiffness than a weld, allowing more flexibility before breaking.
+ /// Uses impulse-based constraint solving with reduced Baumgarte stabilization.
/// </summary>🤖 Prompt for AI Agents
In `@physics/Engine/Constraints/Constraint.cs` around lines 237 - 241, The XML
summary on the SpringConstraint type is incorrect (it describes a weld
constraint); update the XML documentation for the SpringConstraint class (the
summary block near the top of the type in Constraint.cs where SpringConstraint
is declared) to describe spring behavior: that it connects two bodies with a
spring-damper that applies forces to maintain/restoring the distance between
local anchors and damp relative velocity, and mention any stiffness/damping
parameters used by the SpringConstraint constructor/fields. Ensure the summary
replaces the weld-specific wording with accurate spring-specific terms and
references the class name SpringConstraint.
| public static Constraint AddSpringConstraint(this GameEngine engine, PhysicsObject objA, PhysicsObject objB) | ||
| { | ||
| var halfdiff = (objB.Center - objA.Center) / 2f; | ||
| var weldConstraint = new SpringConstraint(objA, objB, halfdiff, -halfdiff); | ||
| engine.PhysicsSystem.Constraints.Add(weldConstraint); | ||
| return weldConstraint; | ||
| } |
There was a problem hiding this comment.
Misleading variable name and missing canBreak parameter.
The variable is named weldConstraint but it holds a SpringConstraint. Additionally, the SpringConstraint constructor supports a canBreak parameter (see physics/Engine/Constraints/Constraint.cs line 249), but this helper doesn't expose it, unlike the AddWeldConstraint overloads.
Proposed fix
- public static Constraint AddSpringConstraint(this GameEngine engine, PhysicsObject objA, PhysicsObject objB)
+ public static Constraint AddSpringConstraint(this GameEngine engine, PhysicsObject objA, PhysicsObject objB, bool canBreak = false)
{
var halfdiff = (objB.Center - objA.Center) / 2f;
- var weldConstraint = new SpringConstraint(objA, objB, halfdiff, -halfdiff);
- engine.PhysicsSystem.Constraints.Add(weldConstraint);
- return weldConstraint;
+ var springConstraint = new SpringConstraint(objA, objB, halfdiff, -halfdiff, canBreak);
+ engine.PhysicsSystem.Constraints.Add(springConstraint);
+ return springConstraint;
}📝 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 static Constraint AddSpringConstraint(this GameEngine engine, PhysicsObject objA, PhysicsObject objB) | |
| { | |
| var halfdiff = (objB.Center - objA.Center) / 2f; | |
| var weldConstraint = new SpringConstraint(objA, objB, halfdiff, -halfdiff); | |
| engine.PhysicsSystem.Constraints.Add(weldConstraint); | |
| return weldConstraint; | |
| } | |
| public static Constraint AddSpringConstraint(this GameEngine engine, PhysicsObject objA, PhysicsObject objB, bool canBreak = false) | |
| { | |
| var halfdiff = (objB.Center - objA.Center) / 2f; | |
| var springConstraint = new SpringConstraint(objA, objB, halfdiff, -halfdiff, canBreak); | |
| engine.PhysicsSystem.Constraints.Add(springConstraint); | |
| return springConstraint; | |
| } |
🤖 Prompt for AI Agents
In `@physics/Engine/Helpers/ConstraintHelper.cs` around lines 29 - 35, In
AddSpringConstraint (in ConstraintHelper.cs) rename the misleading local
variable (currently named weldConstraint) to springConstraint and add a canBreak
parameter to the method signature (e.g., bool canBreak = false) so the helper
mirrors AddWeldConstraint overloads; pass that canBreak value into the
SpringConstraint constructor when instantiating (new SpringConstraint(objA,
objB, halfdiff, -halfdiff, canBreak)) and add it to the returned/added
constraint path (engine.PhysicsSystem.Constraints.Add(springConstraint); return
springConstraint;).
| private Text _reusableText; | ||
| private Font _defaultFont; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Renderer should implement IDisposable to clean up SFML resources.
The class holds several SFML objects that wrap native resources: _defaultFont, _reusableText, _lineRenderer, _circleShapeRenderer, and _rectangleShapeRenderer. These should be properly disposed when the renderer is no longer needed.
♻️ Proposed implementation
- public class Renderer
+ public class Renderer : IDisposable
{
// ... existing code ...
+ public void Dispose()
+ {
+ _reusableText?.Dispose();
+ _defaultFont?.Dispose();
+ _lineRenderer?.Dispose();
+ _circleShapeRenderer?.Dispose();
+ _rectangleShapeRenderer?.Dispose();
+ }
}Also applies to: 164-164, 183-183, 205-205
🤖 Prompt for AI Agents
In `@physics/Engine/Rendering/Renderer.cs` around lines 16 - 17, The Renderer
class must implement IDisposable and release SFML native resources: add
IDisposable to the Renderer type and implement a public Dispose() that calls a
private Dispose(bool disposing); in Dispose(bool) when disposing is true call
Dispose() or Close() (as appropriate) on _defaultFont, _reusableText,
_lineRenderer, _circleShapeRenderer, and _rectangleShapeRenderer if they are
non-null and then null them; call GC.SuppressFinalize(this) in public Dispose()
and optionally add a finalizer that calls Dispose(false) if you need a safety
net. Ensure you reference the field names exactly (_defaultFont, _reusableText,
_lineRenderer, _circleShapeRenderer, _rectangleShapeRenderer) and make Dispose
idempotent.
| _defaultFont = new Font("Resources/good_timing_bd.otf"); | ||
| _reusableText = new Text("", _defaultFont, 24); |
There was a problem hiding this comment.
Font loading may throw if the file is missing.
The font is loaded from a hardcoded path without error handling. If "Resources/good_timing_bd.otf" doesn't exist, this will throw an exception during construction.
Consider wrapping in a try-catch or checking file existence first.
🤖 Prompt for AI Agents
In `@physics/Engine/Rendering/Renderer.cs` around lines 47 - 48, The constructor
currently instantiates _defaultFont and _reusableText directly which will throw
if "Resources/good_timing_bd.otf" is missing; update the Renderer constructor
(or the code that sets _defaultFont/_reusableText) to first check
File.Exists("Resources/good_timing_bd.otf") or wrap the Font(...) call in a
try-catch, and on failure fall back to a safe default Font (or log the error and
use SystemFonts.DefaultFont) before creating _reusableText so the renderer still
initializes without throwing.
| for (int i = 0; i < SEGMENTS; i++) | ||
|
|
||
| // multiples of 20 | ||
| var mulitplier = Math.Max(1,(int)radius / 20); |
There was a problem hiding this comment.
Typo: mulitplier should be multiplier.
Variable name has a spelling error that affects code readability.
✏️ Proposed fix
- var mulitplier = Math.Max(1,(int)radius / 20);
+ var multiplier = Math.Max(1, (int)radius / 20);📝 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.
| var mulitplier = Math.Max(1,(int)radius / 20); | |
| var multiplier = Math.Max(1, (int)radius / 20); |
🤖 Prompt for AI Agents
In `@physics/Engine/Shapes/CirclePhysShape.cs` at line 26, Rename the misspelled
local variable mulitplier to multiplier in CirclePhysShape.cs where it's defined
(the expression "var mulitplier = Math.Max(1,(int)radius / 20);") and update any
subsequent usages to the new name to preserve behavior; ensure the cast and
Math.Max logic remain unchanged and run a build to catch any references that
need renaming.
| // Performance stats | ||
| renderer.DrawText( | ||
| $"ms physics time: {_engine.MsPhysicsTime}\n" + | ||
| $"ms draw time: {_engine.MsDrawTime}\n" + | ||
| $"frame rate: {1000 / Math.Max(_engine.MsFrameTime, 1)}\n" + | ||
| $"num objects: {_physics.ListStaticObjects.Count}", | ||
| 40, 40, 12, Color.White); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the PhysicsSystem class definition
rg -n "class PhysicsSystem" --type=cs -A 20Repository: dankelley2/sharpPhysics
Length of output: 1619
🏁 Script executed:
#!/bin/bash
# Search for all List*Objects or similar count properties
rg -n "List.*Objects|Count.*Objects|_objects" --type=cs | head -50Repository: dankelley2/sharpPhysics
Length of output: 2969
🏁 Script executed:
#!/bin/bash
# Find where _physics is defined/initialized in SandboxDebugUI
rg -n "_physics" SharpPhysics.Demo/DemoProps/SandboxDebugUI.cs -B 2 -A 2Repository: dankelley2/sharpPhysics
Length of output: 2129
Display shows only static object count, not total objects.
The debug display label reads "num objects" but ListStaticObjects.Count only counts static physics objects. The codebase has two separate lists: ListGravityObjects (dynamic) and ListStaticObjects (static). Either change the label to "num static objects" or update the count to show the total: _physics.ListGravityObjects.Count + _physics.ListStaticObjects.Count.
🤖 Prompt for AI Agents
In `@SharpPhysics.Demo/DemoProps/SandboxDebugUI.cs` around lines 84 - 90, The
displayed "num objects" currently uses _physics.ListStaticObjects.Count (in the
renderer.DrawText call inside SandboxDebugUI.cs), which only shows static
objects; update the count expression to show the total by summing dynamic and
static lists (use _physics.ListGravityObjects.Count +
_physics.ListStaticObjects.Count) so the label "num objects" is correct, or
alternatively change the label to "num static objects" if you intend to keep
only the static count.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
physics/Engine/Rendering/Renderer.cs (1)
166-182:⚠️ Potential issue | 🟡 MinorLine thickness parameter is silently ignored in
DrawLinemethod.The
thicknessparameter (lines 166, 174) is accepted but never used. SFML'sPrimitiveType.Linesrenders at 1 pixel only. Multiple callers expect varying thicknesses to work (skeleton rendering passes 3f, constraints use 2f, etc.), but all currently render at 1px. Consider usingRectangleShape(as already done inDesignerRenderer.cs) or the suggested quad-based approach to honor thickness.
🧹 Nitpick comments (1)
physics/Engine/Rendering/Renderer.cs (1)
207-235: Remove commented-out block to keep the method clean.The old RectangleShape block is dead code and can be deleted.
✂️ Cleanup
- //var rect = new RectangleShape(new Vector2f(size.X, size.Y)) - //{ - // Position = new Vector2f(position.X, position.Y), - // FillColor = fillColor, - // OutlineColor = outlineColor ?? Color.Transparent, - // OutlineThickness = outlineThickness - //};
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.