Upload log-u8 density grids directly to GPU in reduced-motion mode - #369
Upload log-u8 density grids directly to GPU in reduced-motion mode#369aymuos15 wants to merge 5 commits into
Conversation
Density count grids ship as log-u8 (§29). The client decoded that to f32 (`lodDecodeLogU8`), then `lodWriteGridTexture` re-encoded it with the same formula before `texImage2D` — at the wire's own max the two cancel exactly, so both passes rebuilt the bytes they started from. Reduced motion made that pure waste: it pins normMax to `d.max` and runs no easing, yet `lodStartNormAnim` still re-encoded and re-uploaded once, so a reply cost one decode, two encodes, and three full-grid allocations. Count-only grids now upload the wire buffer as their R8 texture and keep no f32 grid; because the easing paths skip a missing grid, `lodStartNormAnim` returns early and its re-upload goes with it. 6.3 ms and 1152 KB per reply at 512x384. The gate stays narrow — count-only, at the wire's max. Every reply sets `enc: "log-u8"` and mean-color replies add `rgba` on top, so log-u8 alone proves nothing: those grids upload RGBA8, need the counts in the `1 - (1 - a_pt)^k` exponent, and still read `d.grid` for legend hover. And only reduced motion pins normMax to `d.max` — eased textures are written at other maxima, so animated transitions keep the round-trip. `lodUploadGridBytes` now does the texture setup for both paths, so their unpack and sampling state cannot drift.
|
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)
📝 WalkthroughWalkthroughThe density texture upload path is refactored into shared helpers. Eligible reduced-motion ChangesDensity texture upload flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Merging this PR will not alter performance
Comparing Footnotes
|
…ll grid A direct-wire density stored grid: null, but the home-extent restore in _requestSampleRebin re-uploads _homeDensity.grid, throwing a TypeError under reduced motion after a zoom-in-and-return. Keep a copy of the log-u8 wire bytes on the density object and re-upload those when grid is null; the bytes stay valid because reduced motion pins normMax to d.max. This also keeps the texture rebuildable client-side (§27) at a quarter of the old f32 grid's footprint.
|
Should I include a change to |
Reduced-motion count-only log-u8 replies had no coverage: pytest, all three smokes, and the upload-hash check all passed with a null-grid dereference in the home-extent restore. The new probes force reduced motion, assert the wire bytes upload verbatim and stay retained on density.wire, and drive _requestSampleRebin's home restore to prove it re-uploads from the retained copy instead of throwing. Both fail against the pre-fix client.
Exposure easing never re-reads the color plane: rgba densities null their norm anim (T4 is count-only), so lodStepNorm's rgba read is unreachable for them. The copy is still required — legend-hover dimming and the home-extent restore re-read it after the wire buffer is released.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/render_smoke_nonumpy.py (1)
803-808: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the bytes actually passed to WebGL.
dwireverifiesdensity.wireand texture existence, not the texture upload payload. A regression that decodes/re-encodes—or uploads incorrect bytes—while retainingdensity.wirestill passes. Spy on the relevanttexImage2Dcall (or read back rendered cells) and compare itsUint8Arraypayload withrenc.🤖 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 `@scripts/render_smoke_nonumpy.py` around lines 803 - 808, Update the smoke test around the density upload and `dwire` assertion to capture the relevant WebGL `texImage2D` call and inspect its `Uint8Array` payload. Compare the bytes actually submitted to WebGL with `renc`, while retaining the existing wire, dimensions, normalization, and texture checks.
🤖 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.
Nitpick comments:
In `@scripts/render_smoke_nonumpy.py`:
- Around line 803-808: Update the smoke test around the density upload and
`dwire` assertion to capture the relevant WebGL `texImage2D` call and inspect
its `Uint8Array` payload. Compare the bytes actually submitted to WebGL with
`renc`, while retaining the existing wire, dimensions, normalization, and
texture checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 44d727c7-feaa-43ec-af1b-31080d50ca09
📒 Files selected for processing (3)
js/src/45_lod.tsjs/src/54_kernel.tsscripts/render_smoke_nonumpy.py
🚧 Files skipped from review as they are similar to previous changes (1)
- js/src/45_lod.ts
yes please. since we are leveraging mostly automated code generation at this point, it's important that the spec and code stays aligned. while we do regularly have an agent comparing the spec documents with the implementation to assure conformance, it makes reviewing individual PRs easier when changes are documented in the spec within the same PR. and thanks again for your interest and contributions to making this library the best it can be 🚀 |
|
Thank you very much for explaining in detail, will keep that in mind going forward. |
Reduced-motion density replies decoded the log-u8 wire grid to f32 and re-encoded it with the identical formula before upload. Count-only grids now hand
texImage2Dthe wire bytes unchanged, with pixel-identical output. A copy of the wire bytes is retained on the density object so home-extent restores and context recovery can re-upload without a decoded grid (§27).CPU removed per density reply, seeded power-law grid, excluding the unchanged
gl.texImage2D:Verification
texImage2Dpayload hashes identically to main in the same order (minus the redundantlodStartNormAnimre-upload), with identical rendered outputencode(decode(u)) == ufor all 256 byte values × 8 max valuesdwire,drestore) cover the direct-wire upload and the home-extent restore; both fail against the first commit aloneuv run pytest, Ruff, and pre-commit hooks passMeasured on: Ubuntu 22.04.5 (Linux 6.8, x86_64), Intel i7-12800H, 30 GB RAM, Node 22.22.2, headless Chromium via SwiftShader.
Benchmark (Node)
Run from the repo root:
node bench.mjsSummary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Tests
Documentation