-
Notifications
You must be signed in to change notification settings - Fork 5
fix: prevent camera reset when toggling pathtracing with CurveAttachment #866
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
When pathtracing was toggled, cameras using CurveAttachment positions would reset to the first point of the curve instead of maintaining their current position. Root cause: PathTracingRenderer returned different component types depending on pathtracing state (Fragment vs Pathtracer), causing React to remount children. When Camera remounted, its state reinitialized to the first curve point instead of the resolved CurveAttachment position. Changes: - PathTracingRenderer: Always render Pathtracer with enabled prop to maintain stable React tree structure and prevent child remounting - Canvas: Remove pathtracingEnabled from Canvas key (unnecessary) - Curve: Return empty group instead of null when pathtracing enabled to keep curveRef registered for CurveAttachment resolution Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughMemoized camera position resolution added via a new Changes
Sequence Diagram(s)(omitted — changes are internal/conditional rendering and utilities that do not introduce a new multi-component sequential workflow warranting a diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Changed room_keys.members() to room_keys.users() - the members() method doesn't exist on RoomKeys class. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/components/PathTracingRenderer.tsx (1)
45-48: Remove Fragment fallback to maintain stable component tree and prevent unnecessary remounts.Lines 24-25 return a Fragment when
enabledis false, which changes the wrapper type based on the enabled state. This violates the pattern already established on line 8 (which renders<Pathtracer enabled={false}>for loading state) and can cause remounts when the enabled prop changes. Always renderPathtracerand let theenabledprop control behavior, as shown in the suggested fix.Suggested fix
- // Pass through if not enabled - if (!enabled) { - return <>{children}</>; - } - - return ( - <Pathtracer - minSamples={min_samples} - samples={samples} - bounces={bounces} - tiles={tiles} - enabled={enabled} - > + return ( + <Pathtracer + minSamples={min_samples} + samples={samples} + bounces={bounces} + tiles={tiles} + enabled={enabled} + > {enabled && <PathtracingUpdater settings={settings} />} {enabled && <PathtracingCaptureProvider />} {enabled && environment_preset !== "none" && ( <Environment preset={environment_preset} background={environment_background} backgroundBlurriness={environment_blur} environmentIntensity={environment_intensity} /> )} {children} </Pathtracer> );
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #866 +/- ##
==========================================
- Coverage 79.81% 79.79% -0.02%
==========================================
Files 165 165
Lines 20213 20213
==========================================
- Hits 16133 16129 -4
- Misses 4080 4084 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Known issue:
Now positions is 0,0,0 and the camera only becomes responsive again, when setting positions manually.
Also webgl context loss on pathtracing changes.
When pathtracing was toggled, cameras using CurveAttachment positions would reset to the first point of the curve instead of maintaining their current position.
Root cause: PathTracingRenderer returned different component types depending on pathtracing state (Fragment vs Pathtracer), causing React to remount children. When Camera remounted, its state reinitialized to the first curve point instead of the resolved CurveAttachment position.
Changes:
Summary by CodeRabbit
Bug Fixes
Improvements
Refactor
Chore
✏️ Tip: You can customize this high-level summary in your review settings.