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

feat: ScreenSpace #1131

Merged
merged 6 commits into from Nov 27, 2022
Merged

Conversation

antokhio
Copy link
Contributor

@antokhio antokhio commented Nov 2, 2022

Why

New abstraction that removes camera position and rotation to children.
Gives ability to have 3d objects in the HUD of the camera.

What

Added /src/core/ScreenSpace.tsx
Added info to readme.md
Added to src/core/index.ts
Added .storybook/stories/ScreenSpace.stories.tsx

Checklist

  • Documentation updated
  • Storybook entry added
  • Ready to be merged

@vercel
Copy link

vercel bot commented Nov 2, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
drei ✅ Ready (Inspect) Visit Preview Nov 2, 2022 at 11:58AM (UTC)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 2, 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 34c99a3:

Sandbox Source
great-ioana-g7f0tz Configuration
Ground reflections and video textures Configuration
arc-x-pmndrs-colors Configuration

@CodyJasonBennett CodyJasonBennett changed the title Feat/screen space feat: ScreenSpace Nov 2, 2022
src/core/ScreenSpace.tsx Outdated Show resolved Hide resolved
Comment on lines +10 to +22
export const ScreenSpace = React.forwardRef<Group, ScreenSpaceProps>(({ children, depth = -1, ...rest }, ref) => {
const localRef = React.useRef<Group>(null!)

useFrame(({ camera }) => {
localRef.current.quaternion.copy(camera.quaternion)
localRef.current.position.copy(camera.position)
})
return (
<group ref={mergeRefs([ref, localRef])} {...rest}>
<group position-z={-depth}>{children}</group>
</group>
)
})
Copy link
Member

Choose a reason for hiding this comment

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

Do note that these camera transforms are in local space, so this can break if the camera is parented in some way. Alternatively, you can portal children into the camera and keep a single group for depth:

import { createPortal, useThree } from '@react-three/fiber'

export const ScreenSpace = React.forwardRef<THREE.Group, ScreenSpaceProps>(function ScreenSpace(
  { children, depth = -1, ...rest },
  ref,
) {
  const camera = useThree((state) => state.camera)

  return (
    // Older versions of R3F require portals to be wrapped in fragments to correctly return a JSX.Element
    <>
      {createPortal(
        <group {...rest} ref={ref} position-z={-depth}>
          {children}
        </group>,
        camera,
      )}
    </>
  )

ofc, this would be equivalent to:

import { PerspectiveCamera } from '@react-three/drei'

<PerspectiveCamera makeDefault>
  <group position-z={...}>
    // ...
  </group>
</PerspectiveCamera>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, i need to test both options, but can do that next week prolly

The point of using:

localRef.current.quaternion.copy(camera.quaternion)
localRef.current.position.copy(camera.position)

Was that it supports the Html tag.
Also to take a note there is this approach witch is kind`a resonable:
https://discourse.threejs.org/t/how-to-build-a-hud-in-a-single-scene-with-a-single-camera/16108/2

camera.add(yourobject3d);

@drcmda
Copy link
Member

drcmda commented Nov 2, 2022

would it make sense to extend billboard for this? <Billboard screen> or something like that? asking because billboard does almost the same, except for the position: https://github.com/pmndrs/drei/blob/master/src/core/Billboard.tsx

also, depth, what does this do? is this not the same as position.z?

@antokhio
Copy link
Contributor Author

antokhio commented Nov 3, 2022

would it make sense to extend billboard for this? <Billboard screen> or something like that? asking because billboard does almost the same, except for the position: https://github.com/pmndrs/drei/blob/master/src/core/Billboard.tsx

also, depth, what does this do? is this not the same as position.z?

The depth is actually -position.z in this case it’s distance from camera…

@drcmda
Copy link
Member

drcmda commented Nov 19, 2022

in this case @antokhio @CodyJasonBennett good to merge?

@antokhio
Copy link
Contributor Author

Hi, there was no particular decision in the end, because there are many other ways to achieve same functionality, currently it’s needs a decision feedback if this type of exposed functionality would be good for library and then run few tests on other approaches…

Sorry it’s prolly my bad since I promised to test them… In the end prolly merge is to early or merge and fix after.

p.s. will run other options tests this week and let you know

@antokhio
Copy link
Contributor Author

antokhio commented Nov 27, 2022

@drcmda @CodyJasonBennett just came across issues on stackoverflow where this tech would be applicable, (screen space stencil), I think should merge, improve after by request.

@CodyJasonBennett CodyJasonBennett merged commit 6b07f8e into pmndrs:master Nov 27, 2022
@github-actions
Copy link

🎉 This PR is included in version 9.45.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants