Actionable comments posted: 5

In @src/plugin/physics/src/component/CapsuleCollider.hpp:
- Around line 54-60: The example in CapsuleCollider.hpp uses an inconsistent
type name; replace or alias so the example matches the actual type: change
references of Physics::CapsuleCollider to Physics::Component::CapsuleCollider
(or add a using/typedef alias named Physics::CapsuleCollider that points to
Physics::Component::CapsuleCollider), and ensure the entity.AddComponent<> call
uses the same symbol (e.g.,
entity.AddComponent<Physics::Component::CapsuleCollider>(core, collider)); make
the same consistent fix pattern used for SphereCollider examples.
- Around line 114-120: SetTotalHeight(float) and ForCharacter(...) can produce
negative halfHeight for small total heights; change SetTotalHeight to delegate
to FromTotalHeight(totalHeight, radius) (or perform the same clamped logic) so
it uses the existing clamp, and update ForCharacter to call
FromTotalHeight(totalHeight, radius) instead of directly computing halfHeight;
ensure both paths use the same clamping logic that prevents halfHeight < 0.

In @src/plugin/physics/src/component/SphereCollider.hpp:
- Around line 48-53: The example uses an inconsistent type name: change the
example to use Physics::Component::SphereCollider consistently (i.e., replace
Physics::SphereCollider with Physics::Component::SphereCollider in the example
block around the SphereCollider docs), or alternatively add a public alias in
the Physics.hpp header such as `namespace Physics { using SphereCollider =
Component::SphereCollider; }` so Physics::SphereCollider resolves; update the
docs accordingly to keep one canonical type name.

In @src/plugin/physics/src/Physics.hpp:
- Around line 10-22: Physics.hpp currently exposes the internal-only component
RigidBodyInternal; remove the #include "component/RigidBodyInternal.hpp" from
Physics.hpp so it is no longer part of the public umbrella header, and instead
include "component/RigidBodyInternal.hpp" only in the internal system
translation units that actually use it (e.g., RigidBodySystem.cpp and
SyncTransformSystem.cpp); ensure no other public headers depend on
RigidBodyInternal after removal and update those internal files to include the
header directly.

src/plugin/physics/src/system/RigidBodySystem.cpp (1)

44-64: Fallback shapes are fine, but consider whether you also want to “self-heal” invalid component values.
Right now invalid colliders log and use defaults, but the component remains invalid in the registry. Optional: clamp + write back (or remove the invalid collider) to avoid repeated warnings and keep ECS state truthful.

src/plugin/physics/src/component/SphereCollider.hpp (2)

86-97: Consider clamping in SetDiameter() to preserve invariants (or explicitly document “may become invalid”).
Today SetDiameter(-1) silently makes radius negative; downstream code logs and falls back to 0.5. Optional: radius = std::max(0.0f, diameter * 0.5f) (or assert).

---

55-61: offset is exposed but not applied by the current shape creation path.
If offset support isn’t ready, I’d add a short note here (“offset currently unused by physics system”) to avoid user confusion.

src/plugin/physics/src/component/CapsuleCollider.hpp (2)

127-132: IsSphere() exact float equality is a bit fragile; consider epsilon if this is used for branching.
If callers compute halfHeight from arithmetic (e.g., setters), an epsilon check may be safer than == 0.0f.

---

62-71: offset is exposed but not applied by the current shape creation path.
Same as SphereCollider: either apply it in RigidBodySystem.cpp or document that it’s currently ignored.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 85ce8fd and c8dae9d.

• src/plugin/physics/src/Physics.hpp
• src/plugin/physics/src/component/CapsuleCollider.hpp
• src/plugin/physics/src/component/SphereCollider.hpp
• src/plugin/physics/src/system/RigidBodySystem.cpp

src/plugin/physics/src/component/CapsuleCollider.hpp (5)

• nodiscard (99-99)
• nodiscard (105-105)
• nodiscard (111-111)
• nodiscard (125-125)
• nodiscard (131-131)

src/plugin/physics/src/component/SphereCollider.hpp (6)

• r (113-113)
• r (113-113)
• r (121-121)
• r (121-121)
• nodiscard (84-84)
• nodiscard (96-96)

• GitHub Check: Run tests (macos-latest)
• GitHub Check: Build and verify binaries (windows-latest)
• GitHub Check: Build and verify binaries (macos-latest)
• GitHub Check: Run tests (ubuntu-latest)
• GitHub Check: Run tests (windows-latest)
• GitHub Check: Build and verify binaries (ubuntu-latest)

