chore: small fixes
This commit is contained in:
parent
47c521f9b8
commit
072dafddb3
4 changed files with 146 additions and 3 deletions
137
.agents/history/jerklint-01/review.md
Normal file
137
.agents/history/jerklint-01/review.md
Normal file
|
|
@ -0,0 +1,137 @@
|
|||
# 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.
|
||||
|
|
@ -6,6 +6,6 @@ condition: "(?i)git commit|commit message|conventional commit"
|
|||
|
||||
When commiting changes, follow these rules to write the messages:
|
||||
|
||||
- Conventional Commit (reference available at .ai/resources/conventional-commit.md)
|
||||
- `@commitlint/config-conventional` (reference available at .ai/resources/commitlint.md)
|
||||
- Conventional Commit (reference available at .agents/resources/conventional-commit.md)
|
||||
- `@commitlint/config-conventional` (reference available at .agents/resources/commitlint.md)
|
||||
|
||||
|
|
|
|||
|
|
@ -11,6 +11,6 @@
|
|||
"skipDefaultLibCheck": true,
|
||||
"skipLibCheck": true,
|
||||
"strict": true,
|
||||
"outDir": "dist"
|
||||
"outDir": "out"
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -12,6 +12,12 @@
|
|||
},
|
||||
{
|
||||
"path": "./tsconfig.app.json"
|
||||
},
|
||||
{
|
||||
"path": "./tsconfig.server.json"
|
||||
},
|
||||
{
|
||||
"path": "./tsconfig.shared.json"
|
||||
}
|
||||
]
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue