[Audit][Medium] AABB.contains uses inclusive boundary that may cause off-by-one errors in voxel collision

[MichaelFisher1997]

🔍 Module Scanned

libs/zig-math/ (engine-math module re-exports)

📝 Summary

The AABB.contains method uses inclusive <= comparisons for all axes, treating points exactly at the max boundary as contained within the AABB. When AABBs represent voxel/block regions using the common pattern AABB.init(Vec3.init(x, y, z), Vec3.init(x+1, y+1, z+1)), this creates a half-open interval [min, max) in mathematical terms, but the implementation treats it as fully closed [min, max]. This can cause subtle off-by-one errors in collision detection, especially when checking if a player or entity is "on the ground" versus one block too high.

📍 Location

• File: libs/zig-math/aabb.zig:27-31
• Function/Scope: AABB.contains

🔴 Severity: Medium

• Critical: Crashes, data corruption, security vulnerabilities, GPU device loss
• High: Memory leaks, race conditions, incorrect rendering, broken features
• Medium: Performance degradation, missing error handling, suboptimal patterns
• Low: Code style, dead code, minor improvements

💥 Impact

When checking if a point is within a voxel AABB, a point at the max boundary (e.g., a player's feet at y=1.0 when standing on a block at y=0..1) is incorrectly reported as inside the AABB. This can cause:

• Incorrect collision responses at block boundaries
• Entities appearing to collide with blocks they're standing adjacent to rather than on top of
• Subtle physics glitches where players can't climb single-block steps they're standing on

The same issue affects intersects which relies on comparisons that assume inclusive boundaries.

🔎 Evidence

pub fn contains(self: AABB, point: Vec3) bool {
    return point.x >= self.min.x and point.x <= self.max.x and
        point.y >= self.min.y and point.y <= self.max.y and
        point.z >= self.z and point.z <= self.max.z;
}

The use of <= instead of < means the point at max is contained. In a typical voxel AABB like AABB.init(Vec3.init(0,0,0), Vec3.init(1,1,1)) representing a block at origin, a point at (1, 1, 1) would be reported as contained even though it's at the exclusive upper boundary of the block volume.

🛠️ Proposed Fix

For collision-focused AABBs representing voxel volumes, the contains method should use exclusive upper bounds (< instead of <=) to properly implement the half-open interval semantics expected in voxel engines:

pub fn contains(self: AABB, point: Vec3) bool {
    return point.x >= self.min.x and point.x < self.max.x and
        point.y >= self.min.y and point.y < self.max.y and
        point.z >= self.min.z and point.z < self.max.z;
}

Note: This change should be verified against all callers to ensure the new semantics are correct for their use case. Some callers (like UI rect containment) may intentionally want inclusive boundaries.

An alternative is to add a containsExclusive variant and deprecate the current contains behavior, or add a parameter to control boundary behavior.

✅ Acceptance Criteria

• All existing unit tests pass after any contains semantics change
• Collision detection correctly handles entities at block top surfaces
• isOnGround function in collision.zig correctly detects ground at block boundaries
• New or existing tests verify correct behavior at AABB boundaries (min corner, max corner, just outside)

📚 References

• ZigCraft voxel coordinate conventions define local coordinates as u32 (0-15 for X/Z within chunk)
• Standard half-open interval convention for voxel AABBs in Minecraft-style engines

[github-actions]

Analysis Comment:

This is a valid Bug report regarding AABB boundary semantics.

Validation:

• Issue confirmed: AABB.contains at libs/zig-math/aabb.zig:27-31 uses <= for all axes, treating max boundary as inclusive
• Test conflict: The existing test at src/math_tests.zig:241 explicitly expects aabb.contains(Vec3.init(10, 10, 10)) to be true for an AABB with max at Vec3.init(10, 10, 10). Any fix would break this test.
• Collision system impact is limited: The collision system (modules/engine-physics/src/collision.zig) uses intersects (not contains) for world collision detection, and intersects at line 33-36 uses the correct closed-interval intersection logic (self.min.x <= other.max.x and self.max.x >= other.min.x)

Scope Assessment:
The issue is appropriately scoped. The fix is localized to AABB.contains in libs/zig-math/aabb.zig:27-31, though it requires:

1. Updating the unit test at src/math_tests.zig:241 to expect false for the max boundary point
2. Verifying UI rectangle containment (multiple rect.contains calls in modules/game-ui/) doesn't regress - these may intentionally want inclusive boundaries

Suggested implementation approach:

1. Change contains to use exclusive upper bounds (< instead of <=)
2. Update src/math_tests.zig:241 to test !aabb.contains(Vec3.init(10, 10, 10))
3. Audit UI uses of rect.contains - they may need inclusive behavior via a separate method or parameter

opencode session  |  github run