Repository navigation
art: SH-050 — the ground, the house and the grass stop being cartoons - #26
Merged
Merged
Conversation
The report was "the grass is too cartoonish (big), the house has no real
textures, and the riverbank should be a natural brown, not too simple".
All three were true, with one root cause: everything the player stands on
or next to was SYNTHESISED, while the trees beside it were PHOTOGRAPHED.
Round 5 gave the forest scanned bark and leaf cards; the ground, the bank
and the building never got that pass, so they shared a frame at two
fidelities and lost.
Terrain (closes ASSET-TODO T1). The four splat layers are real CC0 Poly
Haven photoscans instead of two octaves of value noise:
0 grass_lush forrest_ground_01 2.0 m (committed since it landed and
never consumed until now)
1 grass_dry withered_grass 2.0 m
2 dirt brown_mud_dry 1.3 m <- the riverbank
3 rock cliff_side 1.83 m
SH-009 promised the swap would be a file drop; the layer ORDER was already
the ABI, so it was. The procedural generators stay as the no-external/
fallback. Normals are now MEASURED, which matters more than 512 -> 1024:
the old ones were Sobel'd from albedo luminance, which assumes dark = deep.
On grass that is false, so the relief fought the light.
Each layer tiles at its scan's real physical size. The unused 4th component
of each layer tint now carries the source's extent in metres. One shared UV
scale had the 1.3 m mud stretched to 2.86 m, and a pebble 54% too big is
exactly what reads as "toy".
Building. Was noise over flat sandstone plus a mortar stripe: variation,
not material -- nothing under the noise to resolve. Now two photoscan
slices picked by face direction (already computed for the noise
projection), with measured normals and real roughness (the MR array slot is
free here; terrain's holds the splat map):
slice 0 WALL concrete_wall_008 board-formed concrete
slice 1 SLAB concrete_floor_02 floors + roof terrace
Floors and walls used to be the same beige noise, so the roof terrace read
as a wall lying down. The mortar band is now walls-only: it is a function
of world Y, and a slab has constant world Y, so the whole roof tinted
uniformly depending on its height.
Grass. Blades were 12.4 cm wide, 55 cm tall at ~7/m^2 -- a leaf, and you
could count them. Now 3.6 cm / 34 cm at ~21/m^2 (40k -> 120k). Round-9
widened the mesh to stop distant blades aliasing into speckle; that fix was
real but was charged to every blade at every distance to solve a problem
that only exists past ~9 m, where the shader's distance ramp already had it
covered. Narrow the mesh, widen the ramp (1.6 -> 3.4, from 6 m). The
anti-grit constraint is intact.
Grass colour re-tuned against a ground that is now a photograph. Measured
mean albedo: ground 0.568 0.529 0.365 (saturation 0.36, red-dominant) vs
blade tip 0.286 0.467 0.137 (saturation 0.71, green-dominant) -- twice the
saturation of the soil it grows from, biased the opposite way. Against flat
colour stops that passed; against a photograph it is a neon spike in dirt.
The old base also broke the palette rule the terrain generator states in
its own header ("green stays close to red"): r 0.30 vs g 0.42.
BUG FIXED ON THE WAY: SH-010's detail normal has been a flat no-op since it
shipped. fbm read `vnoise(x * f / SIZE * SIZE, ...)`, which parses as
((x*f)/SIZE)*SIZE -- exactly x*f. With integer pixel coords in, every sample
landed on a lattice point, and the hash wraps at period f where (x*f) % f is
0 for every x. Every texel of every octave returned hash(0,0); the output was
a uniform (127,127,255), the precise encoding of "flat". A no-op normal map
does not look broken, it looks like nothing. Detail strength 0.35 -> 0.22,
since the old value was tuned against a texture that did nothing.
Measured, not assumed:
grass 40k -> 120k 26.64 -> 26.21 fps (0.4 fps for 3x the blades; and
the old "past ~40k the scatter saturates" note was
never measured and is wrong -- it places 120000/120000,
and there is a boot log for it now)
textures 512 -> 1024 26.69 -> 26.50 fps (same binary, only the PNGs swapped)
whole change 27 -> 26 fps (title screen)
Traps recorded in assets/textures/external/SOURCES.md:
1. Poly Haven names lie about colour. brown_mud and brown_mud_02 are GREY;
the brief said "natural brown".
2. Judge a texture at the tiling it will really get, not at 2x2. The walls
first shipped plaster_stone_wall_01 -- fine at 2x2, a stamped GRID of
identical stones across a 24x8 m facade (10x4 tiles), worse than the flat
colour it replaced. Sparse distinctive features cannot survive repetition;
board-formed concrete can -- its seams and tie-holes are meant to repeat.
3. `macro` is a reserved word in WGSL (recorded in round 4, re-learned here).
Not changed: the trees. They already have real scanned textures from round 5
(Poly Haven pine_bark + ambientCG LeafSet004).
New tools: bun tools/fetch-external-textures.ts (idempotent, --force to
re-download) and bun tools/build-building-textures.ts.
📝 WalkthroughWalkthroughThe update adds external texture fetching and build tools, integrates scanned terrain and building materials with physical tiling, retunes grass rendering and density, and adds optional FPS diagnostics and documentation for the SH-050 asset pipeline. ChangesScanned texture pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TextureFetcher
participant TextureBuilders
participant Environment
participant TerrainShader
participant BuildingShader
TextureFetcher->>TextureBuilders: provide validated external maps
TextureBuilders->>Environment: provide generated texture assets
Environment->>TerrainShader: bind terrain parameters and layers
Environment->>BuildingShader: bind albedo, normal, and roughness arrays
TerrainShader->>TerrainShader: scale UVs by per-layer scan size
BuildingShader->>BuildingShader: select wall or slab slice by face direction
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@assets/materials/building.wgsl`:
- Around line 167-181: Update the normal construction in the sampled-normal
block to preserve t.z by scaling the geometric normal n by the sampled Z
component before adding the tangent-plane components. Keep the existing slab and
face-orientation swizzles in nw, and retain bp.misc.x as the relief strength
applied to the perturbation.
In `@assets/materials/grass_instanced.wgsl`:
- Around line 83-84: Update the wide multiplier calculation near fade in the
grass shader to interpolate from 1.0 to a maximum of exactly 3.4 when fade
reaches 1.0, using mix or an equivalent 2.4 ramp; preserve the existing
smoothstep fade behavior.
In `@tools/build-terrain-textures.ts`:
- Around line 212-239: The texture builder loop around LAYERS must treat scanned
maps atomically: use scan data only when both diff and nor are available;
otherwise generate both albedo and normal procedurally, rather than mixing
scanned and fallback maps. In src/environment.ts lines 437-459, update the
building binding logic to populate building arrays only when albedo, normal, and
roughness handles are all valid.
In `@tools/fetch-external-textures.ts`:
- Around line 96-99: Update the existing-file early return in the fetch flow to
validate the destination with the existing JPEG signature and size validation
before skipping it. Only return true when the file passes validation; otherwise
continue through the normal download/replacement path, while preserving FORCE
behavior.
- Around line 93-117: Update fetchOne to catch rejections from fetch and
response.arrayBuffer, returning the required structured failure result with ok
false and a descriptive error string instead of allowing exceptions to terminate
processing. Adjust its success and existing failure paths to consistently return
the { ok, error } shape, and update callers to consume that result while
preserving continued processing of remaining assets.
In `@tools/imgutil.ts`:
- Around line 63-70: Update loadRgbaExact to invoke ffmpeg through execFileSync
or spawnSync with separate argument values, avoiding shell interpolation of src
and tmp. Ensure temporary-file cleanup runs in a finally block, and change the
function’s result contract to return { ok, error } rather than throwing on
command or decoding failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 44dbad84-6c2a-4d1a-8372-5cda3c13c093
⛔ Files ignored due to path filters (27)
assets/textures/building_slab_albedo.pngis excluded by!**/*.pngassets/textures/building_slab_normal.pngis excluded by!**/*.pngassets/textures/building_slab_rough.pngis excluded by!**/*.pngassets/textures/building_wall_albedo.pngis excluded by!**/*.pngassets/textures/building_wall_normal.pngis excluded by!**/*.pngassets/textures/building_wall_rough.pngis excluded by!**/*.pngassets/textures/external/brown_mud_dry/brown_mud_dry_diff_2k.jpgis excluded by!**/*.jpgassets/textures/external/brown_mud_dry/brown_mud_dry_nor_gl_2k.jpgis excluded by!**/*.jpgassets/textures/external/cliff_side/cliff_side_diff_2k.jpgis excluded by!**/*.jpgassets/textures/external/cliff_side/cliff_side_nor_gl_2k.jpgis excluded by!**/*.jpgassets/textures/external/concrete_floor_02/concrete_floor_02_diff_2k.jpgis excluded by!**/*.jpgassets/textures/external/concrete_floor_02/concrete_floor_02_nor_gl_2k.jpgis excluded by!**/*.jpgassets/textures/external/concrete_floor_02/concrete_floor_02_rough_2k.jpgis excluded by!**/*.jpgassets/textures/external/concrete_wall_008/concrete_wall_008_diff_2k.jpgis excluded by!**/*.jpgassets/textures/external/concrete_wall_008/concrete_wall_008_nor_gl_2k.jpgis excluded by!**/*.jpgassets/textures/external/concrete_wall_008/concrete_wall_008_rough_2k.jpgis excluded by!**/*.jpgassets/textures/external/withered_grass/withered_grass_diff_2k.jpgis excluded by!**/*.jpgassets/textures/external/withered_grass/withered_grass_nor_gl_2k.jpgis excluded by!**/*.jpgassets/textures/terrain_detail_normal.pngis excluded by!**/*.pngassets/textures/terrain_dirt_albedo.pngis excluded by!**/*.pngassets/textures/terrain_dirt_normal.pngis excluded by!**/*.pngassets/textures/terrain_grass_dry_albedo.pngis excluded by!**/*.pngassets/textures/terrain_grass_dry_normal.pngis excluded by!**/*.pngassets/textures/terrain_grass_lush_albedo.pngis excluded by!**/*.pngassets/textures/terrain_grass_lush_normal.pngis excluded by!**/*.pngassets/textures/terrain_rock_albedo.pngis excluded by!**/*.pngassets/textures/terrain_rock_normal.pngis excluded by!**/*.png
📒 Files selected for processing (13)
CLAUDE.mdassets/materials/building.wgslassets/materials/grass_instanced.wgslassets/materials/terrain.wgslassets/textures/external/SOURCES.mddocs/ASSET-TODO.mddocs/tickets.mdsrc/environment.tssrc/main.tstools/build-building-textures.tstools/build-terrain-textures.tstools/fetch-external-textures.tstools/imgutil.ts
Comment on lines
+167
to
+181
| // ---- normal ------------------------------------------------------------- | ||
| // Tangent -> world by the same swizzle the face plane chose, matching | ||
| // terrain.wgsl's whiteout convention. This is what makes the stones in the | ||
| // plaster catch the sun as relief instead of reading as a photo pasted on a | ||
| // flat box. | ||
| let t = textureSample(normal_array, albedo_array_samp, uv, slice).rgb * 2.0 - 1.0; | ||
| var nw: vec3<f32>; | ||
| if (is_slab) { | ||
| nw = vec3<f32>(t.x, 0.0, t.y); | ||
| } else if (absn.x > absn.z) { | ||
| nw = vec3<f32>(0.0, t.y, t.x); | ||
| } else { | ||
| nw = vec3<f32>(t.x, t.y, 0.0); | ||
| } | ||
| let nrm = normalize(n + nw * bp.misc.x); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the sampled normal’s Z component.
normalize(n + nw * strength) discards t.z, so strength = 1.0 cannot represent the measured normal as authored and systematically flattens relief. Scale the geometric normal by t.z.
Proposed fix
- let nrm = normalize(n + nw * bp.misc.x);
+ let nrm = normalize(n * max(t.z, 1e-4) + nw * bp.misc.x);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // ---- normal ------------------------------------------------------------- | |
| // Tangent -> world by the same swizzle the face plane chose, matching | |
| // terrain.wgsl's whiteout convention. This is what makes the stones in the | |
| // plaster catch the sun as relief instead of reading as a photo pasted on a | |
| // flat box. | |
| let t = textureSample(normal_array, albedo_array_samp, uv, slice).rgb * 2.0 - 1.0; | |
| var nw: vec3<f32>; | |
| if (is_slab) { | |
| nw = vec3<f32>(t.x, 0.0, t.y); | |
| } else if (absn.x > absn.z) { | |
| nw = vec3<f32>(0.0, t.y, t.x); | |
| } else { | |
| nw = vec3<f32>(t.x, t.y, 0.0); | |
| } | |
| let nrm = normalize(n + nw * bp.misc.x); | |
| // ---- normal ------------------------------------------------------------- | |
| // Tangent -> world by the same swizzle the face plane chose, matching | |
| // terrain.wgsl's whiteout convention. This is what makes the stones in the | |
| // plaster catch the sun as relief instead of reading as a photo pasted on a | |
| // flat box. | |
| let t = textureSample(normal_array, albedo_array_samp, uv, slice).rgb * 2.0 - 1.0; | |
| var nw: vec3<f32>; | |
| if (is_slab) { | |
| nw = vec3<f32>(t.x, 0.0, t.y); | |
| } else if (absn.x > absn.z) { | |
| nw = vec3<f32>(0.0, t.y, t.x); | |
| } else { | |
| nw = vec3<f32>(t.x, t.y, 0.0); | |
| } | |
| let nrm = normalize(n * max(t.z, 1e-4) + nw * bp.misc.x); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@assets/materials/building.wgsl` around lines 167 - 181, Update the normal
construction in the sampled-normal block to preserve t.z by scaling the
geometric normal n by the sampled Z component before adding the tangent-plane
components. Keep the existing slab and face-orientation swizzles in nw, and
retain bp.misc.x as the relief strength applied to the perturbation.
Comment on lines
+83
to
+84
| let fade = smoothstep(6.0, 42.0, d_cam); | ||
| let wide = 1.0 + fade * 3.4; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the widening multiplier math.
The comments indicate that the base blade was narrowed by 3.4x, and the far-field ramp should scale it up by exactly 3.4x to compensate. However, 1.0 + fade * 3.4 results in a maximum scale of 4.4x when fade reaches 1.0. To achieve a maximum multiplier of exactly 3.4x, you should use mix(1.0, 3.4, fade) or 1.0 + fade * 2.4.
🐛 Proposed fix for the multiplier logic
- let fade = smoothstep(6.0, 42.0, d_cam);
- let wide = 1.0 + fade * 3.4;
+ let fade = smoothstep(6.0, 42.0, d_cam);
+ let wide = mix(1.0, 3.4, fade);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let fade = smoothstep(6.0, 42.0, d_cam); | |
| let wide = 1.0 + fade * 3.4; | |
| let fade = smoothstep(6.0, 42.0, d_cam); | |
| let wide = mix(1.0, 3.4, fade); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@assets/materials/grass_instanced.wgsl` around lines 83 - 84, Update the wide
multiplier calculation near fade in the grass shader to interpolate from 1.0 to
a maximum of exactly 3.4 when fade reaches 1.0, using mix or an equivalent 2.4
ramp; preserve the existing smoothstep fade behavior.
Comment on lines
+212
to
+239
| for (const l of LAYERS) { | ||
| const diff = l.src ? srcMap(l.src, 'diff') : null; | ||
| const nor = l.src ? srcMap(l.src, 'nor_gl') : null; | ||
|
|
||
| // Albedo — photoscan if present, else the procedural stand-in. | ||
| let albedo: Uint8Array; | ||
| if (diff) { | ||
| albedo = loadRgbaExact(diff, SIZE).rgba; | ||
| } else { | ||
| albedo = makeTexture(SIZE, SIZE, l.fn); | ||
| } | ||
| writeFileSync(`assets/textures/${l.name}_albedo.png`, encodePng(SIZE, SIZE, albedo)); | ||
| // Normals are derived from the albedo's luminance as a height field. Real | ||
| // scans ship a measured normal; this is the honest approximation, and it is | ||
| // what gives the surface relief under a raking sun. | ||
| const normal = heightToNormal(SIZE, SIZE, albedo, l.bump); | ||
|
|
||
| // Normal — the scan's MEASURED nor_gl if present. Poly Haven's _nor_gl is | ||
| // OpenGL-convention tangent space (+Y up in green), which is already what the | ||
| // engine decodes (`v * 2/255 - 1`) and what heightToNormal emitted, so it | ||
| // needs no channel flip. Without a scan, fall back to Sobel-from-luminance. | ||
| let normal: Uint8Array; | ||
| if (nor) { | ||
| normal = loadRgbaExact(nor, SIZE).rgba; | ||
| } else { | ||
| normal = heightToNormal(SIZE, SIZE, albedo, l.bump); | ||
| } | ||
| writeFileSync(`assets/textures/${l.name}_normal.png`, encodePng(SIZE, SIZE, normal)); | ||
| console.log(`wrote ${l.name}_albedo.png + _normal.png (${SIZE}x${SIZE})`); | ||
|
|
||
| const how = diff ? `scan ${l.src} (${l.sizeM} m)` : 'PROCEDURAL fallback'; | ||
| const nhow = nor ? 'measured normal' : 'normal from luminance'; | ||
| console.log(`wrote ${l.name}_albedo/_normal.png ${SIZE}² — ${how}, ${nhow}`); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Treat each scanned material as one atomic resource set.
Partial map availability currently produces mismatched terrain maps or invalid building bindings.
tools/build-terrain-textures.ts#L212-L239: use the scan only when both albedo and normal maps exist; otherwise generate both fallbacks.src/environment.ts#L437-L459: bind building arrays only when albedo, normal, and roughness handles are all valid.
📍 Affects 2 files
tools/build-terrain-textures.ts#L212-L239(this comment)src/environment.ts#L437-L459
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/build-terrain-textures.ts` around lines 212 - 239, The texture builder
loop around LAYERS must treat scanned maps atomically: use scan data only when
both diff and nor are available; otherwise generate both albedo and normal
procedurally, rather than mixing scanned and fallback maps. In
src/environment.ts lines 437-459, update the building binding logic to populate
building arrays only when albedo, normal, and roughness handles are all valid.
Comment on lines
+93
to
+117
| async function fetchOne(s: Set, map: string): Promise<boolean> { | ||
| const dir = `${ROOT}/${s.id}`; | ||
| const dst = `${dir}/${s.id}_${map}_${s.res}.jpg`; | ||
| if (!FORCE && existsSync(dst) && statSync(dst).size > 0) { | ||
| console.log(` skip ${dst} (exists)`); | ||
| return true; | ||
| } | ||
| const u = url(s.id, map, s.res); | ||
| const r = await fetch(u); | ||
| if (!r.ok) { | ||
| console.error(` FAIL ${u} -> HTTP ${r.status}`); | ||
| return false; | ||
| } | ||
| const buf = new Uint8Array(await r.arrayBuffer()); | ||
| // A CDN error page is a 200 with HTML in it. A JPEG starts FF D8 FF; check | ||
| // rather than write a 4 KB "not found" page named like a texture and let | ||
| // build-terrain-textures fail later with something unrelated. | ||
| if (buf.length < 1024 || buf[0] !== 0xff || buf[1] !== 0xd8 || buf[2] !== 0xff) { | ||
| console.error(` FAIL ${u} -> not a JPEG (${buf.length} bytes)`); | ||
| return false; | ||
| } | ||
| mkdirSync(dir, { recursive: true }); | ||
| writeFileSync(dst, buf); | ||
| console.log(` got ${dst} (${(buf.length / 1024).toFixed(0)} KB)`); | ||
| return true; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return structured failures for rejected downloads.
fetch() and arrayBuffer() can reject, bypassing ok = false and terminating the tool before remaining assets are processed. Catch these failures and return the required { ok, error } result.
As per coding guidelines, “In Perry TypeScript code, never throw errors; return { ok: boolean, error: string } instead.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/fetch-external-textures.ts` around lines 93 - 117, Update fetchOne to
catch rejections from fetch and response.arrayBuffer, returning the required
structured failure result with ok false and a descriptive error string instead
of allowing exceptions to terminate processing. Adjust its success and existing
failure paths to consistently return the { ok, error } shape, and update callers
to consume that result while preserving continued processing of remaining
assets.
Source: Coding guidelines
Comment on lines
+96
to
+99
| if (!FORCE && existsSync(dst) && statSync(dst).size > 0) { | ||
| console.log(` skip ${dst} (exists)`); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate existing files before skipping them.
Any nonempty corrupt or partial file is accepted forever unless --force is supplied. Reuse the JPEG signature/size validation before returning true.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/fetch-external-textures.ts` around lines 96 - 99, Update the
existing-file early return in the fetch flow to validate the destination with
the existing JPEG signature and size validation before skipping it. Only return
true when the file passes validation; otherwise continue through the normal
download/replacement path, while preserving FORCE behavior.
Comment on lines
+63
to
+70
| export function loadRgbaExact(src: string, size: number): { width: number; height: number; rgba: Uint8Array } { | ||
| requireFfmpeg(); | ||
| if (!existsSync(src)) throw new Error(`loadRgbaExact: no such file: ${src}`); | ||
| const tmp = join(tmpdir(), `bloom_tex_${basename(src)}_${size}.png`); | ||
| execSync(`ffmpeg -y -loglevel error -i "${src}" -vf "scale=${size}:${size}:flags=lanczos" -pix_fmt rgba "${tmp}"`, | ||
| { stdio: 'inherit' }); | ||
| const png = decodePng(readFileSync(tmp)); | ||
| try { unlinkSync(tmp); } catch { /* a leftover temp file is not worth failing a build over */ } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file first
ast-grep outline tools/imgutil.ts --view expanded || true
printf '\n--- tools/imgutil.ts (relevant lines) ---\n'
cat -n tools/imgutil.ts | sed -n '1,220p'
printf '\n--- search for loadRgbaExact usages ---\n'
rg -n "loadRgbaExact\(" -S .
printf '\n--- search for execSync in tools/imgutil.ts ---\n'
rg -n "execSync|execFileSync|spawnSync|throw new Error|error:" tools/imgutil.tsRepository: Bloom-Engine/shooter
Length of output: 5511
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- relevant callsites ---\n'
sed -n '180,260p' tools/build-terrain-textures.ts
printf '\n---\n'
sed -n '60,130p' tools/build-building-textures.ts
printf '\n--- search for path sources used in these tools ---\n'
rg -n "readFileSync|readdirSync|glob|argv|process\.argv|join\(|basename\(|loadRgbaExact\(" tools/build-terr* tools/build-building* tools/*.tsRepository: Bloom-Engine/shooter
Length of output: 13700
Avoid shell interpolation in loadRgbaExact (tools/imgutil.ts:63-70)
src is passed through execSync(...), so a filename containing quotes or shell metacharacters can escape the command. Switch to execFileSync/spawnSync with args, clean up tmp in finally, and return { ok, error } instead of throwing.
🧰 Tools
🪛 OpenGrep (1.25.0)
[ERROR] 67-68: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/imgutil.ts` around lines 63 - 70, Update loadRgbaExact to invoke ffmpeg
through execFileSync or spawnSync with separate argument values, avoiding shell
interpolation of src and tmp. Ensure temporary-file cleanup runs in a finally
block, and change the function’s result contract to return { ok, error } rather
than throwing on command or decoding failures.
Sources: Coding guidelines, Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reported: "the grass and the house in the map don't look very realistic yet. the grass is too cartoonish (big), the house has no real textures... also the river-side (leading into the water) should also be a natural brown, not too simple/cartoonish"
All three were true, and they had one root cause: everything the player stands on or next to was synthesised, while the trees beside it were photographed. Round 5 gave the forest real scanned bark and leaf cards; the ground, the riverbank and the building never got that pass, so they sat in the same frame at two fidelities and lost.
Before / after
Same camera, same deterministic title screen. Note the diag bar: FPS 27 → 26.
What shipped
Terrain — closes ASSET-TODO T1. Four real CC0 Poly Haven photoscans replacing two octaves of value noise:
forrest_ground_01withered_grassbrown_mud_drycliff_sideSH-009 promised the swap would be a file drop and it genuinely was — the layer order was already the ABI. The procedural generators stay as the fallback for a checkout without
assets/textures/external/.forrest_ground_01had been committed and recorded as "not consumed yet" since the day it landed; this consumes it.Normals are now measured, which matters more than the 512→1024 bump: the old ones were Sobel'd from albedo luminance, which assumes dark = deep. On grass that's false, so the relief was fighting the light.
Each layer tiles at its scan's real physical size — the unused 4th component of each layer tint now carries the source's extent in metres. One shared UV scale had the 1.3 m mud stretched to 2.86 m, and a pebble 54% too big is exactly what reads as "toy".
Building. Two photoscan slices picked by face direction (already computed for the noise projection), with measured normals and real roughness — the MR array slot is free here, unlike terrain's, which holds the splat map.
concrete_wall_008(board-formed concrete)concrete_floor_02(floors + roof terrace)Floors and walls used to be the same beige noise, so the roof terrace read as a wall lying down. The mortar band is now walls-only: it's a function of world Y, and a slab has constant world Y, so the whole roof tinted uniformly depending on its height.
Grass. Round-9 widened the mesh to stop distant blades aliasing into speckle. That fix was real — but it was charged to every blade at every distance to solve a problem that only exists past ~9 m, where the shader's distance ramp already had it covered. So: narrow the mesh, widen the ramp (1.6→3.4, starting at 6 m). The anti-grit constraint is intact.
Grass colour, re-tuned against a ground that is now a photograph. Measured mean albedo:
Twice the saturation of the soil it grows from, biased the opposite way down the spectrum. Against flat colour stops that passed; against a photograph it's a neon spike in dirt — the most cartoonish thing left in frame the moment the terrain got real. The old base also broke the palette rule the terrain generator states in its own header ("green stays close to red"): r 0.30 vs g 0.42.
Bug found and fixed on the way
SH-010's detail normal has been a flat no-op since it shipped.
fbmreadvnoise(x * f / SIZE * SIZE, ...), which parses as((x*f)/SIZE)*SIZE— exactlyx*f. With integer pixel coords in, every sample landed on a lattice point, and the hash wraps at periodfwhere(x*f) % fis 0 for every x. Every texel of every octave returnedhash(0,0); the output was a uniform(127,127,255)— the precise encoding of "flat".A no-op normal map doesn't look broken, it looks like nothing, which is why the "last order of magnitude of ground detail" pass has been doing nothing for its whole life. Verify: distinct colour count went 1 → 2280, still tiling seamlessly. Detail strength 0.35 → 0.22, since the old value was tuned against a texture that did nothing.
Measured, not assumed
The old note's "past ~40k the scatter saturates against its keep-outs" was never measured and is wrong — it places 120000/120000. There's a boot log for it now.
Traps recorded (
assets/textures/external/SOURCES.md)brown_mudandbrown_mud_02are grey. The brief said "natural brown" — picking by name ships the opposite. Every source here was chosen by downloading its diffuse and looking at it.plaster_stone_wall_01— lovely at 2×2, and a stamped grid of identical stones across a 24×8 m facade (10×4 tiles), worse than the flat colour it replaced. Sparse distinctive features can't survive repetition; board-formed concrete can — its seams and tie-holes are supposed to repeat at ~2 m.macrois a reserved word in WGSL (recorded in round 4, re-learned here).Not changed
The trees. They already have real scanned textures from round 5 (Poly Haven
pine_bark+ ambientCGLeafSet004). Nothing to do.Notes for review
external/is committed andSOURCES.mdrecords provenance. All CC0, GPLv3-compatible. Terrain sets deliberately fetch no roughness map: all three of terrain's array slots are spoken for, so nothing could ever sample it.bun tools/fetch-external-textures.ts(idempotent,--forceto re-download) andbun tools/build-building-textures.ts.INDOORCAMwas borrowed for facade captures and restored byte-identical.Summary by CodeRabbit