[Audit][Medium] hit_ceiling boolean set on downward Y movement when passing through collision

[MichaelFisher1997]

🔍 Module Scanned\n (automated audit scan)\n\n## 📝 Summary\nIn , the flag is incorrectly set when the player's Y velocity is negative (falling/jumping down) and their downward movement is blocked by collision. This is backwards - hitting a ceiling means moving upward into an overhead block. The flag should only be set when Y velocity is positive (moving up), not when falling.\n\n## 📍 Location\n- File: \n- Function/Scope: Y-axis collision resolution (line 76-98)\n\n## 🔴 Severity: Medium\n- Medium: Performance degradation, missing error handling, suboptimal patterns\n- This is a logic bug where the flag is set incorrectly\n\n## 💥 Impact\nWhen a player falls and hits the ground, will be instead of . This causes:\n- Incorrect game logic that checks (e.g., to suppress jump or trigger animations)\n- and are mutually exclusive but both can be true at once in this edge case\n- The struct documents as "Whether the entity hit a ceiling" - this semantic is violated\n\n## 🔎 Evidence\n\n\nThe condition indicates the player is moving down (negative Y direction), yet is set in the else branch when (moving up or zero). This is backwards.\n\n\n## 🛠️ Proposed Fix\nSwap the logic so that is set when (moving up into something):\n\n\n\nNote: should always execute regardless of collision direction (it's outside the if/else in the current code, but the structure should be clarified).\n\n## ✅ Acceptance Criteria\n- [ ] is only when the player was moving upward (positive Y velocity)\n- [ ] is only when the player was moving downward (negative Y velocity)\n- [ ] A falling player who lands on ground triggers , not \n- [ ] Unit tests verify correct flags for both upward and downward collisions\n\n## 📚 References\n- - struct documentation\n- - from \n- Issue #720 (related): "CollisionConfig.max_iterations is defined but never used in binary search"\n

[github-actions]

The issue's proposed fix is incorrect. Let me clarify:

Current code at modules/engine-physics/src/collision.zig:89-93:

if (move.y < 0) {
    result.grounded = true;      // Falling, hit ground below
} else {
    result.hit_ceiling = true;   // Moving up (or stationary), hit block above
}

The audit claims this is backwards, but the logic is actually correct:

• move.y < 0 = moving DOWN (falling), so grounded = true is correct — player hit the ground
• move.y >= 0 = moving UP or zero, so hit_ceiling = true is correct — player hit something overhead

The semantic is: positive Y = up, negative Y = down (standard 3D coordinate convention used by Vec3).

The audit's proposed fix would break this — swapping the conditions would set hit_ceiling when falling and grounded when jumping, which is inverted.

Bug classification: False positive. The code correctly implements the documented semantics:

• CollisionResult.grounded: "Whether the entity is standing on solid ground" (line 31)
• CollisionResult.hit_ceiling: "Whether the entity hit a ceiling" (line 33)

The only potential improvement would be renaming move.y < 0 to move.y > 0 with a comment clarifying "moving up" for readability, but the underlying logic is sound.

opencode session  |  github run

[MichaelFisher1997]

/oc confirm this issue still exists and implement a fix. OPEN A PULL REQUEST targeting dev — do NOT push directly to dev.

[MichaelFisher1997]

I've confirmed the issue is a false positive — the described bug does not exist in the code.

Analysis

The actual code at modules/engine-physics/src/collision.zig:91-95 is:

if (move.y < 0) {
    result.grounded = true;       // falling → hit ground
} else {
    result.hit_ceiling = true;    // moving up → hit ceiling
}

The Y-axis convention here is positive = up, negative = down, confirmed by multiple sources in the codebase:

• player.zig:300 — self.velocity.y -= GRAVITY * delta_time; (gravity pulls toward negative Y)
• player.zig:309 — self.velocity.y = JUMP_VELOCITY; (jump = positive Y)
• collision_tests.zig:46,50 — "Fall straight down" uses velocity Vec3.init(0, -5, 0) (negative Y = falling)

So the current logic is correct:

• move.y < 0 (falling) → sets grounded = true ✓ (this is what the issue incorrectly claims sets hit_ceiling)
• move.y >= 0 (moving up) → sets hit_ceiling = true ✓

Verification

The existing test moveAndCollide lands on floor with default config (src/collision_tests.zig:42-61) directly contradicts the issue's premise. It drops a player with downward velocity Vec3.init(0, -5, 0) and asserts:

try testing.expect(result.grounded);      // true  ✓
try testing.expect(!result.hit_ceiling);  // false ✓

I ran zig build test -Dtest-filter="moveAndCollide" and it passes (exit 0). A falling player correctly gets grounded, not hit_ceiling.

Why no fix or PR

The issue's central claim — "the hit_ceiling flag is set when the player's Y velocity is negative (falling)" — is factually wrong about what the code does. When move.y < 0, the code sets grounded, never hit_ceiling. Applying the issue's proposed swap would introduce exactly the bug it describes (falling players would report hitting a ceiling).

I agree with the bot comment already on the issue: this is a false positive. Since the code is already correct and is covered by a passing test, I'm not making any code changes and no PR will be created.

opencode session  |  github run