fix(web): EN-063 - bloom-shared compiles without models3d again#119
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis change enables wasm feature-gated model and ragdoll paths, adapts renderer bindings and formats for WebGPU, expands browser FFI and input support, adds web model and physics parity APIs, and implements clean shutdown behavior across native platforms. ChangesPlatform and renderer compatibility
Web integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 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: 7
🧹 Nitpick comments (1)
native/shared/src/textures.rs (1)
67-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse this helper in the existing loaders to prevent decode-path drift.
load_textureandload_imageduplicate the sameload_from_memory(...).to_rgba8()logic. Routing those methods throughdecode_rgba8would keep the native and web decoding behavior aligned as supported formats or conversion handling evolve.Proposed refactor
pub fn decode_rgba8(file_data: &[u8]) -> Option<(Vec<u8>, u32, u32)> { let img = image::load_from_memory(file_data).ok()?.to_rgba8(); let (w, h) = img.dimensions(); Some((img.into_raw(), w, h)) }Then replace the duplicated decode blocks in
load_textureandload_imagewith calls to this helper.🤖 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 `@native/shared/src/textures.rs` around lines 67 - 76, Update load_texture and load_image to use the existing decode_rgba8 helper instead of directly calling image::load_from_memory(...).to_rgba8(). Preserve each loader’s current handling of decoded pixels and dimensions while removing the duplicated decode logic.
🤖 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 `@native/web/bloom_glue.js`:
- Around line 316-327: Protect the localStorage.getItem calls in
imports.bloom_file_exists and imports.bloom_read_file with exception handling so
storage access failures do not escape the FFI wrapper. On failure, continue
through the existing manifest/syncFetchBytes fallback for bloom_file_exists and
syncFetchText fallback for bloom_read_file.
- Around line 267-277: Build texture arrays atomically in
native/web/bloom_glue.js within bloom_create_texture_array_from_files: if any
layer file is missing or decoding fails, reset the pending array and return 0
immediately instead of continuing. In native/web/src/parity_ffi.rs within the
corresponding texture-array construction logic, reject the entire array on any
layer dimension mismatch rather than skipping that layer; apply the required
change at both listed sites.
- Around line 105-106: Move the setupDomBridge() call from the initial DOM
wiring section to after the renderer initialization wait completes, ensuring
ENGINE is initialized before ResizeObserver can invoke bloom_resize. Keep the
existing DOM bridge setup behavior unchanged once initialization has finished.
- Around line 121-126: Require WebGPU support for at least 19 sampled textures
across all device setup paths: in native/web/bloom_glue.js lines 121-126,
request the adapter with the required 19-texture capability and fail fast when
unsupported; in native/web/src/lib.rs lines 143-151 and
native/shared/src/attach.rs lines 147-191, clamp
max_sampled_textures_per_shader_stage to a minimum of 19 before device creation.
In `@native/web/jolt_bridge.js`:
- Around line 927-946: Release the temporary Jolt settings wrapper immediately
after Create() in the six-DOF constraint flow. In native/web/jolt_bridge.js
lines 1039-1055, destroy the heightfield settings wrapper after its data has
been copied. In lines 1088-1108, update world teardown to evict that world's
entries from queryFilterCache so cached filters do not outlive the world.
In `@native/web/src/ragdoll_ffi.rs`:
- Around line 41-61: Make ragdoll activation transactional in
native/web/src/ragdoll_ffi.rs:41-61, native/web/src/ragdoll_ffi.rs:65-69, and
native/web/src/ragdoll_ffi.rs:99-146 by rejecting invalid or already-active
slots before calling the planning/build flow, then rolling back every body and
constraint created so far whenever capsule/body creation or a required SixDOF
constraint fails. Return 0.0 on any failure, and only mark the slot successful
after all planned attachments complete.
- Around line 65-94: Release each temporary capsule shape handle returned by
jb_shape_capsule after jb_body_create completes, including when body creation
fails, while preserving the existing zero-shape handling and body setup flow in
the builds loop.
---
Nitpick comments:
In `@native/shared/src/textures.rs`:
- Around line 67-76: Update load_texture and load_image to use the existing
decode_rgba8 helper instead of directly calling
image::load_from_memory(...).to_rgba8(). Preserve each loader’s current handling
of decoded pixels and dimensions while removing the duplicated decode logic.
🪄 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: 7cb1b3f5-40e9-48c6-80b0-9fbaad066ed7
⛔ Files ignored due to path filters (3)
native/shared/Cargo.lockis excluded by!**/*.locknative/web/Cargo.lockis excluded by!**/*.locknative/windows/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
docs/tickets.mdnative/linux/src/lib.rsnative/macos/src/lib.rsnative/shared/Cargo.tomlnative/shared/src/engine.rsnative/shared/src/ffi.rsnative/shared/src/lib.rsnative/shared/src/models.rsnative/shared/src/renderer/material_pipeline.rsnative/shared/src/renderer/material_system.rsnative/shared/src/renderer/mod.rsnative/shared/src/renderer/scene_pass.rsnative/shared/src/renderer/shaders/ao.rsnative/shared/src/renderer/shaders/core.rsnative/shared/src/renderer/shaders/post.rsnative/shared/src/renderer/shaders/ssgi.rsnative/shared/src/textures.rsnative/web/bloom_glue.jsnative/web/jolt_bridge.jsnative/web/src/input_ffi.rsnative/web/src/lib.rsnative/web/src/material_ffi.rsnative/web/src/parity_ffi.rsnative/web/src/physics_ffi.rsnative/web/src/ragdoll_ffi.rsnative/windows/src/lib.rspackage.jsonsrc/index.tssrc/math/index.tssrc/models/index.tstools/validate-ffi.js
| // 5. DOM wiring (input + HiDPI canvas sizing + audio unlock). | ||
| setupDomBridge(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Install the DOM bridge only after renderer initialization.
ResizeObserver can fire while requestAdapter() is awaited, calling bloom_resize before ENGINE is initialized. Move setupDomBridge() below the initialization wait.
Proposed fix
- setupDomBridge();
bloom.bloom_init_window(w, h, 0, 0);
const deadline = performance.now() + 15000;
while (bloom.bloom_is_initialized() < 0.5) {
...
}
+ setupDomBridge();Also applies to: 131-139
🤖 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 `@native/web/bloom_glue.js` around lines 105 - 106, Move the setupDomBridge()
call from the initial DOM wiring section to after the renderer initialization
wait completes, ensuring ENGINE is initialized before ResizeObserver can invoke
bloom_resize. Keep the existing DOM bridge setup behavior unchanged once
initialization has finished.
| const adapter = await navigator.gpu.requestAdapter({ powerPreference: 'high-performance' }) | ||
| .catch(() => null); | ||
| if (!adapter) { | ||
| throw new Error('No WebGPU adapter available. Check chrome://gpu — ' | ||
| + 'hardware acceleration must be enabled and the GPU not blocklisted.'); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C4 '19 textures|max_sampled_textures_per_shader_stage|requestAdapter' native/web native/sharedRepository: Bloom-Engine/engine
Length of output: 3542
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '110,170p' native/web/bloom_glue.js
printf '\n--- native/web/src/lib.rs ---\n'
sed -n '120,170p' native/web/src/lib.rs
printf '\n--- native/shared/src/attach.rs ---\n'
sed -n '150,180p' native/shared/src/attach.rsRepository: Bloom-Engine/engine
Length of output: 7031
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- native/web/src/lib.rs call sites ---'
rg -n -C3 'request_device|required_limits|max_sampled_textures_per_shader_stage|bloom_init_window|attach::|attach\(' native/web native/shared
printf '\n%s\n' '--- native/shared/src/attach.rs outline ---'
ast-grep outline native/shared/src/attach.rs --view expanded
printf '\n%s\n' '--- native/web/src/lib.rs outline ---'
ast-grep outline native/web/src/lib.rs --view expandedRepository: Bloom-Engine/engine
Length of output: 37605
Require a 19-texture minimum for the WebGPU device request.
native/web/bloom_glue.js:121-126should fail fast when the adapter cannot support the renderer’s 19 sampled textures.native/web/src/lib.rs:143-151andnative/shared/src/attach.rs:147-191should clampmax_sampled_textures_per_shader_stageto at least 19 instead of inheriting the adapter/default value and letting pipeline creation fail later.
📍 Affects 2 files
native/web/bloom_glue.js#L121-L126(this comment)native/web/src/lib.rs#L143-L151
🤖 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 `@native/web/bloom_glue.js` around lines 121 - 126, Require WebGPU support for
at least 19 sampled textures across all device setup paths: in
native/web/bloom_glue.js lines 121-126, request the adapter with the required
19-texture capability and fail fast when unsupported; in native/web/src/lib.rs
lines 143-151 and native/shared/src/attach.rs lines 147-191, clamp
max_sampled_textures_per_shader_stage to a minimum of 19 before device creation.
| imports.bloom_create_texture_array_from_files = (paths, format, mipLevels) => { | ||
| if (!bloom.bloom_texture_array_files_push) return 0; | ||
| bloom.bloom_texture_array_files_reset(); | ||
| for (const raw of String(paths).split(',')) { | ||
| const p = raw.trim(); | ||
| if (!p) continue; | ||
| const data = syncFetchBytes(p); | ||
| if (data) bloom.bloom_texture_array_files_push(data); | ||
| else console.warn('[texarray] missing layer file:', p); | ||
| } | ||
| return bloom.bloom_texture_array_files_commit(format, mipLevels); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Build texture arrays atomically to preserve layer indices.
Both paths silently omit invalid layers, compacting subsequent layers into different indices and causing materials to sample the wrong textures.
native/web/bloom_glue.js#L267-L277: abort, reset, and return0when a file is missing or decoding fails.native/web/src/parity_ffi.rs#L176-L199: reject the complete array on any dimension mismatch instead of skipping that layer.
📍 Affects 2 files
native/web/bloom_glue.js#L267-L277(this comment)native/web/src/parity_ffi.rs#L176-L199
🤖 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 `@native/web/bloom_glue.js` around lines 267 - 277, Build texture arrays
atomically in native/web/bloom_glue.js within
bloom_create_texture_array_from_files: if any layer file is missing or decoding
fails, reset the pending array and return 0 immediately instead of continuing.
In native/web/src/parity_ffi.rs within the corresponding texture-array
construction logic, reject the entire array on any layer dimension mismatch
rather than skipping that layer; apply the required change at both listed sites.
| imports.bloom_file_exists = (path) => { | ||
| const p = String(path); | ||
| if (localStorage.getItem(LS_PREFIX + p) !== null) return 1; | ||
| if (manifestPaths) return manifestPaths.has(p) ? 1 : 0; | ||
| return syncFetchBytes(p) !== null ? 1 : 0; | ||
| }; | ||
| imports.bloom_read_file = (path) => { | ||
| const v = localStorage.getItem(LS_PREFIX + String(path)); | ||
| return v === null ? '' : v; // plain string; Perry re-encodes via wrapFfiForI64 | ||
| const p = String(path); | ||
| const v = localStorage.getItem(LS_PREFIX + p); | ||
| if (v !== null) return v; | ||
| const text = syncFetchText(p); | ||
| return text === null ? '' : text; // plain string; Perry re-encodes via wrapFfiForI64 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard localStorage reads against access failures.
Unlike writes, getItem() is unprotected. Restricted storage access will throw through the FFI wrapper and break fileExists/readFile; catch it and continue with the asset fallback.
🤖 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 `@native/web/bloom_glue.js` around lines 316 - 327, Protect the
localStorage.getItem calls in imports.bloom_file_exists and
imports.bloom_read_file with exception handling so storage access failures do
not escape the FFI wrapper. On failure, continue through the existing
manifest/syncFetchBytes fallback for bloom_file_exists and syncFetchText
fallback for bloom_read_file.
| let eng = engine(); | ||
|
|
||
| let builds = { | ||
| let Some(a) = eng.models.get_animation(anim) else { | ||
| return 0.0; | ||
| }; | ||
| // 12 bodies / 0.38 radius fraction — same sweet spot as native (spine | ||
| // + limbs; past that you buy fingers, which cost solver time and buy | ||
| // jitter). | ||
| bloom_shared::ragdoll::plan( | ||
| a, | ||
| scale as f32, | ||
| [px as f32, py as f32, pz as f32], | ||
| rot_y as f32, | ||
| 12, | ||
| 0.38, | ||
| ) | ||
| }; | ||
| if builds.is_empty() { | ||
| return 0.0; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make ragdoll activation transactional. Validate the slot before allocation, and if any required body or joint fails, destroy all constraints and bodies created so far before returning 0.0.
native/web/src/ragdoll_ffi.rs#L41-L61: reject invalid or active slots before planning/building.native/web/src/ragdoll_ffi.rs#L65-L69: abort and roll back when a capsule or body cannot be created.native/web/src/ragdoll_ffi.rs#L99-L146: abort and roll back when a required SixDOF constraint returns zero; return success only after a complete attachment.
📍 Affects 1 file
native/web/src/ragdoll_ffi.rs#L41-L61(this comment)native/web/src/ragdoll_ffi.rs#L65-L69native/web/src/ragdoll_ffi.rs#L99-L146
🤖 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 `@native/web/src/ragdoll_ffi.rs` around lines 41 - 61, Make ragdoll activation
transactional in native/web/src/ragdoll_ffi.rs:41-61,
native/web/src/ragdoll_ffi.rs:65-69, and native/web/src/ragdoll_ffi.rs:99-146 by
rejecting invalid or already-active slots before calling the planning/build
flow, then rolling back every body and constraint created so far whenever
capsule/body creation or a required SixDOF constraint fails. Return 0.0 on any
failure, and only mark the slot successful after all planned attachments
complete.
| for b in builds.iter() { | ||
| let shape = jb_shape_capsule(b.half_height as f64, b.radius as f64); | ||
| if shape == 0.0 { | ||
| bodies.push(0.0); | ||
| continue; | ||
| } | ||
| let q = bloom_shared::ragdoll::quat_from_mat(&b.world); | ||
| let body = jb_body_create( | ||
| world, | ||
| shape, | ||
| 2.0, | ||
| b.world[3][0] as f64, | ||
| b.world[3][1] as f64, | ||
| b.world[3][2] as f64, | ||
| q[0] as f64, | ||
| q[1] as f64, | ||
| q[2] as f64, | ||
| q[3] as f64, | ||
| 1.0, | ||
| ); | ||
| if body != 0.0 { | ||
| jb_body_set_friction(body, 0.8); // corpses do not skate | ||
| jb_body_set_restitution(body, 0.02); // they do not bounce | ||
| jb_body_set_linear_damping(body, 0.20); | ||
| // Angular damping does most of the work of making this read as a | ||
| // body rather than a rag. | ||
| jb_body_set_angular_damping(body, 0.75); | ||
| } | ||
| bodies.push(body); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline native/web/src/physics_ffi.rs \
--match 'jb_shape_release|bloom_physics_shape_release'
rg -n -C8 'registerShape|shapeRelease|bodyCreate' native/web/jolt_bridge.jsRepository: Bloom-Engine/engine
Length of output: 6918
Release the temporary capsule shape handle after jb_body_create.
These bodies keep their own shape reference; dropping the capsule handle here prevents the JS shape map from growing forever.
🤖 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 `@native/web/src/ragdoll_ffi.rs` around lines 65 - 94, Release each temporary
capsule shape handle returned by jb_shape_capsule after jb_body_create
completes, including when body creation fails, while preserving the existing
zero-shape handling and body setup flow in the builds loop.
build-web has been red on every main run since PR #107 (EN-055): models.rs grew ungated references to anim_mixer, gltf, image_dds and StagedModel, all of which only exist when the models3d feature is on - and the wasm web-feature check is the one build that runs without it. Two-part fix: - UN-gate anim_mixer in lib.rs. The mixer is pure per-instance state embedded in the always-compiled ModelAnimation struct - no heavy deps. Gating it on models3d was wrong from the start; models3d exists to drop gltf/gltf_json/image_dds from pure-2D binaries, not the mixer math. - Move everything that actually touches the optional deps - the load_gltf* family and its private helpers, ~1200 lines - into a new models_gltf.rs, included from models.rs as a single models3d-gated module. The four ModelManager loader methods carry the same cfg. Every FFI call site already had feature-off stubs in ffi_core/models.rs, so no surface changes in any configuration. The split also takes models.rs from 2346 to ~1090 lines, retiring its EN-052 grandfather entry in tools/file-lines-baseline.json (the ratchet script's own instruction once a file drops under the 2000 limit). Verified locally: wasm --no-default-features --features web (the failing CI command) compiles clean; wasm web,models3d (the native/web shape) and native default-features both still compile. tickets.md EN-063 marked shipped; its loud-gate follow-up stays open.
fa8e9d4 to
7b8c907
Compare
|
Rebuilt from The stray commit Also changed in the rebuild: instead of 19 scattered |
1 participant
Fixes EN-063:
build-webhas been red on every main run since PR #107 (EN-055) —models.rsgrew ungated references toanim_mixer,gltf,image_dds, andStagedModel, and the wasm web-feature check is the one build that runs withoutmodels3d. PRs #108–#117 all merged over the inherited red.The fix, two parts:
anim_mixerinlib.rs— the mixer is pure per-instance state embedded in the always-compiledModelAnimationstruct, no heavy deps.models3dexists to dropgltf/image_ddsfrom pure-2D binaries (EN-014), not the mixer math.ModelManagerloader methods, the fiveload_gltf*functions, and their ten private helpers get#[cfg(feature = "models3d")]. Every FFI call site already had feature-off stubs inffi_core/models.rs, so no surface changes in any configuration.Verified locally: wasm
--no-default-features --features web(the exact failing CI command) compiles clean; wasmweb,models3d(the native/web shape) and native default-features both still compile. Remaining warnings on the web config are pre-existing renderer ones, untouched here.This PR's own
build-webcheck doubles as the proof — it should be the first green one since #104.Also marks EN-063 shipped in
tickets.md(the 'make the red gate loud' follow-up stays open — five PRs merged over this without anyone noticing).Summary by CodeRabbit
New Features
Bug Fixes