Open mattrossman opened 1 year ago
I think we should add a check here if container
is an instanceof THREE.Scene
, otherwise forward the existing state.scene
. This would also remove the need for the as THREE.Scene
type assertion.
a portal can be any object3d technically, and i think it's just a typing or naming issue. useThree(state => state.scene) should point to the root and maybe "root" would have been a better name from the get go but portals are a fairly advanced concept that didn't exist way back when.
I think we should add a check here if container is an instanceof THREE.Scene, otherwise forward the existing state.scene.
it's pretty important to get the root object inside a portal, for after all it's supposed to be a self contained sandbox. the underlying premise still holds, you get to root object which you can use to render out or do something else with it. if that points to something that is not the portals root that imo would be a bug.
I can appreciate the value in providing access to the root object.
However, some components rely on scene-specific behavior, so access to the root Object3D isn't always sufficient. The useThree
hook already exposes the default THREE.Camera
and THREE.WebGLRenderer
managed by R3F so that users can write components that access these objects regardless of where they are mounted, it seems reasonable to expose a stable THREE.Scene
property as well.
For example, the <Environment />
component from drei uses state.scene
to change the background
and envMap
properties. I have also written a similar <BackgroundColor />
component in the past relying on state.scene.background
which does not exist on the broader Object3D
type.
Another use-case (the one that inspired this issue) is that I have a dynamically parented object which contains a THREE.SkeletonHelper
. SkeletonHelpers are supposed to appear directly under the scene as they are affected by parent transforms, so I expect to be able to say createPortal(<skeletonHelper ... />, state.scene)
. Instead I have manually pass the outer state.scene
down to my component or traverse up the parent chain to find the nearest scene.
Perhaps it is worth exposing both the root
and scene
which may or may not be the same reference.
Here is a utility I am using as a workaround:
/**
* Get the nearest THREE.Scene ancestor, or null
*/
function useScene() {
const { scene: root } = useThree()
const scene = useMemo(() => {
let object: THREE.Object3D | null = root
while (object) {
if (object instanceof THREE.Scene) return object
object = object.parent
}
return null
}, [root])
return scene
}
There's two things that createPortal
does that can make this tricky:
scene
property with an Object3D
scene
property for top-level nodes of a root or portal to inform where to attach nodesIf we separate this behavior, it allows for two things:
createPortal(<element />, Object3D, { scene: Scene })
for the desired behavior (<element />
attaches to Object3D
, Scene
parents Object3D
, although the default behavior is still the sameScene
which shouldn't change the perceived rendering behavior. This will need to consider cases where the target is either mounted or unmounted to an existing scene (it's valid behavior to have a Scene
within a Scene
)The second can be breaking for those relying on the fact that scene
would be a specific Object3D
, even if less correct, so I'd want to relegate that to v9.
btw @CodyJasonBennett i think that's actually possible right now, the third argument should do exactly what you laid out, it's for custom inject objects.
@mattrossman you can also access scene.__r3f and in there you will find the root and the previous root. btw if portal is the issue it could just use a scene instead of a group, maybe that's the easiest fix.
WRT https://github.com/pmndrs/react-three-fiber/issues/2725#issuecomment-1398597982, I believe this can be addressed by changing that to scene: container instanceof THREE.Scene ? container : new THREE.Scene().add(container)
for the state model, and keeping the actual React portal target container
.
Sandbox: https://codesandbox.io/s/issue-createportal-and-state-scene-pvtlsz?file=/src/App.jsx
Output:
According to the docs and type definitions,
scene
returned fromuseThree
should be aTHREE.Scene
representing the scene. However, when this value is read within a component that has been dynamically parented to another object withcreatePortal
, thescene
value contains the parent object which could be a different type such asTHREE.Group
.I would expect
scene
to always contain a scene so that components can modify scene-specific properties such as.background
. I would only expect thescene
value to change if thecreatePortal
target was a differentTHREE.Scene
.