src/plugin/physics/src/system/RigidBodySystem.cpp (1)

7-12: Includes look consistent, but please verify the exact Jolt wrapper/header you’ll need if you add collider offsets.
You’ll likely want an extra Jolt shape include (e.g., a translated/rotated wrapper) once offset is applied; please confirm the correct type/header for the Jolt version you vendor.

Also applies to: 21-23

src/plugin/physics/src/Physics.hpp (1)

10-19: Header re-org is clear and helps discoverability.
The new “Components - Colliders” / “Components - RigidBody” grouping reads well.

Actionable comments posted: 2

In @examples/graphic_usage_with_physics/src/main.cpp:
- Around line 132-145: The capsule collider in CreateCharacterCapsule has total
height 2.0 and diameter 0.5 while the visual uses a scaled cube via
Object::Helper::CreateCube (glm::vec3(0.5f,1.5f,0.5f)), which can be confusing;
either make the render proxy match the collider by adjusting the cube scale to
the capsule’s visual dimensions or add a short clarifying comment above the
Object::Helper::CreateCube call stating the cube is a visual placeholder that
intentionally doesn’t match the CapsuleCollider dimensions (refer to
CreateCharacterCapsule, Physics::Component::CapsuleCollider, and
Object::Helper::CreateCube).

In @src/plugin/physics/src/component/SphereCollider.hpp:
- Around line 1-22: The license header in SphereCollider.hpp is inconsistent by
combining "MIT License" with "All rights reserved"; update the file header
(SphereCollider.hpp) to match the project's MIT wording—remove "All rights
reserved" and ensure the header states redistribution/modification under the MIT
License and references the project's LICENSE file (or replace with the canonical
MIT boilerplate used elsewhere in the repo) so the license text is unambiguous.

src/plugin/physics/src/component/SphereCollider.hpp (1)

56-123: Solid, minimal API; consider guarding SetDiameter against negatives.

IsValid() requires radius > 0 (Line 97), but SetDiameter() (Line 91) can set radius negative. A cheap guard (clamp to 0, assert, or document “caller must pass > 0”) would prevent accidental invalid states.

src/plugin/physics/src/system/RigidBodySystem.cpp (1)

31-94: Code is safe but consider refactoring for clarity.

The current code is actually correct and safe—JPH::RefConst constructor calls AddRef on raw pointers, and RotatedTranslatedShape properly increments the inner shape's ref-count. However, this relies on implicit ref-counting behavior that requires knowledge of Jolt's API. The ownership model would be clearer with explicit RefConst wrapping.

Consider the proposed refactor to introduce a wrapWithOffset helper lambda that:

• Deduplicates the offset-wrapping logic across all four collider branches
• Uses explicit RefConst to make ownership management clearer
• Calls base.GetPtr() to clarify the pointer-passing boundary

This improves readability without changing correctness.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 7060471 and 9884d4f.

• examples/graphic_usage_with_physics/src/main.cpp
• examples/graphic_usage_with_physics/xmake.lua
• src/plugin/physics/src/component/BoxCollider.hpp
• src/plugin/physics/src/component/CapsuleCollider.hpp
• src/plugin/physics/src/component/SphereCollider.hpp
• src/plugin/physics/src/system/RigidBodySystem.cpp

• examples/graphic_usage_with_physics/xmake.lua

• src/plugin/physics/src/component/BoxCollider.hpp

• src/plugin/physics/src/component/CapsuleCollider.hpp

src/plugin/physics/src/component/BoxCollider.hpp (1)

• nodiscard (75-75)

src/plugin/physics/src/component/CapsuleCollider.hpp (5)

• nodiscard (100-100)
• nodiscard (106-106)
• nodiscard (112-112)
• nodiscard (130-130)
• nodiscard (136-136)

src/plugin/physics/src/component/BoxCollider.hpp (4)

• BoxCollider (57-57)
• BoxCollider (63-63)
• BoxCollider (63-63)
• BoxCollider (70-70)

src/plugin/object/src/component/Transform.hpp (6)

• x (47-51)
• x (47-47)
• x (57-61)
• x (57-57)
• x (67-71)
• x (67-67)

src/plugin/physics/src/component/SphereCollider.hpp (5)

• SphereCollider (66-66)
• SphereCollider (72-72)
• SphereCollider (72-72)
• SphereCollider (79-79)
• SphereCollider (107-107)

src/plugin/physics/src/component/CapsuleCollider.hpp (3)

• CapsuleCollider (76-76)
• CapsuleCollider (83-83)
• CapsuleCollider (91-94)

