arkavo-org / arkavo-org/VRMMetalKit

VRMNode: amortize updateLocalMatrix via dirty flag in updateWorldTransform

Open
#211 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Swift
Stars
6
Forks
2
Avg merge
18h 51m
Merged PRs (30d)
26

Description

## Summary

Surfaced by the PR #208 review (follow-up to vrm-conformance #206).

#206's fix made `updateWorldTransform()` always call `updateLocalMatrix()` first so external callers (e.g. the vrm-conformance adapter setting `node.translation` directly) get correct propagation without remembering the convention. The cost: internal callers that already call `updateLocalMatrix()` explicitly before `updateWorldTransform()` now pay the rebuild *twice*.

Concretely:

- `AnimationPlayer.update()` calls `model.updateNodeTransforms()` up to twice per frame (pre- and post-constraints).
- `SpringBoneComputeSystem` calls `updateLocalMatrix()` + `updateWorldTransform()` inside its per-chain update loop (lines 1201–1202, 1285–1286, 1291–1292).
- `ProceduralAnimation.swift`, `VRMSkinning.swift`, `VRMLookAtController.swift`, `ConstraintSolver.swift` all follow the same set-T/R/S → `updateLocalMatrix()` → `updateWorldTransform()` pattern.

For a ~200-node avatar at 60 Hz that's roughly:
```
200 nodes × (quaternion→matrix + 1× 4×4 multiply) × 2 passes ≈ 40k extra fp ops/frame
```

Well below the noise floor on M-series, but the package ships `VRMBenchmark`'s `transforms` mode specifically to measure this. Expect a visible (~1.5–2×) regression on that bench mode.

## Why it matters

This is the kind of "death by a thousand redundant calls" that compounds when more systems start participating in the per-frame transform pass. Worth fixing while the surface area is still small.

## Proposed solution

Add a `private var localMatrixDirty: Bool = true` flag to `VRMNode`:

1. `translation` / `rotation` / `scale` property setters mark it dirty (`didSet { localMatrixDirty = true }`).
2. `updateLocalMatrix()` clears the flag after rebuilding.
3. `updateWorldTransform()` early-outs the `updateLocalMatrix()` call when not dirty.

External callers (e.g. the conformance adapter) hit the rebuild because their setter triggers `didSet`. Internal callers who explicitly `updateLocalMatrix()` before `updateWorldTransform()` see the second call become a no-op.

The handful of internal spots that write the stored field directly (e.g. `init`, `resetToBindPose`, the matrix-init path) can opt in with an explicit `markLocalMatrixDirty()` — easy to audit (grep for `translation =`, `rotation =`, `scale =`).

## Alternative

Add `localTranslation` / `localScale` setters mirroring the existing `localRotation` setter (which already auto-refreshes `localMatrix`). Migrate internal callers to those. External callers using `node.translation = X` get a deprecation warning. Closes the API asymmetry that caused #206 in the first place — see follow-up issue for that.

## Verification plan

Before/after on `VRMBenchmark transforms` mode (p50/p99). Should show the regression introduced by #206's safety net is recovered by the dirty-flag optimisation.

## Out-of-scope context

This is a *performance* follow-up, not a correctness one. The #206 fix as-shipped is correct; this issue exists to recover the few-microsecond regression it introduced.

Contributor guide

Open the contributing guide

Research direction

Locate VRMNode and inspect its translation, rotation, scale, updateLocalMatrix(), and updateWorldTransform() paths, then audit the direct writes noted in init, resetToBindPose, and matrix initialization. Review the listed AnimationPlayer, SpringBoneComputeSystem, ProceduralAnimation.swift, VRMSkinning.swift, VRMLookAtController.swift, and ConstraintSolver.swift call sites, then run VRMBenchmark in transforms mode; done means redundant local-matrix work is avoided without changing transform behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
swift
Domain
computer-graphics, performance
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
67/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.