137 lines
8.4 KiB
Markdown
137 lines
8.4 KiB
Markdown
# Jerklint Review 01
|
|
|
|
Reviewed 2026-08-04. Scope: handwritten source under `engine/`, `app/`,
|
|
`server/`, `shared/`, and `scripts/`; root runtime, build, lint, editor, and
|
|
project configuration; `README.md`. Generated assets, build output,
|
|
dependencies, and lockfile were excluded.
|
|
|
|
**Jerklint Findings**
|
|
|
|
1. **High** `engine/render/Color.ts:37-43`,
|
|
`engine/render/Texture.ts:46-56`, `engine/render/Rasterizer.ts:199-203`,
|
|
`engine/render/RenderConfig.ts:104-114`, `scripts/gen-assets.ts:136-143`:
|
|
`Color.lerp` promises to interpolate a `Color` but omits alpha, so `Color.rgb`
|
|
silently supplies `255`. Every bilinear sample therefore becomes opaque.
|
|
The `clean` preset enables linear filtering, while the NPC asset has a
|
|
transparent background and alpha cutout happens after sampling. This is a
|
|
false generic abstraction with a direct behavior change hidden behind an
|
|
unrelated render knob. Smallest fix: interpolate alpha in `Color.lerp`; add
|
|
an explicitly named RGB-only blend only if fog or sky ever needs one.
|
|
|
|
2. **High** `engine/render/RenderConfig.ts:16-58`,
|
|
`engine/render/RenderConfig.ts:64-115`, `app/main.ts:19-29`,
|
|
`engine/render/Framebuffer.ts:43-73`: live render configuration has no
|
|
authoritative validity or application boundary. Public presets are mutable
|
|
singleton objects, `useConfig` retains the supplied alias, dimensions are
|
|
snapshotted into framebuffer resources, and documented numeric ranges are
|
|
not enforced. A direct width change leaves resources stale; values such as
|
|
`colorDepth = 0` make quantization divide by zero. This will become immediate
|
|
pressure when planned live sliders arrive. Smallest fix: make presets
|
|
readonly templates, create a fresh validated config for each update, and
|
|
route updates through one app-side apply function that rebuilds resources
|
|
when dimensions change.
|
|
|
|
|
|
4. **Medium** `app/level.ts:18-27`, `app/level.ts:66-105`,
|
|
`app/player.ts:2`, `app/player.ts:19-31`, `app/player.ts:61-97`,
|
|
`app/main.ts:48-52`: collision policy has no single owner. `Player.update`
|
|
accepts the render-heavy `Level`, then reconstructs gameplay facts using a
|
|
hardcoded ground height and NPC radius; NPC visual size lives in `main.ts`.
|
|
`buildLevel` also constructs geometry, collision, NPC placement, and sky in
|
|
one body. NPC or floor changes therefore require cross-file agreement and
|
|
isolated player tests need unrelated render data. Smallest fix: let `Level`
|
|
own a compact NPC descriptor and a narrow collision world containing ground,
|
|
boxes, and circles; pass only that collision value to `Player.update`.
|
|
|
|
5. **Medium** `app/main.ts:54-69`, `app/player.ts:23-55`: browser key codes are
|
|
the player simulation API. Physics owns WASD and Space bindings, receives a
|
|
mutable `Set<string>`, cannot represent a press edge, and therefore treats a
|
|
held jump key as a fresh jump whenever landing. Lost `keyup` events also
|
|
leave state stuck because no blur path clears the set. Rebinding, gamepad
|
|
input, and deterministic tests all require physics edits. Smallest fix: map
|
|
browser events in `main.ts` to a semantic `PlayerInput` value with movement
|
|
axes and `jumpPressed`; clear raw input on blur.
|
|
|
|
6. **Medium** `engine/scene/Camera.ts:27-33`,
|
|
`engine/math/Mat4.ts:23-34`, `engine/render/Rasterizer.ts:9-19`,
|
|
`engine/render/Rasterizer.ts:58-68`, `engine/render/Rasterizer.ts:77-115`:
|
|
projection limits and clipping have separate authorities. Camera projection
|
|
declares near/far values of `0.05` and `100`, but `Rasterizer.project`
|
|
discards clip-space `z` and clips only against unrelated `w >= 0.01`; far
|
|
clipping is absent. `Rasterizer.draw` looks matrix-generic while relying on
|
|
perspective-specific `w` semantics for clipping, depth, and fog. Smallest
|
|
fix: retain clip-space `z` and clip against canonical near/far planes, or
|
|
narrow and name the API so its perspective-matrix contract is explicit.
|
|
|
|
7. **Medium** `engine/render/RenderConfig.ts:48-54`,
|
|
`engine/render/Texture.ts:36-38`, `engine/render/Rasterizer.ts:51-57`,
|
|
`engine/render/Sky.ts:34-36`, `engine/render/Sky.ts:93-98`,
|
|
`engine/render/Sky.ts:121-128`: closed option unions fail open. New texture
|
|
filters silently become nearest, new lighting modes silently become unlit,
|
|
and new cloud kinds silently become basic because each dispatcher uses a
|
|
binary predicate plus fallback. `basicCumulus` accepts the full `CloudLayer`
|
|
union, preventing TypeScript from exposing the missing branch. Smallest fix:
|
|
use exhaustive switches with a `never` check and narrow cloud helpers to
|
|
their concrete variant types.
|
|
|
|
11. **Low** `engine/scene/Sprite.ts:9-23`, `app/main.ts:48-52`,
|
|
`app/main.ts:107-112`: `Sprite.texture` is assigned but never read; drawing
|
|
separately reaches back to `textures.npc`. Two sources can diverge, and the
|
|
type promises ownership the render path ignores. Smallest fix: draw with
|
|
`npc.texture`, or remove the field if material binding is intentionally
|
|
external.
|
|
|
|
12. **Low** `engine/scene/Camera.ts:4-13`, `engine/math/Mat4.ts:37-43`,
|
|
`engine/render/Sky.ts:62-66`, `app/main.ts:73-79`: camera pitch validity is
|
|
documented by `Camera` but enforced by one caller through the unexplained
|
|
literal `1.4`. Both camera and sky basis construction rely on that ritual.
|
|
Any second camera producer can create a degenerate basis. Smallest fix: own
|
|
the pitch limit and look-delta/clamp operation in the `Camera` namespace.
|
|
|
|
14. **Low** `engine/math/Vec3.ts:3-4`, `engine/math/Vec3.ts:34-37`:
|
|
`Vec3.normalize` violates its namespace-wide fresh-result guarantee only for
|
|
zero vectors by returning the mutable input alias. Callers can safely mutate
|
|
ordinary results but unexpectedly mutate source state at one edge. Smallest
|
|
fix: return a fresh zero vector.
|
|
|
|
**Scorecard**
|
|
|
|
- DRY: concern - collision, release, and resource-update policy have split owners.
|
|
- KISS: pass - core engine stays direct, data-oriented, and framework-free.
|
|
- YAGNI: concern - two unused public placeholders remain.
|
|
- SOC: concern - render config application and release artifacts mix concerns.
|
|
- Cohesion: concern - player physics consumes unrelated level rendering data.
|
|
- Coupling: fail - config/resources and projection/clipping rely on hidden cross-module rules.
|
|
- Dependency Direction: pass - browser code points inward and engine remains DOM-free.
|
|
- Law of Demeter: pass - shallow plain-data access; no traversal chains or service locators.
|
|
- Immutability: concern - mutable preset aliases and zero-vector aliasing weaken boundaries.
|
|
- Declarative Shape: concern - option unions and release phases are not exhaustively interpreted.
|
|
- Implicit Contracts: fail - alpha, dimensions, pitch, clipping, checks, and publishing depend on rituals.
|
|
- Abstraction Pressure: concern - important rules are under-centralized while stale exports remain.
|
|
- Naming/API Clarity: fail - `Color.lerp`, `check`, `publish`, and `Sprite.texture` overpromise.
|
|
- Locality: concern - NPC behavior and configuration changes require cross-file coordination.
|
|
- Testability: fail - zero tests, false-green test command, and broad player fixtures.
|
|
- File/API Shape: concern - type/namespace pattern is strong; ignored and stale exports weaken it.
|
|
- Predicate Accuracy: concern - binary fallback dispatch silently accepts future union variants.
|
|
- Construction Phase Separation: concern - level and release construction each hide multiple phases.
|
|
|
|
**Strengths**
|
|
|
|
- Engine/browser dependency boundary is clean and enforced by separate TypeScript libraries.
|
|
- Domain types generally own behavior through matching namespaces and matching filenames.
|
|
- Hot-loop mutation is local, explicit, and appropriate for a software rasterizer.
|
|
- Rasterizer, sky, texture loading, and procedural mesh generation remain cohesive despite numeric code.
|
|
- `main.ts` is still a reasonable composition root; splitting presentation or FPS code now would add ceremony.
|
|
- No ECS, service locator, class hierarchy, utility bag, dependency cycle, or speculative plugin system appeared.
|
|
|
|
**Validation**
|
|
|
|
- `bun run build`: passed.
|
|
- `bunx tsc --build --dry`: selected engine, app, and config only.
|
|
- `bun run lint`: exited with one `typescript(array-type)` warning at `scripts/gen-assets.ts:162`.
|
|
- `bun run test`: found zero tests and emitted Bun's tsconfig directory-mismatch internal error; `--pass-with-no-tests` kept the command green.
|
|
|
|
**Verdict**
|
|
|
|
Refactor soon. Keep the core architecture. Fix lying primitives and quality
|
|
commands first, then centralize configuration, input, and collision contracts.
|