examples/graphic_usage_with_physics/src/main.cpp (3)

99-107: Floor visual vs collider alignment: please sanity-check the contact plane.

With entity position y = -1.0 (Line 101) and BoxCollider halfExtents.y = 1.0 (Line 103), the collider’s top surface lands at y = 0.0. If the rendered plane is still visually at y = -1.0, objects may appear to float 1 unit above the mesh.

---

119-130: Bouncy ball setup looks consistent (mesh radius == collider radius).

---

147-162: Scene additions + camera repositioning are straightforward.

Actionable comments posted: 0

examples/graphic_usage_with_physics/src/main.cpp (2)

101-103: Consider aligning floor visual and collider top surfaces.

The plane is positioned at y=-1.0f, while the box collider (with halfExtents.y=1.0f centered at y=-1.0f) has its top surface at y=0.0f. This means objects resting on the collider will appear to float 1 unit above the visible floor plane, which may look odd to users.

If the floor should visually align with the physics surface, consider either:

• Moving the plane to y=0.0f, or
• Adjusting the collider halfExtents.y to 0.0f (making it infinitely thin at y=-1), or
• Using an offset in the BoxCollider constructor to shift the collider down by 1 unit

---

132-145: Consider using capsule geometry for visual consistency.

The collider dimensions are correctly calculated (total height 2.0f = 2×halfHeight + 2×radius = 2×0.75 + 2×0.25), and the physics properties are well-suited for a character controller. However, the visual representation uses a scaled cube rather than an actual capsule mesh.

While functional, this creates a shape mismatch between what users see (a box) and the actual collision shape (a capsule). Consider using a capsule mesh helper (if available) or creating a custom capsule geometry for better visual fidelity.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 9884d4f and c9605b1.

• examples/graphic_usage_with_physics/src/main.cpp

src/plugin/physics/src/component/BoxCollider.hpp (4)

• BoxCollider (57-57)
• BoxCollider (63-63)
• BoxCollider (63-63)
• BoxCollider (70-70)

src/plugin/physics/src/component/SphereCollider.hpp (5)

• SphereCollider (66-66)
• SphereCollider (72-72)
• SphereCollider (72-72)
• SphereCollider (79-79)
• SphereCollider (107-107)

src/plugin/physics/src/component/CapsuleCollider.hpp (3)

• CapsuleCollider (76-76)
• CapsuleCollider (83-83)
• CapsuleCollider (91-94)

• GitHub Check: Run tests (windows-latest)
• GitHub Check: Run tests (macos-latest)
• GitHub Check: Run tests (ubuntu-latest)
• GitHub Check: Build and verify binaries (ubuntu-latest)
• GitHub Check: Build and verify binaries (windows-latest)

examples/graphic_usage_with_physics/src/main.cpp (3)

119-130: LGTM! Excellent visual-physics alignment.

The sphere visual radius (0.5f) perfectly matches the SphereCollider radius, and the physics properties (high restitution=0.8f, low friction=0.2f) are well-suited for a bouncy ball. Clean implementation.

---

151-152: LGTM!

Spawn positions are well-chosen to demonstrate the new collider types without immediate collisions. The ball's higher starting position (y=15) will showcase its bounce behavior effectively.

---

156-156: LGTM!

Moving the camera back from z=-2.0f to z=-10.0f is appropriate given the expanded scene with additional entities at varying positions. This provides better framing for the demonstration.

Actionable comments posted: 1

In @src/plugin/object/src/utils/ShapeGenerator.cpp:
- Around line 375-388: The addRing lambda currently sets normals as purely
horizontal (glm::normalize(glm::vec3(cosT,0,sinT))) which is wrong for
hemisphere caps; change it to compute the normal from the vertex relative to the
hemisphere center (normal = glm::normalize(vertex - glm::vec3(0.0f, centerY,
0.0f))) and ensure addRing accepts a centerY parameter (use +halfHeight for the
top hemisphere, -halfHeight for the bottom, and 0 or existing behavior for the
cylinder) so mesh.normals.emplace_back receives the correct radial hemisphere
normals.

src/plugin/object/src/helper/CreateShape.hpp (1)

121-134: Consider adding a usage example for consistency.

Other Create* functions in this file include @example documentation blocks. Adding one for CreateCapsule would maintain consistency.

📝 Suggested documentation addition

  * @param heightSegments Number of vertical segments for the cylinder (default: 4)
  * @return Engine::Entity The created entity with mesh and transform
