brianzinn / react-babylonjs

React for Babylon 3D engine
https://brianzinn.github.io/react-babylonjs/
818 stars 105 forks source link

Gizmo looses relation to activeCamera - Lifecycle #179

Closed dennemark closed 2 years ago

dennemark commented 2 years ago

Hi,

I tried to play with the Gizmo components. Very nice addition! I am currently facing an issue though, where a gizmo component seems to loose information on the scene and active camera. I am wondering if it is related to the Gizmo Lifecycle.

Since the issue is related to the Tunnel storybook, I would not mind if we do not manage to resolve the issue. But it maybe @brianzinn you know the reason for the issue.

The Issue:

I get the following error: Cannot read properties of null (reading 'globalPosition') at Position Gizmo

This points to the babylon gizmo.ts line var cameraPosition = activeCamera.globalPosition; https://github.com/BabylonJS/Babylon.js/blob/dea37daa9a3e63f151975d0e4094b0dfa8dd7229/src/Gizmos/gizmo.ts#L218

Reproduction

To reproduce the issue, one can adapt the tunnel storybook by exchanging the ZustandTunnel component with the attached code. Run the storybook and press the Set Positions button. The rendering process should stop and throw the above error within development console. It seems the positionGizmo cannot handle the position state update well, even though it does not use the prop. If you comment out that line, everything works as expected.

// Exchange everything below the WithTunnel component with the following code:

const MyTunnel = ({position} = props) => {
  const ref = useRef(null)
  useEffect(() => {
    if (ref.current) {
     ref.current.position.x = Math.abs(position - 4);
    }
  }, [position])
   return <TunnelEntrance uid="hyperloop">

      <utilityLayerRenderer>
        <box name="box" size={2} position={Vector3.Zero()}>
            <standardMaterial name="mat" diffuseColor={Color3.Black()} specularColor={Color3.Black()} />
            <positionGizmo/> {/* comment out this line and everything works as expected without gizmo */}
        </box>
        </utilityLayerRenderer>
        <box name="box" ref={ref} position={new Vector3(3, position, 0)}>
          <standardMaterial name="matblue" diffuseColor={Color3.Blue()} specularColor={Color3.Black()} />
        </box>
      </TunnelEntrance>

}

export const ZustandTunnel = () => {

  /** ref to tunnel is possible */

  const [position, setPosition] = useState(1)

  return <>
    <div style={{ flex: 1, display: 'flex' }}>
      <button onClick={() => setPosition(Math.abs(position - 1))}>Set Position</button>
    </div>
    <div style={{ flex: 1, display: 'flex' }}>
      {/** Multiple Tunnel Entrances */}
      <MyTunnel position={position} />

      <TunnelEntrance uid="subway">
        <box name="box" position={new Vector3(position, 0, 1)}>
          <standardMaterial name="mat" diffuseColor={Color3.Red()} specularColor={Color3.Black()} />
        </box>
      </TunnelEntrance>
      <Engine antialias adaptToDeviceRatio canvasId='babylonJS'>
        <Scene>
          <freeCamera name='camera1' position={new Vector3(0, 5, -10)}
            setTarget={[Vector3.Zero()]} />
          <WithTunnel uids={["subway"]} />
          <transformNode name="node" position={new Vector3(5, 0, 0)}>
            <WithTunnel />
          </transformNode>
          <hemisphericLight name='light1' intensity={0.7} direction={Vector3.Up()} />
        </Scene>
      </Engine>
    </div>
  </>
}

Possible Solution

Is the Gizmo Lifecycle missing an unmount function? It might be the case, that the Zustand tunnel remounts the component on every state change? That might not be very efficient and leads to some side effects for Tunneling.

brianzinn commented 2 years ago

I think it may just be because you need to declare inside of the Scene component. If you track the constructor it ends up here: https://github.com/BabylonJS/Babylon.js/blob/master/src/Rendering/utilityLayerRenderer.ts#L78

I'll try to give it a go tonight - otherwise if you moved those below <Engine..>...</Engine> may help there as there would be a LastCreatedScene?

dennemark commented 2 years ago

For me the tunneling is still a bit confusing and that is a bit unfortunate. So far I was thinking, that zustand will place the components within TunnelExit that is living within the Engine/Scene. So it seems there is a bit more going on, before the components reach the TunnelExit? Otherwise I guess they should be able to consume contexts properly.

I would like to avoid this Tunneling, but if it would work, it is pretty convenient to work with. I have a 2d gui that is interacting with the 3d viewer. Instead of updating data in a store via the 2d gui, reading from it within 3d gui and therefore placing two components in two different parts of the react tree, I can just handle all these things in one spot. Would you know by any chance a good idea for this?

brianzinn commented 2 years ago

The best way is probably like you are doing, since the context boundary is lost when changing renderers. I actually wouldn't mind bringing in Zustand (or just dropping Providers) - it makes sense to bring in a library like that which brings in more benefits. Lots of people get caught up on how the state doesn't cross the boundary - looking at the new React.Cache doesn't look like it will land in React 18.

I am working on the grid first issue and then will start on this one, but it looks to me like the ordering of declared components is coming into play here. It would be nice to have a more reactive model that triggers a re-render on your tunnel when the scene is ready (there is a callback for that on Scene, although typically it is always ready on init).

brianzinn commented 2 years ago

Closing for housekeeping purposes - I am definitely open to switching over to Zustand everything, but it looks like you have a workaround. I will accept a PR with breaking change over to state management instead of Context Provider.