EN-055: animation instancing + staged-commit aux-texture/normal-kind fixes#107
Conversation
…ered instantiateAnimation(src): a fresh animation INSTANCE (own mixer, joint matrices, mask cache) over an Arc-shared clip set - N characters no longer cost N GLB re-parses. ModelAnimation's parsed data (skeleton, keyframe tracks, rest rotations) is immutable after load and now shared via Arc; deref coercion absorbed the whole refactor (cargo check: zero fallout sites). AnimMixer moved to its own module (a first slice of EN-052's models.rs split, and it pays the line-ratchet budget for the additions). FFI + manifest + TS API + web impl + watchos stub; validate-ffi and check-file-lines both green locally; 127 shared tests pass. Honest measurement, which reframed the ticket: on the shooter's boot the per-slot animation parses were 43 ms, not the 5.5 s SH-049 attributed to them - the real cost was SERIAL loadModel texture decode. The API is still right (any crowd wants instances, and slot instancing measures 2-4 ms), but the boot win comes from the staged path below. bloom_commit_model, fixed for real use (the shooter's parallel boot is the first consumer): - it remapped ONLY texture_idx; normal/MR/emissive/occlusion indices were left pointing at staged-local numbers (the round-5 'stale aux-texture remap' bug) - any model with SH-046-class maps lost them silently through this path. - staged textures all registered via register_texture (sRGB/albedo path); normal maps need register_texture_kind's linear + LEADR-mip path or committed models shade flatter than the same GLB via loadModel. StagedTexture now carries is_normal, set by load_gltf_staged from the material pre-walk (mirrors load_gltf_with_textures). Verified end-to-end by the shooter (see its EN-055 PR): boot 8702 -> 4372 ms; title-screen capture shows the bsuit's normal/MR/emissive maps pixel-faithful through the staged path; AITEST behavioural run clean. Also: fetch-dxc.ps1 no longer claims missing DXC DLLs are non-fatal. With DynamicDxc configured, a missing dxcompiler.dll drops the ENTIRE DX12 backend out of the wgpu instance, and the broken Vulkan surface path makes that a hard crash at boot - reproduced twice today when the DLLs vanished from the shooter root. EN-058's severity goes up accordingly.
📝 WalkthroughWalkthroughAnimation instances now share parsed clip data while owning independent mixer and joint state, with new native and TypeScript APIs. Staged model textures preserve normal-map classification and remap all material texture references during commit. DXC failure documentation is corrected. ChangesAnimation instantiation
Texture staging and commit
DXC configuration documentation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TypeScript
participant NativeFFI
participant ModelManager
participant SourceAnimation
TypeScript->>NativeFFI: bloom_instantiate_animation(src)
NativeFFI->>ModelManager: instantiate_animation(src)
ModelManager->>SourceAnimation: read shared animation data
ModelManager-->>NativeFFI: create fresh runtime state
NativeFFI-->>TypeScript: return new animation handle
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
package.json (1)
1186-1192: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClear the consumer game's
.perry-cache/.As per coding guidelines, remember to clear the consumer game's
.perry-cache/directory since a new FFI function has been added to the manifest.🤖 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 `@package.json` around lines 1186 - 1192, Clear the consumer game's .perry-cache/ directory after adding the bloom_instantiate_animation FFI function to the manifest, ensuring stale generated cache data is removed before validation.Source: Coding guidelines
native/shared/src/models.rs (1)
505-527: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInitialize joint matrices to identity to prevent pose leaking.
If
instantiate_animationis called on a source handle that is already playing, the new instance will inherit the source's current pose. This can cause a 1-frame visual pop if the new instance is rendered before its firstadvance_and_updatecall. Initializing these vectors tomat4_identity()ensures a clean rest-pose start.♻️ Proposed fix
Some(a) => ModelAnimation { skeleton: a.skeleton.clone(), animations: a.animations.clone(), ref_rest_rotations: a.ref_rest_rotations.clone(), - joint_matrices: a.joint_matrices.clone(), + joint_matrices: vec![mat4_identity(); a.joint_matrices.len()], mixer: AnimMixer::default(), - joint_world: a.joint_world.clone(), + joint_world: vec![mat4_identity(); a.joint_world.len()], mask_weights: vec![0.0; a.mask_weights.len()], mask_cached_root: -1, },🤖 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/models.rs` around lines 505 - 527, Update instantiate_animation so the new ModelAnimation initializes joint_matrices and joint_world with identity matrices sized to the source skeleton, rather than cloning the source’s current pose; keep the shared skeleton/animation data and fresh mixer/mask state unchanged.
🤖 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/shared/src/staging.rs`:
- Around line 10-14: Replace the image-level is_normal tracking in the staged
texture flow with per-material-use texture-kind tracking. Update
load_gltf_staged and the commit-time registration path around
register_texture_kind so an image referenced as both normal and base-color is
registered appropriately for each usage, either by creating separate staged
entries or by propagating the role through commit; do not apply one global role
to all references.
---
Nitpick comments:
In `@native/shared/src/models.rs`:
- Around line 505-527: Update instantiate_animation so the new ModelAnimation
initializes joint_matrices and joint_world with identity matrices sized to the
source skeleton, rather than cloning the source’s current pose; keep the shared
skeleton/animation data and fresh mixer/mask state unchanged.
In `@package.json`:
- Around line 1186-1192: Clear the consumer game's .perry-cache/ directory after
adding the bloom_instantiate_animation FFI function to the manifest, ensuring
stale generated cache data is removed before validation.
🪄 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: 5cb0550d-504e-4bc4-9742-3904becd8b46
📒 Files selected for processing (11)
native/shared/src/anim_mixer.rsnative/shared/src/ffi_core/models.rsnative/shared/src/lib.rsnative/shared/src/models.rsnative/shared/src/staging.rsnative/watchos/src/ffi_stubs.rsnative/web/src/material_ffi.rspackage.jsonsrc/index.tssrc/models/index.tstools/fetch-dxc.ps1
| /// Normal maps need `register_texture_kind`'s linear-space + LEADR mip | ||
| /// path at commit time; registering them like albedo (sRGB) visibly | ||
| /// flattens the shading. Set by `load_gltf_staged` from the material's | ||
| /// `normal_texture` references, mirroring `load_gltf_with_textures`. | ||
| pub is_normal: bool, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file outline ==\n'
ast-grep outline native/shared/src/staging.rs --view expanded || true
printf '\n== relevant symbols ==\n'
rg -n "is_normal|load_gltf_staged|decode_and_stage_texture|register_texture_kind|StagedTexture|normal_texture|base_color|texture kind" native/shared/src/staging.rs native/shared/src -S
printf '\n== staging.rs excerpt ==\n'
sed -n '1,220p' native/shared/src/staging.rs
printf '\n== nearby commit/registry code ==\n'
rg -n "register_texture_kind|commit.*texture|is_normal" native/shared/src -SRepository: Bloom-Engine/engine
Length of output: 23816
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== list files in native/shared/src ==\n'
git ls-files native/shared/src | sed -n '1,120p'
printf '\n== ast outline for candidate files that mention registration ==\n'
for f in $(git ls-files native/shared/src | grep -E 'staging\.rs|texture|gltf|commit'); do
echo "--- $f ---"
ast-grep outline "$f" --view expanded 2>/dev/null || true
doneRepository: Bloom-Engine/engine
Length of output: 6973
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== models.rs around load_gltf_with_textures ==\n'
sed -n '1430,1535p' native/shared/src/models.rs
printf '\n== models.rs around load_gltf_staged ==\n'
sed -n '1800,1875p' native/shared/src/models.rs
printf '\n== texture_store.rs register_texture_kind ==\n'
sed -n '80,165p' native/shared/src/renderer/texture_store.rs
printf '\n== ffi_core/models.rs texture commit path ==\n'
sed -n '800,875p' native/shared/src/ffi_core/models.rsRepository: Bloom-Engine/engine
Length of output: 17010
Track texture kind per material use, not per image. A glTF image can be reused as both a normal map and a base-color texture; this image-level is_normal flag forces one registration kind for every reference to that image, so one of the roles renders incorrectly. Split the staged texture by usage or carry the role through commit.
🤖 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/staging.rs` around lines 10 - 14, Replace the image-level
is_normal tracking in the staged texture flow with per-material-use texture-kind
tracking. Update load_gltf_staged and the commit-time registration path around
register_texture_kind so an image referenced as both normal and base-color is
registered appropriately for each usage, either by creating separate staged
entries or by propagating the role through commit; do not apply one global role
to all references.
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.
Implements EN-055 (engine#106) and fixes the two staged-path bugs its consumer hit.
instantiateAnimation
ModelAnimation's parsed clip data (skeleton, keyframe tracks, rest rotations) is immutable after load — nowArc-shared, with mixer/joint state per instance.instantiateAnimation(src)allocates a new handle over the shared set. Deref coercion absorbed the refactor (zero fallout sites).AnimMixermoved to its own module — first slice of EN-052, and it pays the models.rs line-ratchet budget. FFI on all surfaces (macro + manifest + TS + web real-impl + watchOS stub);validate-ffi+check-file-linesgreen; 127 shared tests pass.Measurement corrected the ticket's premise: the shooter's per-slot anim parses were 43 ms, not the 5.5 s SH-049 blamed (real cost = serial
loadModeltexture decode). The API is still right — crowds want instances (2–4 ms for 14 slots) — but the boot win came from fixing the staged path:bloom_commit_model fixes (quality-critical)
texture_idxwas remapped; normal/MR/emissive/occlusion kept staged-local indices — the round-5 "stale aux-texture remap" bug, which silently stripped SH-046 character maps from any staged model.register_texture; normal maps now carryis_normal(set byload_gltf_staged's material pre-walk) and register viaregister_texture_kind's linear/LEADR path, matchingloadModel.Verified by the consumer (shooter EN-055 PR)
Boot 8,702 → 4,372 ms (parallel
stageModels+ fixed commits); title capture shows the bsuit's maps pixel-faithful through the staged path; AITEST behavioral run clean.Also
fetch-dxc.ps1no longer claims missing DXC DLLs are non-fatal — withDynamicDxc, a missingdxcompiler.dlldrops the entire DX12 backend from the wgpu instance and the broken Vulkan surface path turns that into a boot crash (reproduced twice today). EN-058 severity upgraded.Summary by CodeRabbit
New Features
Bug Fixes