Ticket the 2026-07-16 audit findings (EN-053..EN-060)#106
Conversation
The full shooter+engine audit found the remaining perf cost concentrated in unglamorous, previously UNTICKETED places. EN-053 shadow-mask pass; EN-054 cache build_world_triangles; EN-055 animation instancing; EN-056 per-frame upload/alloc tail; EN-057 Hi-Z with zero consumers; EN-058 DXC silent fallback; EN-059 UTF-8-as-Latin-1 text decode; EN-060 quality-gated grass LOD. EN-053/060 carry the product owner's hard constraint verbatim: no perf win may cost visual quality.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesEngine audit tickets
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 3
🤖 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 `@docs/tickets.md`:
- Around line 1777-1779: Update the shadow-mask proposal in the ticket to scope
it to the main screen-space/deferred lighting consumer, or explicitly define
separate masks or cached shadow data for the independently rendered GI
card-light and planar-probe consumers; do not imply that one render-resolution
screen-space mask serves all consumers.
- Around line 1883-1887: Update the quality gate described under “Hard
constraint from the product owner: no visible quality loss” to require temporal
validation in addition to A/B screenshots. Require camera-motion or multi-frame
captures at relevant distances and, where applicable, across temporal-upscaling
configurations, verifying no shimmer, popping, or unstable thinning before
accepting dithered per-instance dropout.
- Around line 1864-1867: Update the mojibake diagnosis in the documented text to
identify CP1252 or another non-UTF-8 single-byte decoding, not Latin-1, and
ensure the example uses the correct CP1252 rendering. Keep the explanation
encoding-neutral if preferred while preserving the
readFile/loadWorld-to-drawText context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| Fix shape: a screen-space **shadow-mask pass** at render resolution — compute | ||
| the sun-shadow term once, sample the mask from every consumer. Optionally | ||
| distance-adaptive tap counts. **Hard constraint: quality-neutral.** The |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Scope the shadow-mask proposal to compatible consumers.
A render-resolution screen-space mask can serve the main deferred lighting view, but it cannot directly provide correct shadow terms for the independently rendered GI card-light and planar-probe consumers named above. Scope the ticket to screen-space lighting, or specify separate masks/cache data in each consumer’s coordinate space.
🤖 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 `@docs/tickets.md` around lines 1777 - 1779, Update the shadow-mask proposal in
the ticket to scope it to the main screen-space/deferred lighting consumer, or
explicitly define separate masks or cached shadow data for the independently
rendered GI card-light and planar-probe consumers; do not imply that one
render-resolution screen-space mask serves all consumers.
| `Arena 02 a-hat-euro-quote Outdoor plaza` — the three-char cp1252 image of | ||
| the UTF-8 bytes. So somewhere between `readFile`/`loadWorld` and `drawText`, | ||
| file bytes are decoded per-byte (Latin-1) instead of as UTF-8. Every | ||
| non-ASCII character in any text asset (world names, future localized strings |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use CP1252, not Latin-1, for this mojibake diagnosis. The — output matches UTF-8 bytes misread as CP1252; Latin-1 would render â��. Keep the wording encoding-neutral, or say “likely CP1252/non-UTF-8 single-byte decoding.”
🤖 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 `@docs/tickets.md` around lines 1864 - 1867, Update the mojibake diagnosis in
the documented text to identify CP1252 or another non-UTF-8 single-byte
decoding, not Latin-1, and ensure the example uses the correct CP1252 rendering.
Keep the explanation encoding-neutral if preferred while preserving the
readFile/loadWorld-to-drawText context.
| **Hard constraint from the product owner: no visible quality loss.** A naive | ||
| density cut reads as the far field thinning and is REJECTED by definition. | ||
| Acceptable shapes: dithered per-instance dropout beyond the distance where a | ||
| blade is sub-pixel (its coverage already handled by the shader's distance | ||
| ramp), with A/B screenshots at multiple distances proving indistinguishable. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add temporal validation to the quality gate.
A/B screenshots can miss shimmer, popping, or unstable thinning caused by dithered per-instance dropout. Require camera-motion or multi-frame captures, ideally across the relevant temporal-upscaling configurations, before accepting the optimization.
🤖 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 `@docs/tickets.md` around lines 1883 - 1887, Update the quality gate described
under “Hard constraint from the product owner: no visible quality loss” to
require temporal validation in addition to A/B screenshots. Require
camera-motion or multi-frame captures at relevant distances and, where
applicable, across temporal-upscaling configurations, verifying no shimmer,
popping, or unstable thinning before accepting dithered per-instance dropout.
… crash, not a silent fallback
1 participant
The 2026-07-16 full audit (shooter + engine) found the remaining perf/quality cost concentrated in places that had no tickets — and this registry's own history says unticketed work doesn't happen. Eight new entries, each with file:line evidence verified against today's main:
build_world_trianglesontlas_versioninstantiateAnimationAPIEN-053 and EN-060 embed the product owner's standing constraint verbatim: no performance win may cost visual quality — dev-box fps is accepted; real hardware is the target.
Summary by CodeRabbit