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: create objects with container effects #2607

Merged
merged 15 commits into from
Jun 22, 2023

Conversation

CodyJasonBennett
Copy link
Member

@CodyJasonBennett CodyJasonBennett commented Nov 2, 2022

Continues #2378, follow-up to #2465.

Defers instance object creation to when the tree is being linked, avoiding duplication of constructor side-effects.

@krispya
Copy link
Member

krispya commented Feb 27, 2023

Studying this some more, am I understanding correctly that this PR would move the instance class construction from the render phase to the commit phase? And in doing so would negate the concurrency benefits of allowing work to be sliced and yielded in the render phase? ie facebook/react#20271 (comment)

Also, why is the object creation and mutation done on appendChild and not commitMount as suggested here? https://github.com/facebook/react/tree/main/packages/react-reconciler#appendchildparentinstance-child

@CodyJasonBennett
Copy link
Member Author

CodyJasonBennett commented Feb 27, 2023

Yes, this intentionally opts out of concurrency and suspense; that is only useful for the virtualized instances rather than their (real) three.js counterparts. We use handleContainerEffects from #2465 over commitMount as this is called for incomplete trees which can lead to duplication (#2250). This should effectively batch changes to the scene-graph.

@krispya
Copy link
Member

krispya commented Feb 28, 2023

I have been testing this PR and it does what it says on the tin. My custom classes have side effects that run only once, even with a series of contrived and nested suspense. However, all children have stopped attaching. Only the top level group of a component attaches and then it has no children!

@CodyJasonBennett
Copy link
Member Author

I'll need to mirror react-ogl. It has the v9 architecture and does this without issue.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 28, 2023

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 a83e06e:

Sandbox Source
example Configuration

@CodyJasonBennett CodyJasonBennett marked this pull request as ready for review February 28, 2023 05:02
@krispya
Copy link
Member

krispya commented Feb 28, 2023

Works beautifully now. I'll be testing with some complicated setups and see if anything breaks. Do you anticipate any problems I should look out for?

@krispya
Copy link
Member

krispya commented Feb 28, 2023

Used to be that adding props to primitive worked, but with this PR it does not. I know it's an odd use case, but one that is used in Drei currently.

const controls = useMemo(
	() => new FlyControlsImpl(camera, explDomElement),
	[camera, explDomElement]
);

return (
	<primitive
		object={controls}
		args={[camera, explDomElement]}
		movementSpeed={movementSpeed}
		rollSpeed={rollSpeed}
		dragToLook={dragToLook}
		{...rest}
	/>
);

@krispya krispya merged commit 66b7cb3 into v9 Jun 22, 2023
@CodyJasonBennett CodyJasonBennett deleted the experiment/object-constructor-effects branch August 17, 2023 09:10
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.

2 participants