+ *
+ * @example "Creating a capsule entity:"
+ * @code
+ * auto capsule = Object::CreateCapsule(core, 0.5f, 1.0f, glm::vec3(0, 5, 0));
+ * capsule.AddComponent<Physics::CapsuleCollider>(core, Physics::CapsuleCollider(0.5f, 0.5f));
+ * @endcode
  */

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between c9605b1 and 5de6a58.

• examples/graphic_usage_with_physics/src/main.cpp
• src/plugin/object/src/helper/CreateShape.cpp
• src/plugin/object/src/helper/CreateShape.hpp
• src/plugin/object/src/utils/ShapeGenerator.cpp
• src/plugin/object/src/utils/ShapeGenerator.hpp

src/plugin/object/src/helper/CreateShape.cpp (6)

• CreatePlane (37-49)
• CreatePlane (37-38)
• CreateSphere (23-35)
• CreateSphere (23-24)
• CreateCapsule (65-77)
• CreateCapsule (65-66)

src/plugin/object/src/helper/CreateShape.hpp (3)

• CreatePlane (96-98)
• CreateSphere (76-77)
• CreateCapsule (132-134)

src/plugin/physics/src/component/BoxCollider.hpp (4)

• BoxCollider (57-57)
• BoxCollider (63-63)
• BoxCollider (63-63)
• BoxCollider (70-70)

src/plugin/physics/src/component/SphereCollider.hpp (5)

• SphereCollider (66-66)
• SphereCollider (72-72)
• SphereCollider (72-72)
• SphereCollider (79-79)
• SphereCollider (107-107)

src/plugin/physics/src/component/CapsuleCollider.hpp (3)

• CapsuleCollider (76-76)
• CapsuleCollider (83-83)
• CapsuleCollider (91-94)

src/plugin/object/src/utils/ShapeGenerator.cpp (2)

• GenerateCapsuleMesh (359-442)
• GenerateCapsuleMesh (359-359)

src/plugin/object/src/utils/ShapeGenerator.hpp (1)

• GenerateCapsuleMesh (90-91)

src/plugin/object/src/utils/ShapeGenerator.hpp (1)

81-91: LGTM!

The new GenerateCapsuleMesh declaration follows the established pattern of other shape generators in this file. The default parameters are sensible, and the documentation clearly describes the function's purpose and parameters.

src/plugin/object/src/helper/CreateShape.cpp (1)

65-77: LGTM!

The CreateCapsule implementation follows the established pattern of other shape creation helpers (e.g., CreateSphere, CreatePlane). The entity creation, transform setup, and mesh attachment are consistent with the codebase conventions.

examples/graphic_usage_with_physics/src/main.cpp (4)

99-110: LGTM!

The floor collider setup correctly uses a BoxCollider with offset to align the collision surface with the visual plane at y=0. The comments clearly explain the intent.

---

122-133: LGTM!

The bouncy ball setup correctly matches the visual sphere radius (0.5f) with the SphereCollider radius. The physics properties (high restitution for bounciness, low friction) are appropriate for the intended behavior.

---

135-148: LGTM!

The capsule dimensions are correctly matched between visual and physics:

• Visual: height=1.5 (cylinder) + 2×radius=0.5 = 2.0 total
• Physics: 2×halfHeight=1.5 + 2×radius=0.5 = 2.0 total

The angular damping helps stabilize the character capsule, which is a good practice.

---

150-165: LGTM!

The setup properly creates the new physics entities at appropriate positions, and the camera is positioned to observe the scene.

src/plugin/object/src/utils/ShapeGenerator.cpp (1)

359-442: Algorithm structure is sound, pending the normals fix.

The mesh generation correctly constructs the capsule geometry with proper vertex positions for:

• Top hemisphere (pole to equator)
• Cylindrical middle section
• Bottom hemisphere (equator to pole)

The index generation correctly builds triangles between adjacent rings. Once the normal calculation is fixed as noted above, this implementation should produce correct results.

Actionable comments posted: 1

In @src/plugin/object/src/utils/ShapeGenerator.cpp:
- Line 369: The ringCount computation is overcounting by two due to extra +1u
terms; change the calculation of ringCount (the uint32_t variable named
ringCount) to match the actual rings generated: capRings + (heightSegments + 1u)
+ capRings so reserve() uses the correct count (this mirrors the correct
expression already used elsewhere in the file).

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 5de6a58 and dec8dd4.

• src/plugin/object/src/utils/ShapeGenerator.cpp

