Skip to content

fix: give the diver a body instead of colliding on its centre - #129

Merged
N1k4G merged 4 commits into
mainfrom
fix/122-diver-extent
Aug 24, 2026
Merged

fix: give the diver a body instead of colliding on its centre#129
N1k4G merged 4 commits into
mainfrom
fix/122-diver-extent

Conversation

@N1k4G

@N1k4G N1k4G commented Aug 23, 2026

Copy link
Copy Markdown
Owner

Closes #122.

The defect

solidAt() is a point-in-AABB test, and both movement call sites passed the diver's centre. Movement only stopped once the centre reached a wall, so the sprite had already penetrated about a metre. Probing the wreck's bow stem (x=14..16) at d=33.8:

Diver x centre x − 0.5 m x − 1 m
16.4 open solid solid
16.7 open open solid

Closed #101"rock hitboxes feel off; collision triggers noticeably before/after the diver visually touches" — is plausibly the same defect from the player's side. A point test fires late by half the sprite on every structure in every site, regardless of how well the AABB fits the art.

The fix

solidBoxAt() widens the same AABB comparison by an extent — exact, not sampled, so a box cannot slip between probe points — and diverSolidAt() applies the diver's own.

solidAt() is left untouched. It is the predicate the parity suite replays, and fauna steering and visual zones genuinely want a point test.

The extent is a gameplay decision, so here is the evidence behind it

The hull has to be tighter than the sprite. The Pixi diver is drawn 2.1 m × 0.76 m — fin-tip to fingertip, arms out. The authored passages are tighter than that:

Site Tightest vertical opening Tightest horizontal opening
wreck 1.5 m — bulkhead doorways 2.0 m — mess/cabin door at d=23
cave 5.9 m 6.0 m

A sprite-sized hull literally cannot pass the 2.0 m door. Collision uses the torso instead:

const DIVER_HALF_WIDTH_M  = 0.45;   // 1.1 m spare in the 2.0 m door
const DIVER_HALF_HEIGHT_M = 0.30;   // 0.9 m spare in a 1.5 m doorway

These are the numbers I would most expect you to want to change. They are named constants, and the tests assert the invariants rather than the values — passability is stated against measured geometry, so a different extent does not need different tests.

The escape clause the horizontal path never had

With a point test, the diver's centre could not be inside a wall without the diver being inside it, so there was nothing to escape from. An extent makes overlap reachable — a restored save, a site switch — and without the guard the diver is walled in with no way out in either direction.

Vertical already had it (src/physics.js:132); horizontal did not. Removing it now pins the probe at x=15.5 going both ways.

Tests

tests/collision.spec.js, three properties, each failing only its own defect:

Reintroduced Fails
extent zeroed (back to a point test) body-vs-centre test
hull set to sprite size passability
horizontal escape clause removed stuck: right=15.5 left=15.5

The passability test is the guard against over-correcting — a hull wider than an opening silently walls off a route, which is a worse bug than the one being fixed. It also asserts the sweep actually found the doorways, so it cannot pass vacuously if the geometry scan ever breaks.

Blast radius

Baseline traces are unaffected. All four scenarios run simulatedGeometry: 'open' — no structures, nothing to collide with. Verified rather than assumed.

Site resources regenerated because sites.js changed and its digest moved; the payload is byte-identical (one line each, the digest).

Not in scope

Per the review decision: no flake work, no #124. Two pre-existing e2e flakes surfaced while running the suite here and are not from this branch — see #126 and the note added to it.

Closes #122.

solidAt() is a point-in-AABB test and both movement call sites passed the
diver's centre, so movement only stopped once the CENTRE reached a wall — the
sprite had already penetrated about a metre. Probing the wreck's bow stem
(x=14..16) at d=33.8, the centre read open water at x=16.7 while a point 1 m to
its left was solid.

solidBoxAt() widens the same AABB comparison by an extent — still exact rather
than sampled, so a box cannot slip between probe points — and diverSolidAt()
applies the diver's own. solidAt() is untouched: it is what the parity suite
replays, and fauna steering and visual zones genuinely want a point test.

