Skip to content

perf: stop re-subscribing the replay render loop every frame#906

Open
Anexus5919 wants to merge 1 commit into
Somil450:mainfrom
Anexus5919:fix/replay3d-raf-loop-deps
Open

perf: stop re-subscribing the replay render loop every frame#906
Anexus5919 wants to merge 1 commit into
Somil450:mainfrom
Anexus5919:fix/replay3d-raf-loop-deps

Conversation

@Anexus5919

Copy link
Copy Markdown
Contributor

📌 Related Issue

Fixes #869


📝 Description

The requestAnimationFrame render-loop useEffect in Replay3DModel.tsx had currentFrameIdx in its dependency array, and the loop advances currentFrameIdx (setCurrentFrameIdx) on every playback step. So on every frame the effect tore down (cancelAnimationFrame) and re-subscribed (requestAnimationFrame), rebuilding the renderLoop closure. The other dependencies are memoized/stable, so currentFrameIdx was the only one changing during playback.

🔹 What has been changed?

  • src/components/Replay3DModel.tsx: the loop now reads the frame index from a ref (currentFrameIdxRef, kept current by a small dedicated effect) instead of the closure-captured currentFrameIdx, and currentFrameIdx is removed from the render-loop effect's dependency array. The loop is now set up once (re-subscribing only when frames/isPlaying/modelLoaded/etc. actually change).

🔹 Why are these changes needed?

  • Re-subscribing the RAF loop every frame is wasteful (effect cleanup/setup, closure rebuild, full dependency comparison per frame). The loop already advanced playback via frameFloatRef, so currentFrameIdx did not need to be a reactive dependency; reading it from an always-current ref preserves behaviour (including the paused/scrub render) while setting the loop up once.

🛠️ Type of Change

  • ⚡ Performance Improvement

🧪 Testing

✅ Tests Performed

  • Tested locally
  • npx tsc --noEmit: Replay3DModel.tsx is clean (the only pre-existing tsc errors are in exerciseEngine.ts, unrelated).
  • npx eslint src/components/Replay3DModel.tsx: 0 problems — importantly, no react-hooks/exhaustive-deps warning, confirming the effect no longer references currentFrameIdx (all uses go through the ref).
  • Reviewed behaviour: during playback the loop renders from frameFloatRef (unchanged); when paused/scrubbing it renders currentFrameIdxRef.current, which the small sync effect keeps current — so the displayed frame is the same, without the per-frame re-subscribe.

🔹 Note on automated tests

No runtime unit test is included: the loop is a WebGL requestAnimationFrame loop inside a large Three.js component, which is not feasible to unit test without a full WebGL + DOM harness. Verification is by tsc, eslint (exhaustive-deps), and a behaviour review.

🌐 Browsers Tested

Not applicable (render-loop lifecycle change).


📷 Screenshots / Demo (if applicable)

Not applicable.


📋 Checklist

  • I have read the project's CONTRIBUTING guidelines
  • My code follows the project style guidelines
  • I have performed a self-review of my code
  • I have tested my changes locally
  • I have added/updated documentation where necessary (not applicable)
  • My changes do not introduce new warnings or errors
  • This PR is linked to an existing issue

💬 Additional Notes

Branched from latest upstream/main (5464425). Small, self-contained change (6 insertions, 4 deletions).

@vercel

vercel Bot commented Jun 22, 2026

Copy link
Copy Markdown

@Anexus5919 is attempting to deploy a commit to the somiljain2024-4175's projects Team on Vercel.

A member of the Team first needs to authorize it.

@Anexus5919

Copy link
Copy Markdown
Contributor Author

@Somil450 @diksha78dev Kindly have a review on this pr. Thanks!

The render-loop useEffect listed currentFrameIdx in its dependency array,
and the loop advances currentFrameIdx as it plays. So on every frame step the
effect tore down (cancelAnimationFrame) and re-subscribed
(requestAnimationFrame), recreating the renderLoop closure each time.

Read the frame index from a ref (kept current by a small effect) inside the
loop and drop currentFrameIdx from the dependency array, so the loop is set
up once. Behaviour is unchanged; the loop already advanced playback via
frameFloatRef, and the remaining deps are memoized.
@Anexus5919 Anexus5919 force-pushed the fix/replay3d-raf-loop-deps branch from e82c6b1 to 5523032 Compare June 23, 2026 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: Replay3DModel render loop re-subscribes the requestAnimationFrame effect on every playback frame (currentFrameIdx in deps)

1 participant