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(web): accept function as gl that returns a renderer #1717

Merged
merged 3 commits into from Oct 12, 2021

Conversation

Tirzono
Copy link
Contributor

@Tirzono Tirzono commented Oct 6, 2021

This allows passing in another renderer without having to create a canvas element yourself:

<Canvas
  gl={(canvas) => new WebGPURenderer({ canvas })}
>

</Canvas>

I do need some advice on how to JavaScript -> TypeScript this bit of code :)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 6, 2021

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 823e5e5:

Sandbox Source
example Configuration

@Tirzono
Copy link
Contributor Author

Tirzono commented Oct 7, 2021

@CodyJasonBennett I am not sure how this would work together with your change in #1714

@CodyJasonBennett
Copy link
Member

That is funny timing, but I don't see anything problematic between the two. I'd merge them like this:

const createRendererInstance = <TElement extends Element>(
  gl:
    | THREE.WebGLRenderer
    | ((canvas: TElement) => THREE.WebGLRenderer)
    | Partial<Properties<THREE.WebGLRenderer> | THREE.WebGLRendererParameters>
    | undefined,
  canvas: TElement,
): THREE.WebGLRenderer => {
  const customRenderer = (typeof gl === 'function' ? gl(canvas) : gl) as THREE.WebGLRenderer
  if (isRenderer(customRenderer)) return customRenderer

  const renderer = new THREE.WebGLRenderer({
    powerPreference: 'high-performance',
    canvas: canvas as unknown as HTMLCanvasElement,
    antialias: true,
    alpha: true,
    ...gl,
  })
  if (gl) applyProps(renderer as any, gl as any)

  return renderer
}

@drcmda
Copy link
Member

drcmda commented Oct 7, 2021

good idea! is this ready to merge?

@Tirzono
Copy link
Contributor Author

Tirzono commented Oct 7, 2021

If you guys are happy with my code then it's ready to merge. @CodyJasonBennett are you happy to rebase your branch on this and apply your changes in your branch or would you like me to apply your changes to my branch?

@joshuaellis
Copy link
Member

If possible it'd be better to have one PR that does both. Maybe @CodyJasonBennett you can move your changes over incl tests?

packages/fiber/src/web/index.tsx Outdated Show resolved Hide resolved
@CodyJasonBennett
Copy link
Member

CodyJasonBennett commented Oct 7, 2021

@joshuaellis, sure thing. My only concern would be how to handle asynchronous callbacks and whether that should be our responsibility. Currently, R3F's render method is entirely synchronous.

Taking the WebGPU example, users must asynchronously call WebGPURenderer#init before being able to render.

This is how it would be done in userland:

const [frameloop, setFrameLoop] = React.useState('never')

return (
  <Canvas
    frameloop={frameloop}
    gl={(canvas) => {
      const renderer = new WebGPURenderer({ canvas })
      renderer.init().then(() => setFrameLoop('always'))

      return renderer
    }}>
    <gridHelper />
  </Canvas>
)

@joshuaellis
Copy link
Member

I'll take a look at how to handle that now, shouldn't be an issue though.

@joshuaellis
Copy link
Member

update before I park it for the evening. I'm overloading the functions, I just need typescript to figure out if there is a GL prop added to the render function then that's the Renderer assuming it's not an object belonging to props of THREE.WebGLRenderer it's almost there, I just don't want to introduce breaking changes for Typescript users.

@joshuaellis joshuaellis closed this Oct 7, 2021
@joshuaellis joshuaellis reopened this Oct 7, 2021
@joshuaellis
Copy link
Member

Didn't mean to close.

@joshuaellis joshuaellis self-assigned this Oct 9, 2021
@CodyJasonBennett CodyJasonBennett linked an issue Oct 9, 2021 that may be closed by this pull request
@Tirzono
Copy link
Contributor Author

Tirzono commented Oct 12, 2021

Hi @joshuaelli / @CodyJasonBennett,

Is there anything I can help with to wrap up the PR? Happy to help!

@joshuaellis
Copy link
Member

So I think we're going to move this into v8 as part of our proposal in #1725. But as @CodyJasonBennett pointed out, this needs to be able to be an async function. So it then becomes more complicated because we need to overload functions. This is still on my plate, but I wouldn't expect it to be released anytime soon.

If you desperately need it, you could use patch-package in your repo 😄

@Tirzono
Copy link
Contributor Author

