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

Create Tunnel #168

Merged
merged 12 commits into from
Nov 8, 2021
Merged

Create Tunnel #168

merged 12 commits into from
Nov 8, 2021

Conversation

dennemark
Copy link
Contributor

hi @brianzinn,

I figured out a way to pass components from one renderer to another while keeping context and refs.
Since portal stay within renderer and bridges only pass context hierarchically into other renderers, i justed named it tunnel.

I added some comments to the component:

A tunnel allows to render components of one renderer inside another.
I.e. babylonjs components normally need to live within Engine component.
A tunnel entrance allows to position components in a different renderer, such as ReactDOM
and move it to the tunnel exit, that must exist within Engine component.

The nice thing is, even refs can be used outside of Engine context.

The createTunnel function creates a tunnel entrance and exit component.
The tunnel works one-directional.
TunnelEntrance only accepts components that are allowed to live within the renderer of TunnelExit.
Multiple entrances and exits are possible.

If components need to be rendererd the other way around, a second Tunnel is needed.

I am not sure, how stable it is.
Besides the storybook, which sends bjs components from outside engine into engine, i also tried to send reactdom data from within bjs outside of it. it connects to the context of the renderer it is send to. and refs seem to work, too.
i used zustand to make this work. also tried valtio, but had issues with rerendering and keeping refs. both state libraries can live outside of react, but trigger state changes. therefore they seemed ideal to send data from a to b. hope it will be stable..

mixedrenderers

brianzinn and others added 8 commits October 24, 2021 11:05
…r is likely caused by a bug in React. Please file an issue."
…stance<T>` - seems like createPortal is adding to root container. Something wonky going on here. I may need to attach everything to the babylon object (and return directly as public instance) and that has some odd side-effects for swapping children/fibers etc.
…er.ts and in storybook. portal is now outside of transformnode -> box is moved from outside into node
… 0 on first render, led to vec being [NaN, NaN], therefore just added an isNaN check.
@dennemark dennemark closed this Oct 27, 2021
@dennemark dennemark deleted the create-portal branch October 27, 2021 13:15
@dennemark dennemark restored the create-portal branch October 27, 2021 13:16
@dennemark
Copy link
Contributor Author

dennemark commented Oct 27, 2021

deleted the wrong branch...woops - reopening

and there is still an issue with removing elements from array. I need to have unique identifiers within the store to know which element to add and remove.

@dennemark dennemark reopened this Oct 27, 2021
@dennemark dennemark mentioned this pull request Oct 27, 2021
@brianzinn
Copy link
Owner

Thanks @dennemark for this great PR!! I am hesitant to add zustand as a dependency for a single component. If I were to replace the current hooks with a better state management that didn't suffer from current issues with reconciler context boundaries then for sure would bring in a library like zustand.

In the meantime I would like to move zustand to storybook and add this as an integration story, but certainly really like the idea. Just trying to keep bundle size down unless it's part of core functionality. Besides reconciler there are currently no dependencies outside babylonjs and the upcoming v4 will remove @babylonjs/gui and replace with registration system for tree-shaking.

Do you mind if I accept and then move to storybook for now? Keen to have a discussion to move the hooks like useScene, but just working out multi-scene maybe with optional keys?

@dennemark
Copy link
Contributor Author

@brianzinn this makes sense! Sure it is possible to move it all to storybook! Actually the tunnel component might be useful for other libraries, too, like r3f, pixi, kanva. As far as I could see, they face similar issues with renderers. So it might be useful to use this as a separate npm. Would be my first npm package, lets see.

We could see how much the other hooks would profit from a different state library like zustand. If we decide to add it as a new dependency, we could improve the html component, too. I did it in my project today and now I can style my components with context of the renderer surrounding react-babylonjs. Its pretty convenient now. Hope no sideeffects will happen, i.e. too inefficient rerendering of children.

@brianzinn
Copy link
Owner

I think zustand selector prevents unnecessary rerenders:

const someValue = useStore(state => state.someValue);

@brianzinn
Copy link
Owner

if it's OK with you I will merge this PR next week and move some pieces to storybook. it's a really good recipe 😄

@dennemark
Copy link
Contributor Author

sure sounds good!
concerning zustand selectors, maybe the tunnel can be even improved by using useCallback. but thats for some future pr ;)

@brianzinn brianzinn merged commit 059968b into brianzinn:master Nov 8, 2021
brianzinn added a commit that referenced this pull request Nov 8, 2021
@drcmda
Copy link

drcmda commented Jan 21, 2022

@dennemark @brianzinn i think it can still be simplified without having to rely on id's. i've made a small library on poimandres to test it out, modelled it after React.context: https://github.com/pmndrs/tunnel-rat now tunneling through multiple renderers can still have unforseen side effects, useEffect practically looses all guarantees to catch a ref next, especially with suspense and concurrency. that i think cannot be fixed in userland.

@dennemark
Copy link
Contributor Author

@dennemark @brianzinn i think it can still be simplified without having to rely on id's. i've made a small library on poimandres to test it out, modelled it after React.context: https://github.com/pmndrs/tunnel-rat now tunneling through multiple renderers can still have unforseen side effects, useEffect practically looses all guarantees to catch a ref next, especially with suspense and concurrency. that i think cannot be fixed in userland.

nice! great to see another view on this from a more experienced developer :) good learning material and cleanup!
i am currently using my tunnels even in production - not sure if i tested useLayoutEffect, but seems to make sense! sometimes i had some issues with few contexts or states being lost. so i had to wrap them in a zustand, too. but it mainly worked so far.

i had a version without id´s, too. but its disadvantage is, that one can only have one tunnel entrance and exit. with ids, we can create multiple tunnel exits and entrances from same component. in my case i used one exit within my Scene and multiple entrances outside of it. This way I dont need to maintain exit tunnels. Not sure if there is a better way than using ids, though..

@drcmda
Copy link

drcmda commented Jan 21, 2022

multiple exits are possible,

<dom.Out />
<div>
  <dom.Out />
  <dom.Out />
</div>

multiple ins without GUID perhaps as well but this would seem a little too crazy given that there's no sane order after this, while multiple outs can at least be composed. as for uLE, it fires before the view is committed, i think it could be a good place for it.

@brianzinn
Copy link
Owner

@dennemark - we should bring in Zustand as a dep. can get to that after this current work on docs. i have a few other breaking changes i want to bring in, so good chance for me to do a good cleanup.

@dennemark
Copy link
Contributor Author

dennemark commented Jan 24, 2022

@dennemark - we should bring in Zustand as a dep. can get to that after this current work on docs. i have a few other breaking changes i want to bring in, so good chance for me to do a good cleanup.

you have a better overview, but is it still possible to avoid zustand in react-babylonjs and create a seperate package containing the additional components using it? - maybe we should talk about it in discussion :D

@brianzinn
Copy link
Owner

it is entirely possible to avoid zustand as status quo - would be nice to bring it in though as a way to not have to bridge across renderer boundaries and cleanup the context provider code. i'll bring up a discussion once I get through the big list of todos! 😄

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.

3 participants