The hull is deliberately tighter than the sprite. The Pixi diver is drawn
2.1 m x 0.76 m, but the wreck's authored passages are tighter than that: the
bulkhead doorways are 1.5 m in depth and the mess/cabin door at d=23 is 2.0 m
wide, so a sprite-sized hull could not fit through either. 0.45 m x 0.30 m
half-extents leave 1.1 m of clearance in the door and 0.9 m in a doorway.

The horizontal path also gains the escape clause it never had. With a point
test the diver's centre could not be inside a wall without the diver being
inside it, so there was nothing to escape from; with an extent an overlap is
reachable — a restored save, a site switch — and without the guard the diver
would be walled in with no way out in either direction. Verified: removing it
pins the probe at x=15.5 in both directions.

tests/collision.spec.js asserts three properties, each failing only its own
defect: the body test disagrees with the point test where it should; an
overlapping diver can swim free; and every authored passage still admits the
diver. The third is the guard against over-correcting — a hull wider than an
opening silently walls off a route, which is worse than the bug being fixed.
It states the bound against measured geometry rather than a copied number, and
checks the sweep found the doorways so it cannot pass vacuously.

Setting the hull to sprite size fails it; zeroing the extent fails the first
test; removing the escape clause fails the second.

Baseline traces are unaffected — all four scenarios run simulatedGeometry
'open', with no structures to collide with. Site resources regenerated for the
sites.js digest; the payload is byte-identical.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@N1k4G N1k4G added the bug Something isn't working label Aug 23, 2026
@github-actions github-actions Bot added release:patch Patch production release / fix or polish labels Aug 23, 2026
Its comment described it as "half-width of the diver collision shape", which
was never true — collision was a point test, so there was no shape. Now that
DIVER_HALF_WIDTH_M and DIVER_HALF_HEIGHT_M are authoritative, a second
constant claiming the same role with a different value is actively misleading.

Removed rather than renamed: it had no callers anywhere in src, tests, scripts
or the test harness — only its own declaration and an eslint globals entry.
Renaming would have preserved dead code with a fresh name.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@N1k4G

N1k4G commented Aug 23, 2026

Copy link
Copy Markdown
Owner Author

DIVER_RADIUS removed — 238fc29

Removed rather than renamed. It had no callers anywhere — src, tests, scripts, the 648K test harness — only its own declaration and an eslint globals entry. Renaming would have kept dead code under a fresh name.

Worth noting what it actually was: its comment claimed "half-width of the diver collision shape" at 0.6 m, but collision was a point test, so no such shape existed. It described an intent that was never implemented. With DIVER_HALF_WIDTH_M / DIVER_HALF_HEIGHT_M now authoritative, a second constant claiming the same role with a different value is worse than none.

Green after removal: lint, typecheck, sites:check, 133 unit, 28 parity, 3 collision. The site digest is unchanged — it hashes sites.js plus MAX_DEPTH, and neither moved.

On freezing clearances into the tests

Agreed, and that is deliberately how they are written. The passability test derives the tightest opening from the geometry at runtime and asserts the hull is strictly smaller than it:

expect(result.halfH * 2).toBeLessThan(result.tightestVertical);
expect(result.halfW * 2).toBeLessThan(result.tightestHorizontal);

No clearance figure appears as a literal. Changing the extent, or authoring a tighter passage, re-derives both sides — so the test keeps meaning something rather than needing an update to stay green. The 0.9 m / 1.1 m figures live only in a comment and the PR description, as the values that happened to hold when this landed.

The one literal is verticalCount > 100, guarding that the sweep still finds the bulkhead doorways — there so the test cannot pass vacuously if the geometry scan ever breaks.

Flake split

Done: #130 for the reload-resume timeout, cross-linked, with #126 narrowed back to the wreck-slice depth assertion and a note explaining why they are separate.

Two review findings on #129, both reproduced before changing anything.

[1] The escape clause was a noclip. "Block only when crossing from open water
into solid" meant that once the body overlapped, nothing blocked at all — any
movement was permitted, including further in and out the far side. From x=16.2,
where the centre is clear but the body clips the bow stem, kicking inward
crossed the entire stem to x=11.01. A diver resting 0.1 m inside the 39..40 m
deck sank through it to d=43.68.

Movement is now judged by buried area rather than a boolean: solidOverlapArea()
sums the diver box's intersection with every structure, and a step is refused
if it would increase that. Escaping is always permitted, going deeper never is,
and the far side is unreachable.

