Closed dennemark closed 2 years ago
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.
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?
@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.
I think zustand selector prevents unnecessary rerenders:
const someValue = useStore(state => state.someValue);
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 😄
sure sounds good! concerning zustand selectors, maybe the tunnel can be even improved by using useCallback. but thats for some future pr ;)
@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 @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..
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.
@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 - 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
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! 😄
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:
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..