Skip to content
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

[v9] experiment: rework reconciler architecture, create objects on commit #2378

Closed
wants to merge 35 commits into from

Conversation

CodyJasonBennett
Copy link
Member

@CodyJasonBennett CodyJasonBennett commented Jul 21, 2022

Fixes #2250 and a host of other issues when mutating a three.js object as an internal instance by separating the two and delegating three.js object creation until commit mount. This mitigates side-effects from creating objects and mutating them since the tree is now finalized and connected via instance descriptors.

An instance now looks like this, with object being the underlying three.js object and object.__r3f referring to its respective Instance:

interface Instance {
  root: UseBoundStore<RootState>
  type: string
  parent: Instance | null
  children: Instance[]
  props: InstanceProps
  object: any | null
  eventCount: number
  handlers: Partial<EventHandlers>
  attach?: AttachType
  previousAttach?: any
}

Notably, objects was renamed children and is used for all instances, regardless of the use of attach. memoizedProps is now props and indiscriminately contains elements' props.

Note: I'm keeping this as a draft until I clarify the behavior of commitMount and container instances with react. #2268 should have been sufficient to fix #2250, but useLayoutEffect fires before container instances are committed. This affects portals in this PR and is the reason behind the usage of <primitive object={scene} /> in root#render and the omission of container reconciler hooks.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 21, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5b3a237:

Sandbox Source
example Configuration
append-bug Issue #2250

@CodyJasonBennett CodyJasonBennett changed the base branch from master to v9 July 21, 2022 15:54
@CodyJasonBennett CodyJasonBennett changed the base branch from v9 to master July 21, 2022 15:55
@CodyJasonBennett CodyJasonBennett changed the title experiment: separate instance & object, create objects on commit [v9] experiment: separate instance & object, create objects on commit Jul 21, 2022
@CodyJasonBennett CodyJasonBennett changed the title [v9] experiment: separate instance & object, create objects on commit [v9] experiment: rework instance architecture, create objects on commit Jul 21, 2022
@CodyJasonBennett CodyJasonBennett changed the title [v9] experiment: rework instance architecture, create objects on commit [v9] experiment: rework reconciler architecture, create objects on commit Jul 21, 2022
@CodyJasonBennett CodyJasonBennett changed the base branch from master to v9 July 25, 2022 17:40
Comment on lines 113 to 128
<Farm scale={10} rotation={[0, 0, 0]} position={[-1, -2, -10]} />
<Soda scale={5} position={[2, -2, -1.5]} />
<Suspense fallback={null}>
<Farm scale={10} rotation={[0, 0, 0]} position={[-1, -2, -10]} />
<Soda scale={5} position={[2, -2, -1.5]} />
</Suspense>
<Portal scale={[4, 5, 1]} position={[2, 0, -5]} rotation={[0, 0, 0]}>
<Lights />
<Soda scale={8} position={[0, -2, -1.5]} />
<Environment preset="city" background="only" />
<Suspense fallback={null}>
<Soda scale={8} position={[0, -2, -1.5]} />
<Environment preset="city" background="only" />
</Suspense>
</Portal>
</Portal>
<Ramen scale={4} position={[-2, 0, 2]} />
<Soda scale={5} position={[1.5, 0, 3]} />
<Suspense fallback={null}>
<Ramen scale={4} position={[-2, 0, 2]} />
<Soda scale={5} position={[1.5, 0, 3]} />
</Suspense>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR causes ULE in Portal to fire without a ref attached outside of createPortal.

Copy link
Member Author

@CodyJasonBennett CodyJasonBennett Aug 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like reconciler hook timings are really off for elements coming off of suspend as well.

image

import * as React from 'react'
import * as THREE from 'three'
import { suspend } from 'suspend-react'
import { createPortal, Canvas } from '@react-three/fiber'

function Test() {
  suspend(async () => null, [])
  const group = React.useRef<THREE.Group>(null!)
  React.useEffect(() => console.log('Test useEffect', group.current?.uuid), [])
  React.useLayoutEffect(() => console.log('Test useLayoutEffect', group.current?.uuid), [])
  return <group name="Test" ref={group} />
}

function Portal({ children }: { children?: React.ReactNode }) {
  const group = React.useRef<THREE.Group>(null!)
  const [scene] = React.useState(() => new THREE.Scene())
  React.useEffect(() => void console.log('Portal useEffect', group.current?.uuid), [])
  React.useLayoutEffect(() => void console.log('Portal useLayoutEffect', group.current?.uuid), [])
  return (
    <group name="Portal" ref={group}>
      {children && createPortal(children, scene)}
    </group>
  )
}

export default () => (
  <Canvas>
    <Portal>
      <React.Suspense>
        <Test />
      </React.Suspense>
    </Portal>
  </Canvas>
)

@CodyJasonBennett
Copy link
Member Author

Continued in #2465, dropping architecture changes.

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.

Suspense causes attach to be called multiple times
1 participant