The comparison blocks on a strict INCREASE rather than on "not a decrease". A
diver buried deep enough to be fully engulfed sees a flat gradient — every
nearby step has identical buried area — so demanding a strict decrease pins it
in place, which is the stuck-diver bug the guard exists to prevent. Allowing
equal lets it drift to where the gradient tips. The residual is that a fully
engulfed diver can still slide along inside a slab at constant depth; being
permanently stuck is the worse failure.

[2] Every probe in the spec approached a vertical wall, so only
DIVER_HALF_WIDTH_M was exercised — DIVER_HALF_HEIGHT_M could regress to 0 and
the file still passed, because the passability assertion only requires the
height to be smaller than a doorway and zero is smaller. Adds a probe just
above the horizontal deck slab at d=39, where the centre is in open water and
the diver's underside is not.

Each defect class now fails its own tests and no others:

  halfHeight -> 0        vertical extent, travel-through
  halfWidth  -> 0        body-vs-centre, travel-through
  permissive escape      travel-through
  hull at sprite size    passability

Site resources regenerated for the sites.js digest; payload byte-identical.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@N1k4G

N1k4G commented Aug 23, 2026

Copy link
Copy Markdown
Owner Author

Both confirmed and fixed — e888207

Reproduced each first. Your numbers matched to three decimals.

[1] The escape clause was a noclip

bow stem x=14..16, start x=16.2 (centre clear, body overlapping)
  kick outward -> x = 21.390    escaping, intended
  kick inward  -> x = 11.010    crossed the entire stem

deck d=39..40 at x=50, start d=38.8 (0.1 m buried)
  free fall    -> d = 43.681    fell through

"Block only when crossing from open water into solid" meant that once the body overlapped, nothing blocked at all. I wrote that guard to stop a diver being trapped and turned it into free passage through any structure.

A boolean cannot tell "getting out" from "going further in", so movement is now judged by buried area. solidOverlapArea() sums the diver box's intersection with every structure, and a step is refused if it would increase that. Escaping is always allowed, deeper never is, and the far side is unreachable.

After: inward is pinned at 16.2, the deck holds at 38.8, outward still escapes to 21.39.

One subtlety worth flagging. The comparison blocks on a strict increase, not on "not a decrease". Demanding a strict decrease looked more correct and broke the existing escape test — a diver fully engulfed sees a flat gradient, every nearby step having identical buried area, so it can never take a first step and is pinned exactly as before. Measured at x=15.5 inside the stem: area is 0.81 at both 15.2 and 15.5, only falling at 15.8.

Allowing equal lets it drift to where the gradient tips. The residual is that a fully engulfed diver can still slide along inside a slab at constant depth. That is a real gap, and I chose it over reintroducing the permanent trap — happy to switch to resolving the overlap before physics resumes if you would rather close it properly.

[2] Vertical extent could regress to zero

Correct, and the whole file passed with DIVER_HALF_HEIGHT_M = 0. Every probe approached a vertical wall, so only the width was ever exercised, and the passability assertion just wants the height smaller than a doorway — zero qualifies.

Added a probe just above the horizontal deck slab at d=39, where the centre is in open water and the diver's underside is not.

Each defect class now fails its own tests, and no others

Reintroduced Fails
halfHeight -> 0 vertical extent, travel-through
halfWidth -> 0 body-vs-centre, travel-through
permissive escape travel-through
hull at sprite size passability

Verification

typecheck, lint, sites:check, build, 133 unit, 28 parity, 5 collision, 32 e2e all green. Resources regenerated for the sites.js digest; payload byte-identical.

One honest note on the run: a full e2e pass failed once on wreck-slice.spec.js:25, then passed on the next full run and 4/4 in isolation. That matches #126's known ~1-in-2 full-suite pattern, but I did not capture the failure signature, so I cannot say for certain it was that flake rather than something new.