Learnt from: Miou-zora
Repo: EngineSquared/EngineSquared PR: 411
File: src/plugin/graphic/src/resource/Shader.hpp:89-91
Timestamp: 2026-01-01T16:46:34.114Z
Learning: The EngineSquared graphics engine uses a left-handed coordinate system with clockwise (CW) front-face winding order. All procedural meshes in ShapeGenerator.cpp are generated with CW winding, and the render pipeline is configured with frontFace = wgpu::FrontFace::CW to match.

Applied to files:

• src/plugin/object/src/utils/ShapeGenerator.cpp

src/plugin/object/src/utils/ShapeGenerator.cpp (4)

363-364: Input validation looks good.

The minimum constraints on segments and heightSegments are consistent with other shape generators in this file and prevent degenerate geometry.

---

375-388: Excellent normal calculation fix with centerY parameter.

The centerY parameter enables correct normal computation for both hemispheres (radial normals from sphere center) and cylinder (horizontal normals). Line 383 computes y - centerY for the normal's Y component, which correctly produces:

• Radial normals for hemispheres when centerY is the sphere center (halfHeight or -halfHeight)
• Horizontal normals for the cylinder when centerY == y (resulting in y - y = 0)

This addresses the improvement mentioned in the commit message.

---

391-416: Hemisphere and cylinder generation is geometrically correct.

The three sections properly form a capsule:

• Top hemisphere (lines 391-398): Generates rings from pole to equator (excluding equator), with normals pointing from (0, halfHeight, 0)
• Cylinder (lines 401-406): Connects hemispheres at equator level with horizontal normals
• Bottom hemisphere (lines 409-416): Generates rings from equator (excluding) to pole, with normals pointing from (0, -halfHeight, 0)

The exclusion of equator rings from hemispheres and inclusion in the cylinder section ensures proper connectivity without duplicate vertices.

---

419-439: Index generation follows consistent winding order.

The triangle winding pattern (i0 -> i2 -> i1, then i1 -> i2 -> i3) matches the existing sphere (lines 177-183) and cylinder (lines 292-298) generators, ensuring consistency with the engine's left-handed coordinate system and CW front-face convention.

Based on learnings, the render pipeline is configured with frontFace = wgpu::FrontFace::CW, and this implementation maintains that convention.

Actionable comments posted: 2

In @src/plugin/object/src/utils/ShapeGenerator.cpp:
- Around line 418-439: The loop building indices allows ring to reach
ringsTotal-1 which computes nextStart past the last vertex ring; update the
outer loop in ShapeGenerator.cpp (the ring loop that uses ringsTotal, ringStart,
nextStart, segments) to iterate only while ring + 1 < ringsTotal (e.g., for
(uint32_t ring = 0; ring + 1 < ringsTotal; ++ring)) or otherwise compute
nextStart safely so you never reference a non-existent ring, ensuring all
mesh.indices emplace_back calls use valid vertex indices.
- Around line 363-388: The addRing lambda can produce NaNs/divide-by-zero when
radius <= 0 or (2*radius + height) == 0; before computing normals and tex V,
validate and guard those denominators: ensure radius > 0 and denom = (2.0f *
radius + height) != 0. If invalid, either clamp radius/denom to a small epsilon,
or set a safe fallback normal (e.g., up vector) and a clamped tex V (0.5f) so
glm::normalize and the tex coordinate expression cannot divide by zero; update
references in addRing (the glm::normalize call and the texCoords.emplace_back V
calculation) accordingly.

src/plugin/object/src/utils/ShapeGenerator.cpp (1)

369-374: Minor: reserve indices + remove duplicate ring-count variables.
You compute ringCount (Line 369) and again ringsTotal (Line 419) with the same formula; also mesh.indices isn’t reserved in this function, unlike others in this file.

Possible cleanup

-    uint32_t ringCount = capRings + (heightSegments + 1u) + capRings;
+    uint32_t ringCount = capRings + (heightSegments + 1u) + capRings;
     uint32_t vertexEstimate = ringCount * (segments + 1u);
     mesh.vertices.reserve(vertexEstimate);
     mesh.normals.reserve(vertexEstimate);
     mesh.texCoords.reserve(vertexEstimate);
+    mesh.indices.reserve((ringCount - 1u) * segments * 6u);
 ...
-    uint32_t ringsTotal = capRings + (heightSegments + 1u) + capRings;
-    for (uint32_t ring = 0; ring + 1u < ringsTotal; ++ring)
+    for (uint32_t ring = 0; ring + 1u < ringCount; ++ring)
     {

Also applies to: 418-424

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between dec8dd4 and bdc8cb9.

• src/plugin/object/src/utils/ShapeGenerator.cpp