From ce9bacf0e7dfdfd5932bfc6c55c676b29286997d Mon Sep 17 00:00:00 2001 From: Cody Bennett <23324155+CodyJasonBennett@users.noreply.github.com> Date: Fri, 2 Sep 2022 07:26:44 -0500 Subject: [PATCH 1/5] experiment: handle attach on container append, de-dup attach on suspense hide/unhide --- packages/fiber/src/core/renderer.ts | 32 ++++++++++++++++++++--------- packages/fiber/src/core/utils.ts | 1 + 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/packages/fiber/src/core/renderer.ts b/packages/fiber/src/core/renderer.ts index d42cf40ad2..c9c172392f 100644 --- a/packages/fiber/src/core/renderer.ts +++ b/packages/fiber/src/core/renderer.ts @@ -41,6 +41,7 @@ export interface Instance { handlers: Partial attach?: AttachType previousAttach?: any + isHidden: boolean } interface HostConfig { @@ -101,9 +102,7 @@ function appendChild(parent: HostConfig['instance'], child: HostConfig['instance child.parent = parent parent.children.push(child) - if (child.props.attach) { - attach(parent, child) - } else if (parent.object instanceof THREE.Object3D && child.object instanceof THREE.Object3D) { + if (!child.props.attach && parent.object instanceof THREE.Object3D && child.object instanceof THREE.Object3D) { parent.object.add(child.object) } @@ -120,9 +119,8 @@ function insertBefore( child.parent = parent parent.children.splice(parent.children.indexOf(beforeChild), 0, child) - if (child.props.attach) { - attach(parent, child) - } else if ( + if ( + !child.props.attach && parent.object instanceof THREE.Object3D && child.object instanceof THREE.Object3D && beforeChild.object instanceof THREE.Object3D @@ -276,6 +274,11 @@ const reconciler = Reconciler< if (!child || !scene) return appendChild(scene, child) + + if (child.props.attach) attach(scene, child) + for (const childInstance of child.children) { + if (childInstance.props.attach) attach(child, childInstance) + } }, removeChildFromContainer(container, child) { const scene = (container.getState().scene as unknown as Instance['object']).__r3f @@ -288,6 +291,11 @@ const reconciler = Reconciler< if (!child || !beforeChild || !scene) return insertBefore(scene, child, beforeChild) + + if (child.props.attach) attach(scene, child) + for (const childInstance of child.children) { + if (childInstance.props.attach) attach(child, childInstance) + } }, getRootHostContext: () => null, getChildHostContext: (parentHostContext) => parentHostContext, @@ -338,15 +346,19 @@ const reconciler = Reconciler< instance.object.visible = false } + instance.isHidden = true invalidateInstance(instance) }, unhideInstance(instance) { - if (instance.props.attach && instance.parent?.object) { - attach(instance.parent, instance) - } else if (instance.object instanceof THREE.Object3D && instance.props.visible !== false) { - instance.object.visible = true + if (instance.isHidden) { + if (instance.props.attach && instance.parent?.object) { + attach(instance.parent, instance) + } else if (instance.object instanceof THREE.Object3D && instance.props.visible !== false) { + instance.object.visible = true + } } + instance.isHidden = false invalidateInstance(instance) }, createTextInstance: handleTextInstance, diff --git a/packages/fiber/src/core/utils.ts b/packages/fiber/src/core/utils.ts index 3012cb7ea3..5539ad4d9b 100644 --- a/packages/fiber/src/core/utils.ts +++ b/packages/fiber/src/core/utils.ts @@ -174,6 +174,7 @@ export function prepare( object, eventCount: 0, handlers: {}, + isHidden: false, } object.__r3f = instance } From 18deabcbb5c11021368f15d83fe3b9400129ea7e Mon Sep 17 00:00:00 2001 From: Cody Bennett <23324155+CodyJasonBennett@users.noreply.github.com> Date: Fri, 2 Sep 2022 07:52:41 -0500 Subject: [PATCH 2/5] fix: recursively attach --- packages/fiber/src/core/renderer.ts | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/packages/fiber/src/core/renderer.ts b/packages/fiber/src/core/renderer.ts index c9c172392f..2614e53c9b 100644 --- a/packages/fiber/src/core/renderer.ts +++ b/packages/fiber/src/core/renderer.ts @@ -206,6 +206,7 @@ function switchInstance( // Move children to new instance for (const child of oldInstance.children) { appendChild(newInstance, child) + if (child.props.attach) attach(newInstance, child) } oldInstance.children = [] @@ -213,6 +214,7 @@ function switchInstance( const parent = oldInstance.parent if (parent) { appendChild(parent, newInstance) + if (newInstance.props.attach) attach(parent, newInstance) removeChild(parent, oldInstance, false) } @@ -240,6 +242,11 @@ function switchInstance( return newInstance } +function attachRecursive(parent: Instance, child: Instance) { + if (child.props.attach) attach(parent, child) + for (const childInstance of child.children) attachRecursive(child, childInstance) +} + // Don't handle text instances, warn on undefined behavior const handleTextInstance = () => console.warn('R3F: Text is not allowed in JSX! This could be stray whitespace or characters.') @@ -274,11 +281,7 @@ const reconciler = Reconciler< if (!child || !scene) return appendChild(scene, child) - - if (child.props.attach) attach(scene, child) - for (const childInstance of child.children) { - if (childInstance.props.attach) attach(child, childInstance) - } + attachRecursive(scene, child) }, removeChildFromContainer(container, child) { const scene = (container.getState().scene as unknown as Instance['object']).__r3f @@ -291,11 +294,7 @@ const reconciler = Reconciler< if (!child || !beforeChild || !scene) return insertBefore(scene, child, beforeChild) - - if (child.props.attach) attach(scene, child) - for (const childInstance of child.children) { - if (childInstance.props.attach) attach(child, childInstance) - } + attachRecursive(scene, child) }, getRootHostContext: () => null, getChildHostContext: (parentHostContext) => parentHostContext, From c216a77792a3af184e796512b9de59f64d4ad97d Mon Sep 17 00:00:00 2001 From: Cody Bennett <23324155+CodyJasonBennett@users.noreply.github.com> Date: Fri, 2 Sep 2022 08:13:36 -0500 Subject: [PATCH 3/5] fix: attach on appendChild when parent is mounted --- packages/fiber/src/core/renderer.ts | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/packages/fiber/src/core/renderer.ts b/packages/fiber/src/core/renderer.ts index 2614e53c9b..ae235a9f91 100644 --- a/packages/fiber/src/core/renderer.ts +++ b/packages/fiber/src/core/renderer.ts @@ -102,7 +102,9 @@ function appendChild(parent: HostConfig['instance'], child: HostConfig['instance child.parent = parent parent.children.push(child) - if (!child.props.attach && parent.object instanceof THREE.Object3D && child.object instanceof THREE.Object3D) { + if (child.props.attach) { + if (parent.parent) attach(parent, child) + } else if (parent.object instanceof THREE.Object3D && child.object instanceof THREE.Object3D) { parent.object.add(child.object) } @@ -119,8 +121,9 @@ function insertBefore( child.parent = parent parent.children.splice(parent.children.indexOf(beforeChild), 0, child) - if ( - !child.props.attach && + if (child.props.attach) { + if (parent.parent) attach(parent, child) + } else if ( parent.object instanceof THREE.Object3D && child.object instanceof THREE.Object3D && beforeChild.object instanceof THREE.Object3D @@ -203,21 +206,19 @@ function switchInstance( // Create a new instance const newInstance = createInstance(type, props, oldInstance.root) - // Move children to new instance - for (const child of oldInstance.children) { - appendChild(newInstance, child) - if (child.props.attach) attach(newInstance, child) - } - oldInstance.children = [] - // Link up new instance const parent = oldInstance.parent if (parent) { appendChild(parent, newInstance) - if (newInstance.props.attach) attach(parent, newInstance) removeChild(parent, oldInstance, false) } + // Move children to new instance + for (const child of oldInstance.children) { + appendChild(newInstance, child) + } + oldInstance.children = [] + // Re-bind event handlers if (newInstance.object.raycast !== null && newInstance.object instanceof THREE.Object3D && newInstance.eventCount) { const rootState = newInstance.root.getState() From f861b32c6cba7539b2d0747c19846276dbb4b56d Mon Sep 17 00:00:00 2001 From: Cody Bennett <23324155+CodyJasonBennett@users.noreply.github.com> Date: Fri, 2 Sep 2022 16:36:14 -0500 Subject: [PATCH 4/5] fix: consolidate container-effects, cleanup --- packages/fiber/src/core/renderer.ts | 79 +++++++++++++++++++---------- 1 file changed, 53 insertions(+), 26 deletions(-) diff --git a/packages/fiber/src/core/renderer.ts b/packages/fiber/src/core/renderer.ts index ae235a9f91..b27e409775 100644 --- a/packages/fiber/src/core/renderer.ts +++ b/packages/fiber/src/core/renderer.ts @@ -68,19 +68,23 @@ function createInstance( props: HostConfig['props'], root: UseBoundStore, ): HostConfig['instance'] { + // Get target from catalogue const name = `${type[0].toUpperCase()}${type.slice(1)}` const target = catalogue[name] + // Validate element target if (type !== 'primitive' && !target) throw new Error( `R3F: ${name} is not part of the THREE namespace! Did you forget to extend? See: https://docs.pmnd.rs/react-three-fiber/api/objects#using-3rd-party-objects-declaratively`, ) + // Validate primitives if (type === 'primitive' && !props.object) throw new Error(`R3F: Primitives without 'object' are invalid!`) // Throw if an object or literal was passed for args if (props.args !== undefined && !Array.isArray(props.args)) throw new Error('R3F: The args prop must be an array!') + // Create instance const object = props.object ?? new target(...(props.args ?? [])) const instance = prepare(object, root, type, props) @@ -96,18 +100,40 @@ function createInstance( return instance } +// https://github.com/facebook/react/issues/20271 +// This will make sure events and attach are only handled once when trees are complete +function handleContainerEffects(parent: Instance, child: Instance) { + // Bail if tree isn't mounted or parent is not a container. + // This ensures that the tree is finalized and React won't discard results to Suspense + const state = child.root.getState() + if (!parent.parent && parent.object !== state.scene) return + + // Handle interactivity + if (child.eventCount > 0 && child.object.raycast !== null && child.object instanceof THREE.Object3D) { + state.internal.interaction.push(child.object) + } + + // Handle attach + if (child.props.attach) attach(parent, child) + for (const childInstance of child.children) handleContainerEffects(child, childInstance) +} + function appendChild(parent: HostConfig['instance'], child: HostConfig['instance'] | HostConfig['textInstance']) { if (!child) return + // Link instances child.parent = parent parent.children.push(child) - if (child.props.attach) { - if (parent.parent) attach(parent, child) - } else if (parent.object instanceof THREE.Object3D && child.object instanceof THREE.Object3D) { + // Add Object3Ds if able + if (!child.props.attach && parent.object instanceof THREE.Object3D && child.object instanceof THREE.Object3D) { parent.object.add(child.object) } + // Attach tree once complete + handleContainerEffects(parent, child) + + // Tree was updated, request a frame invalidateInstance(child) } @@ -118,12 +144,13 @@ function insertBefore( ) { if (!child || !beforeChild) return + // Link instances child.parent = parent parent.children.splice(parent.children.indexOf(beforeChild), 0, child) - if (child.props.attach) { - if (parent.parent) attach(parent, child) - } else if ( + // Manually splice Object3Ds + if ( + !child.props.attach && parent.object instanceof THREE.Object3D && child.object instanceof THREE.Object3D && beforeChild.object instanceof THREE.Object3D @@ -133,12 +160,21 @@ function insertBefore( child.object.dispatchEvent({ type: 'added' }) } + // Attach tree once complete + handleContainerEffects(parent, child) + + // Tree was updated, request a frame invalidateInstance(child) } -function removeRecursive(children: HostConfig['instance'][], parent: HostConfig['instance'], dispose: boolean = false) { +function removeRecursive( + children: HostConfig['instance'][], + parent: HostConfig['instance'], + recursive: boolean = false, + dispose: boolean = false, +) { for (const child of children) { - removeChild(parent, child, dispose) + removeChild(parent, child, recursive, dispose) } } @@ -150,10 +186,12 @@ function removeChild( ) { if (!child) return + // Unlink instances child.parent = null const childIndex = parent.children.indexOf(child) if (childIndex !== -1) parent.children.splice(childIndex, 1) + // Eagerly tear down tree if (child.props.attach) { detach(parent, child) } else if (child.object instanceof THREE.Object3D && parent.object instanceof THREE.Object3D) { @@ -175,7 +213,7 @@ function removeChild( // Remove nested child objects. Primitives should not have objects and children that are // attached to them declaratively ... - if (!isPrimitive && recursive) removeRecursive(child.children, child, shouldDispose) + if (!isPrimitive && recursive) removeRecursive(child.children, child, recursive, shouldDispose) // Unlink instance object delete child.object.__r3f @@ -194,6 +232,7 @@ function removeChild( } } + // Tree was updated, request a frame for top-level instance if (dispose === undefined) invalidateInstance(child) } @@ -209,12 +248,13 @@ function switchInstance( // Link up new instance const parent = oldInstance.parent if (parent) { - appendChild(parent, newInstance) removeChild(parent, oldInstance, false) + appendChild(parent, newInstance) } // Move children to new instance for (const child of oldInstance.children) { + removeChild(oldInstance, child, false, false) appendChild(newInstance, child) } oldInstance.children = [] @@ -238,16 +278,12 @@ function switchInstance( } }) + // Tree was updated, request a frame invalidateInstance(newInstance) return newInstance } -function attachRecursive(parent: Instance, child: Instance) { - if (child.props.attach) attach(parent, child) - for (const childInstance of child.children) attachRecursive(child, childInstance) -} - // Don't handle text instances, warn on undefined behavior const handleTextInstance = () => console.warn('R3F: Text is not allowed in JSX! This could be stray whitespace or characters.') @@ -282,7 +318,6 @@ const reconciler = Reconciler< if (!child || !scene) return appendChild(scene, child) - attachRecursive(scene, child) }, removeChildFromContainer(container, child) { const scene = (container.getState().scene as unknown as Instance['object']).__r3f @@ -295,7 +330,6 @@ const reconciler = Reconciler< if (!child || !beforeChild || !scene) return insertBefore(scene, child, beforeChild) - attachRecursive(scene, child) }, getRootHostContext: () => null, getChildHostContext: (parentHostContext) => parentHostContext, @@ -324,15 +358,8 @@ const reconciler = Reconciler< Object.assign(instance.props, changedProps) applyProps(instance.object, changedProps) }, - // https://github.com/facebook/react/issues/20271 - // This will make sure events are only added once to the central container - finalizeInitialChildren: (instance) => instance.eventCount > 0, - commitMount(instance) { - if (instance.object.raycast !== null && instance.object instanceof THREE.Object3D && instance.eventCount) { - const rootState = instance.root.getState() - rootState.internal.interaction.push(instance.object) - } - }, + finalizeInitialChildren: () => false, + commitMount() {}, getPublicInstance: (instance) => instance?.object!, prepareForCommit: () => null, preparePortalMount: (container) => prepare(container.getState().scene, container, '', {}), From 39763ce35ea9c002f4dbedbb50d3ea30e44bd74c Mon Sep 17 00:00:00 2001 From: Cody Bennett <23324155+CodyJasonBennett@users.noreply.github.com> Date: Fri, 2 Sep 2022 18:00:52 -0500 Subject: [PATCH 5/5] chore(tests): add e2e suspense test case --- packages/fiber/tests/core/renderer.test.tsx | 45 +++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/packages/fiber/tests/core/renderer.test.tsx b/packages/fiber/tests/core/renderer.test.tsx index 9f0983593c..ca85372a73 100644 --- a/packages/fiber/tests/core/renderer.test.tsx +++ b/packages/fiber/tests/core/renderer.test.tsx @@ -788,4 +788,49 @@ describe('renderer', () => { expect(console.warn).toHaveBeenCalled() console.warn = warn }) + + it('should gracefully interrupt when building up the tree', async () => { + const calls: string[] = [] + let lastAttached!: string | undefined + let lastMounted!: string | undefined + + function SuspenseComponent({ reconstruct = false }: { reconstruct?: boolean }) { + suspend(async (reconstruct) => reconstruct, [reconstruct]) + + return ( + + void (lastMounted = self?.uuid)} + attach={(parent, self) => { + calls.push('attach') + lastAttached = self.uuid + return () => calls.push('detach') + }} + /> + + ) + } + + function Test(props: { reconstruct?: boolean }) { + React.useLayoutEffect(() => void calls.push('useLayoutEffect'), []) + + return ( + + + + ) + } + + await act(async () => root.render()) + + // Should complete tree before layout-effects fire + expect(calls).toStrictEqual(['attach', 'useLayoutEffect']) + expect(lastAttached).toBe(lastMounted) + + await act(async () => root.render()) + + expect(calls).toStrictEqual(['attach', 'useLayoutEffect', 'detach', 'attach']) + expect(lastAttached).toBe(lastMounted) + }) })