Comment thread src/physics.js Outdated
// further or reach the far side.
var vHere = diverOverlapArea(diverX, depth);
var vThere = diverOverlapArea(diverX, ndp);
if (vThere > vHere) {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Allow numerically equal overlap before zeroing motion

The strict comparison has no floating-point tolerance, so a mathematically flat overlap can be classified as increasing and the fully-engulfed escape case remains trapped. On the wreck bow stem at x=15.5, d=63.7, the true overlap is a constant 0.54 m^2 while moving vertically, but adjacent samples evaluate as 0.539999999999994 and 0.5400000000000005. In the real physics loop, starting there and running 900 ticks ends at d=63.706328 with velocity 0 and the diver still solid. This is exactly the flat-gradient case the new comment says must be allowed; exact double comparison makes that allowance coordinate-dependent. Please compare with a small scale-aware epsilon in both movement axes and add a fully-engulfed vertical/tangential escape regression.

The flat-gradient allowance only works if "no change" is recognised as no
change. A fully engulfed diver has a mathematically constant buried area
whichever way it moves — that is what lets it work its way out instead of being
pinned — but constant does not mean bitwise equal. At wreck (15.5, 63.7)
adjacent depths sample as 0.539999999999994 and 0.5400000000000005, so an exact
`>` classified flat as deeper and re-trapped the diver: 900 ticks of vertical
movement travelled 0.006 m, ending at d=63.706328 with zero velocity, still
inside the hull.

Worse than a plain bug, it was coordinate-dependent — whether the guard trapped
the diver depended on where the overlap happened to land in float space, so
testing one spot with an exact comparison proved nothing about the next.

diverOverlapGrew() now owns the comparison for both movement axes, with a
tolerance scaled to the diver's own box so it stays correct if the extents
change: ~1e-9 of the box area, far above double noise at this magnitude
(~1e-16) and far below any overlap change a movement step could produce.

The regression asserts its own premise before asserting the fix — that adjacent
samples differ only by float noise — so it cannot quietly stop testing anything
if those values ever become bitwise equal. Removing only the tolerance fails it
with "pinned vertically: moved 0.006328327822146207 m in 900 ticks".

Site resources regenerated for the sites.js digest; payload byte-identical.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@N1k4G

N1k4G commented Aug 23, 2026

Copy link
Copy Markdown
Owner Author

Confirmed and fixed — 25f3977

Reproduced exactly, including the endpoint:

wreck (15.5, 63.7), fully engulfed in the bow stem (x=14..16, d=28..66)

  buried area at adjacent depths — mathematically a constant 0.54 m²:
     d=63.698   0.539999999999994
     d=63.699   0.539999999999994
     d=63.700   0.539999999999994
     d=63.701   0.5400000000000005     <- reads as "deeper"
     d=63.702   0.5400000000000005

  900 vertical ticks -> d=63.706328, v=0, still solid

An increase of 6.5e-15 was enough to re-trap the diver in exactly the case the flat-gradient allowance exists for.

The part that makes this a P1 rather than a nit is your point that it is coordinate-dependent. I tested the flat-gradient case at (15.5, 40) where the doubles happened to land equal, watched it escape, and concluded the allowance worked. It did — there. Testing one spot with an exact comparison proves nothing about the next one, which is why this needed the epsilon rather than a different probe.

diverOverlapGrew() now owns the comparison for both axes, with the tolerance scaled to the diver's own box so it stays right if the extents change — ~1e-9 of the box area, far above double noise at this magnitude (~1e-16) and far below any overlap change a movement step could produce.

After: vertical travel goes from 0.006 m to about a metre; horizontal escape is unchanged at 8.15 m.

The regression asserts its own premise

Since the whole defect is that two values are mathematically equal but not bitwise equal, the test would quietly stop proving anything if those samples ever became identical. So it checks the premise first:

const spread = Math.max(...result.areas) - Math.min(...result.areas);
expect(spread, 'adjacent samples should differ only by float noise').toBeLessThan(1e-9);

then asserts movement. Removing only the tolerance fails it with:

Error: pinned vertically: moved 0.006328327822146207 m in 900 ticks

— the same 63.706328 endpoint you reported, arrived at independently.

Verification

typecheck, lint, sites:check, build, 133 unit, 28 parity, 6 collision, 33 e2e all green. Resources regenerated for the sites.js digest; payload byte-identical.

@N1k4G
N1k4G merged commit e7bd472 into main Aug 24, 2026
6 checks passed
@N1k4G
N1k4G deleted the fix/122-diver-extent branch August 24, 2026 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working release:patch Patch production release / fix or polish

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] solidAt() tests a single point, so the diver clips ~1m into geometry Review rock (AABB boulder) hitboxes

1 participant