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.

Sign up for GitHub

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

fix(core): revert #2268 for uLE timings #2316

Merged
merged 2 commits into from
Jun 13, 2022
Merged

fix(core): revert #2268 for uLE timings #2316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0fbcdf9
fix(core): revert #2268 for uLE timings
CodyJasonBennett Jun 13, 2022
86d892e
fix(core): revert attach on reconstruct, appendChild is effectful
CodyJasonBennett Jun 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 15 additions & 21 deletions packages/fiber/src/core/renderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,10 @@ function createRenderer<TCanvas>(roots: Map<TCanvas, Root>, getEventPriority?: (
function appendChild(parentInstance: Instance, child: Instance) {
let added = false
if (child) {
// The attach attribute implies that the object attaches itself on the parent.
// That is handled at commit to avoid duplication during Suspense
if (!child.__r3f?.attach && child.isObject3D && parentInstance.isObject3D) {
// The attach attribute implies that the object attaches itself on the parent
if (child.__r3f?.attach) {
attach(parentInstance, child, child.__r3f.attach)
} else if (child.isObject3D && parentInstance.isObject3D) {
// add in the usual parent-child way
parentInstance.add(child)
added = true
Expand All @@ -137,7 +138,9 @@ function createRenderer<TCanvas>(roots: Map<TCanvas, Root>, getEventPriority?: (
function insertBefore(parentInstance: Instance, child: Instance, beforeChild: Instance) {
let added = false
if (child) {
if (!child.__r3f?.attach && child.isObject3D && parentInstance.isObject3D) {
if (child.__r3f?.attach) {
attach(parentInstance, child, child.__r3f.attach)
} else if (child.isObject3D && parentInstance.isObject3D) {
child.parent = parentInstance as unknown as THREE.Object3D
child.dispatchEvent({ type: 'added' })
const restSiblings = parentInstance.children.filter((sibling) => sibling !== child)
Expand Down Expand Up @@ -234,12 +237,7 @@ function createRenderer<TCanvas>(roots: Map<TCanvas, Root>, getEventPriority?: (
instance.children = []
}

// Copy over child attachments
for (const child of instance.__r3f.objects) {
appendChild(newInstance, child)
detach(instance, child, child.__r3f.attach!)
attach(newInstance, child, child.__r3f.attach!)
}
instance.__r3f.objects.forEach((child) => appendChild(newInstance, child))
instance.__r3f.objects = []

removeChild(parent, instance)
Expand All @@ -251,11 +249,6 @@ function createRenderer<TCanvas>(roots: Map<TCanvas, Root>, getEventPriority?: (
rootState.internal.interaction.push(newInstance as unknown as THREE.Object3D)
}

// Attach instance to parent
if (newInstance.__r3f?.attach) {
attach(parent, newInstance, newInstance.__r3f.attach)
}

// This evil hack switches the react-internal fiber node
// https://github.com/facebook/react/issues/14983
// https://github.com/facebook/react/pull/15021
Expand Down Expand Up @@ -297,7 +290,7 @@ function createRenderer<TCanvas>(roots: Map<TCanvas, Root>, getEventPriority?: (
const localState = (instance?.__r3f ?? {}) as LocalState
// https://github.com/facebook/react/issues/20271
// Returning true will trigger commitMount
return !!localState.handlers || !!localState.attach
return !!localState.handlers
},
prepareUpdate(instance: Instance, type: string, oldProps: any, newProps: any) {
// Create diff-sets
Expand Down Expand Up @@ -341,11 +334,6 @@ function createRenderer<TCanvas>(roots: Map<TCanvas, Root>, getEventPriority?: (
if (instance.raycast && localState.handlers && localState.eventCount) {
instance.__r3f.root.getState().internal.interaction.push(instance as unknown as THREE.Object3D)
}

// The attach attribute implies that the object attaches itself on the parent
if (localState.parent && localState.attach) {
attach(localState.parent, instance, localState.attach)
}
},
getPublicInstance: (instance: Instance) => instance,
shouldDeprioritizeSubtree: () => false,
Expand All @@ -356,10 +344,16 @@ function createRenderer<TCanvas>(roots: Map<TCanvas, Root>, getEventPriority?: (
clearContainer: () => false,
detachDeletedInstance: () => {},
hideInstance(instance: Instance) {
// Deatch while the instance is hidden
const { attach: type, parent } = instance?.__r3f ?? {}
if (type && parent) detach(parent, instance, type)
if (instance.isObject3D) instance.visible = false
invalidateInstance(instance)
},
unhideInstance(instance: Instance, props: InstanceProps) {
// Re-attach when the instance is unhidden
const { attach: type, parent } = instance?.__r3f ?? {}
if (type && parent) attach(parent, instance, type)
if ((instance.isObject3D && props.visible == null) || props.visible) instance.visible = true
invalidateInstance(instance)
},
Expand Down
23 changes: 0 additions & 23 deletions packages/fiber/tests/core/renderer.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,20 +425,10 @@ describe('renderer', () => {
let state: RootState = null!
const instances: { uuid: string; parentUUID?: string; childUUID?: string }[] = []

let lastAttached: Instance = null!
let lastDetached: Instance = null!

const Test = ({ n }: { n: number }) => (
// @ts-ignore args isn't a valid prop but changing it will swap
<group args={[n]} onPointerOver={() => null}>
<group />
<group attach="test" />
<group
attach={(parent) => {
lastAttached = parent
return () => void (lastDetached = parent)
}}
/>
</group>
)

Expand All @@ -452,12 +442,6 @@ describe('renderer', () => {
childUUID: state.scene.children[0].children[0]?.uuid,
})

// Has initial attachments
expect((state.scene.children[0] as any).test).toBeInstanceOf(THREE.Group)
expect(lastAttached).not.toBe(null)
expect(lastAttached.uuid).toBe(state.scene.children[0].uuid)
expect(lastDetached).toBe(null)

await act(async () => {
state = root.render(<Test n={2} />).getState()
})
Expand All @@ -477,13 +461,6 @@ describe('renderer', () => {
expect(oldInstance.parentUUID).toBe(newInstance.parentUUID)
expect(oldInstance.childUUID).toBe(newInstance.childUUID)

// Preserves initial attachments
expect((state.scene.children[0] as any).test).toBeInstanceOf(THREE.Group)
expect(lastAttached).not.toBe(null)
expect(lastAttached.uuid).toBe(newInstance.uuid)
expect(lastDetached).not.toBe(null)
expect(lastDetached.uuid).toBe(oldInstance.uuid)

// Rebinds events
expect(state.internal.interaction.length).not.toBe(0)
})
Expand Down