-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix(viewer): wall-clock scene-ready cap + skip unreachable dirty nodes #484
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -249,11 +249,22 @@ function ToneMappingExposure() { | |
| } | ||
|
|
||
| function hasPendingSceneBuildWork() { | ||
| const { dirtyNodes, nodes } = useScene.getState() | ||
| const { dirtyNodes, nodes, rootNodeIds } = useScene.getState() | ||
|
|
||
| for (const id of dirtyNodes) { | ||
| const node = nodes[id] | ||
| if (!node) continue | ||
| // Unreachable nodes (orphaned by a broken detach: the parent doesn't list | ||
| // them in `children`, or no parent and not a root) never render — every | ||
| // renderer enumerates the parent's `children` array — so no system will | ||
| // ever build them or clear their mark. They must not hold scene-ready | ||
| // hostage (observed in prod scene data: two dangling windows kept every | ||
| // bake of that scene waiting out the full readiness cap). | ||
| const parent = node.parentId ? nodes[node.parentId as AnyNodeId] : undefined | ||
| const reachable = parent | ||
| ? (parent as { children?: string[] }).children?.includes(id) === true | ||
| : rootNodeIds.includes(id) | ||
| if (!reachable) continue | ||
| const def = nodeRegistry.get(node.type) | ||
| if (def?.geometry || def?.capabilities?.floorPlaced || DIRTY_BUILD_KINDS.has(node.type)) { | ||
| return true | ||
|
|
@@ -272,13 +283,16 @@ function hasCommittedSceneRoot() { | |
| function SceneReadyTracker({ | ||
| onSceneReadyChange, | ||
| sceneReadyKey, | ||
| sceneReadyMaxWaitMs, | ||
| }: { | ||
| onSceneReadyChange?: (ready: boolean) => void | ||
| sceneReadyKey?: string | number | null | ||
| sceneReadyMaxWaitMs?: number | ||
| }) { | ||
| const readyRef = useRef(false) | ||
| const settledFramesRef = useRef(0) | ||
| const waitedFramesRef = useRef(0) | ||
| const waitStartRef = useRef<number | null>(null) | ||
| const onSceneReadyChangeRef = useRef(onSceneReadyChange) | ||
|
|
||
| useEffect(() => { | ||
|
|
@@ -290,17 +304,24 @@ function SceneReadyTracker({ | |
| readyRef.current = false | ||
| settledFramesRef.current = 0 | ||
| waitedFramesRef.current = 0 | ||
| waitStartRef.current = null | ||
| onSceneReadyChangeRef.current?.(false) | ||
| }, [sceneReadyKey]) | ||
|
|
||
| useFrame(() => { | ||
| if (!(onSceneReadyChangeRef.current && !readyRef.current)) return | ||
|
|
||
| waitedFramesRef.current += 1 | ||
| if ( | ||
| waitedFramesRef.current < SCENE_READY_MAX_WAIT_FRAMES && | ||
| (!hasCommittedSceneRoot() || hasPendingSceneBuildWork()) | ||
| ) { | ||
| waitStartRef.current ??= performance.now() | ||
| // Give-up cap so a permanently-dirty node can't block readiness forever. | ||
| // The frame-count default assumes display-rate frames; a host whose frame | ||
| // cadence is decoupled from wall time (the headless bake page's timer-driven | ||
| // loop runs 180 frames in 3.6s — faster than a cold item download) passes | ||
| // `sceneReadyMaxWaitMs` to make the cap wall-clock instead. | ||
| const capReached = sceneReadyMaxWaitMs | ||
| ? performance.now() - waitStartRef.current >= sceneReadyMaxWaitMs | ||
| : waitedFramesRef.current >= SCENE_READY_MAX_WAIT_FRAMES | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Zero max wait ignoredLow Severity
Reviewed by Cursor Bugbot for commit 00459d0. Configure here. |
||
| if (!capReached && (!hasCommittedSceneRoot() || hasPendingSceneBuildWork())) { | ||
| settledFramesRef.current = 0 | ||
| return | ||
| } | ||
|
|
@@ -345,6 +366,13 @@ interface ViewerProps { | |
| */ | ||
| sceneReadyKey?: string | number | null | ||
| onSceneReadyChange?: (ready: boolean) => void | ||
| /** | ||
| * Wall-clock give-up cap for scene readiness, replacing the default | ||
| * frame-count cap. Set it on hosts whose frame cadence is decoupled from | ||
| * real time (the headless bake page's timer-driven loop runs the default | ||
| * 180-frame cap in ~3.6s — shorter than a cold item-model download). | ||
| */ | ||
| sceneReadyMaxWaitMs?: number | ||
| /** | ||
| * Skip the TSL post-processing pipeline (SSGI/denoise/ink/outline) and render | ||
| * the scene directly. For headless/capture surfaces (the bake page) where | ||
|
|
@@ -379,6 +407,7 @@ const Viewer = forwardRef<ViewerHandle, ViewerProps>(function Viewer( | |
| isolate, | ||
| sceneReadyKey, | ||
| onSceneReadyChange, | ||
| sceneReadyMaxWaitMs, | ||
| disablePostFx = false, | ||
| }, | ||
| ref, | ||
|
|
@@ -527,7 +556,11 @@ const Viewer = forwardRef<ViewerHandle, ViewerProps>(function Viewer( | |
| <ViewerCamera /> | ||
| <GPUDeviceWatcher /> | ||
| <ToneMappingExposure /> | ||
| <SceneReadyTracker onSceneReadyChange={onSceneReadyChange} sceneReadyKey={sceneReadyKey} /> | ||
| <SceneReadyTracker | ||
| onSceneReadyChange={onSceneReadyChange} | ||
| sceneReadyKey={sceneReadyKey} | ||
| sceneReadyMaxWaitMs={sceneReadyMaxWaitMs} | ||
| /> | ||
|
|
||
| <ErrorBoundary fallback={null} scope="viewer-scene"> | ||
| {/* <directionalLight position={[10, 10, 5]} intensity={0.5} castShadow | ||
|
|
||


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.
Roots ignored for reachability
Medium Severity
hasPendingSceneBuildWorktreats a dirty node as unreachable whenever itsparentIdresolves but that parent’schildrenomits the id, without also checkingrootNodeIds.SceneRendererstill mounts ids listed at the root, so pending build work on those nodes can be ignored andonSceneReadyChange(true)may fire before geometry finishes.Reviewed by Cursor Bugbot for commit 00459d0. Configure here.