Tirzono commented Oct 12, 2021

No problem at all, completely understand it. Looking forward to v8. In the meantime I'll patch-package it in my repo.

Thanks for all your help. The Poimandres open source developer collective is amazing ❤️

@drcmda
Copy link
Member

drcmda commented Oct 12, 2021

No problem at all, completely understand it. Looking forward to v8. In the meantime I'll patch-package it in my repo.

Thanks for all your help. The Poimandres open source developer collective is amazing ❤️

you dont need patch package, you can outright build your own canvas using the render api. https://github.com/pmndrs/react-three-fiber/blob/master/packages/fiber/src/web/Canvas.tsx

everything in there is exported. i think the only change is that createPointerEvents is just events. but you can just copy that file, add the gl logic.

@drcmda
Copy link
Member

drcmda commented Oct 12, 2021

@joshuaellis, sure thing. My only concern would be how to handle asynchronous callbacks and whether that should be our responsibility. Currently, R3F's render method is entirely synchronous.

Taking the WebGPU example, users must asynchronously call WebGPURenderer#init before being able to render.

This is how it would be done in userland:

const [frameloop, setFrameLoop] = React.useState('never')

return (
  <Canvas
    frameloop={frameloop}
    gl={(canvas) => {
      const renderer = new WebGPURenderer({ canvas })
      renderer.init().then(() => setFrameLoop('always'))

      return renderer
    }}>
    <gridHelper />
  </Canvas>
)

what happens if gl.render is called before init has completed?

@joshuaellis
Copy link
Member

what happens if gl.render is called before init has completed?

this is where it would have to await gl function. This would turn render into an async function. I thought it would be easier to see the change in code before reviewing?

@drcmda
Copy link
Member

drcmda commented Oct 12, 2021

looking through the webgpu code it looks like render itself isnt async, just init. i think it would already be solveable in userland:

<Canvas gl={(canvas) => new WebGPURenderer({ canvas })} frameloop="never" onCreated={async (state) => {
  await state.gl.init()
  state.set({ frameloop: 'always' })
}>

to me that's perhaps even cleaner than adding some async stuff to something as simple as the gl prop.

ps. this needs the PR of course due to the (canvas) => ... thing

@joshuaellis
Copy link
Member

If we can avoid async behaviour in the gl prop it's definitely simpler (and preferred for me imo). @CodyJasonBennett can you confirm the above? I've not used WebGPURenderer.

@drcmda
Copy link
Member

drcmda commented Oct 12, 2021

@joshuaellis @CodyJasonBennett @Tirzono if this works i think let's merge. it is a feature, but so small and helpful, i could just cherry-pick it into v8 later on.

@CodyJasonBennett
Copy link
Member

CodyJasonBennett commented Oct 12, 2021

@drcmda, @joshuaellis, both snippets work with the current implementation.

If you call WebGPURenderer#render before it inits, it will crash. So we turn the frameloop off/on asynchronously to prevent that.

@Tirzono
Copy link
Contributor Author

Tirzono commented Oct 12, 2021

While I gave the example of WebGPURenderer in my first post, I am actually using this for the CanvasRenderer three.js had back in the days (because browsers only allow a maximum number of WebGL contexts on a webpage and I need more than that), so unfortunately I can't tell if it works for the WebGPURenderer (also because there are other reasons why WebGPURenderer is not working, related to mrdoob/three.js#22549). But for my CanvasRenderer in plain JS, this implementation works.

@drcmda
Copy link
Member

drcmda commented Oct 12, 2021

@Tirzono could you fix the merge conflict? @CodyJasonBennett says it's working so async is not a blocker.

@CodyJasonBennett
Copy link
Member

CodyJasonBennett commented Oct 12, 2021

I went ahead and fixed the conflict. Any accompanying documentation on here or the website repo would need to note that gl callbacks must be synchronous.

@drcmda drcmda merged commit 13f37ff into pmndrs:master Oct 12, 2021
@drcmda
Copy link
Member

drcmda commented Oct 12, 2021

the docs in this repo are out of date imo. im not sure if the website/docs site can already use remote mdx, that would be optimal. if not it needs to be documented over there.

@CodyJasonBennett
Copy link
Member

I'll make a note there. I've fixed a bunch of stuff for various repos' docs that weren't updated upstream, but we can start with R3F since stuff shouldn't change until v8.

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.

gl should also accept/apply renderer properties
4 participants