brianzinn / react-babylonjs

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

<mesh> element, fromInstance not updating properly #205

Closed DefaultV closed 2 years ago

DefaultV commented 2 years ago

Bug repo: https://github.com/DefaultV/create-react-app-typescript-babylonjs/tree/Bug-MeshFromInstanceChange

Hello! Currently when using the <mesh fromInstance={...}> property, it doesn't seem to dispose of the old mesh when updating the value, nor update the reference to the new mesh. Is this intended behaviour or am I missing something?

  1. In the following repo, I'm firstly creating a box within a useState(box).
  2. Then I pass this to a <mesh fromInstance={box}> component.
  3. Then I replace the state with a new sphere mesh, setState(MeshBuilder.CreateSphere(...)

Following screenshot is the result, I expected the box to disappear on step 3

image

brianzinn commented 2 years ago

that is by design, but certainly I can consider changing that. the fromInstance is equivalent currently to instantiation (or in this case replacement for instantiation), which happens only at time of object creation. the reconciler understands that - have you considered using the key prop to signal to the reconciler that it is a different object. I'm trying to work out if this should be part of core functionality and maybe you can make that case?

DefaultV commented 2 years ago

In my other project I use the key prop for multiple objects which all seem to have the same behaviour. What would be a workaround for this? Storing refs to the meshes I need to replace and then disposing re-parenting the new ones?

Maybe my approach is just very niché, one of the functionalities of my project is to generate and replace meshes at runtime.

I don't know if it should be core functionality, but if I were supposed to override the instance I would expect the old mesh to be disposed somehow, maybe have a separate prop for disposeMeshOnOverride. Just another name than that :smile:

brianzinn commented 2 years ago

hi @DefaultV I think I understand your issue and hope this is a solution. There exists that functionality already and it is an "opt-in" mechanism. Whenever fromInstance prop is used I am assuming that the developer wants to manage the lifetime of the object. Imagine, for example, that you want to re-use a Texture and pass it through in different places, etc.

It's a bit buried in the code, but I am redoing the docs right now and will make sure to have an example that uses this property. Also, there will be a webpage dedicated to all of these Custom Props. Anyway, this ought to fix it for you:??

   <mesh fromInstance={box} disposeInstanceOnUnmount />

https://github.com/brianzinn/react-babylonjs/blob/master/packages/react-babylonjs/src/CustomProps.ts#L24

DefaultV commented 2 years ago

That sounds great! I'd love to see an example, I'm currently having a hard time to get it to work on my end. See my PR for more: https://github.com/DefaultV/create-react-app-typescript-babylonjs/tree/Bug-MeshFromInstanceChange

brianzinn commented 2 years ago

sorry - I totally didn't see your comment. i will look tonight.

brianzinn commented 2 years ago

I made a couple of small changes to your Box.tsx to make it clearer and isolate the issue. You can put back the useRef if you want.

import { Mesh, MeshBuilder } from "@babylonjs/core";
import React from "react";
import { FunctionComponent, useState } from "react";
import { useScene } from "react-babylonjs";

const Boxed: FunctionComponent = () => {
  const scene = useScene();
  const [mesh, setNewMesh] = useState<Mesh>(() => {
    const boxMesh = MeshBuilder.CreateBox(
      "box",
      {
        size: 2
      },
      scene
    );
    boxMesh.position.y = 1;
    boxMesh.onDisposeObservable.add(() => console.log("disposing"));
    console.log("initial box created");
    return boxMesh;
  });

  const handleOnCreate = (mesh: Mesh) => {
    if (mesh.name === "box") {
      setTimeout(() => {
        const sphereMesh = MeshBuilder.CreateSphere("sphere", {}, scene);
        sphereMesh.position.y = 1;
        console.log("created sphere");
        setNewMesh(sphereMesh);
      }, 2500);
    }
  };

  console.log(mesh.name, mesh.uniqueId, mesh.isDisposed());

  return (
    <mesh
      key={mesh.name}
      name="boxed"
      fromInstance={mesh}
      onCreated={handleOnCreate}
      disposeInstanceOnUnmount
    />
  );
};

export { Boxed };

output:

Babylon.js v4.2.1 - WebGL2 - Parallel shader compilation 
initial box created 
box, 3. false
boxed, 3, false
disposing 
created sphere 
sphere, 17, false

Notes:

  1. the name "boxed" is applied to the mesh. that is because there is a props handler. you can see it's the same mesh from the uniqueId
  2. the onCreated callback will be fired even when you assign a new one, since the "key" changes and it will call those lifecycle events.
  3. useState with the init function is only called once - not on each render.

You can always use a useEffect hook and call dispose yourself, but what I have shown above is how to get the reconciler to work with you.

If you have a tricker example then feel free to share, but otherwise I hope that makes it clear. 😄

DefaultV commented 2 years ago

Thanks a ton for the example! I am still quite curious. On your example, changing line 8 to just:

const [mesh, setNewMesh] = useState<Mesh>(
    MeshBuilder.CreateBox(
      "box",
      {
        size: 2,
      },
      scene
    )
  );

Seems to break the functionality somehow, why is that?

brianzinn commented 2 years ago

It will be evaluated evey render, whereas a function () => ... is only called the first time useState is called. The best way to imagine it is that javascript will pass in a function without evaluating the contents, but if you pass in code then it will be run by javascript and passed in (and ignored after the first render, but "ignoring" a MeshBuilder.CreateBox, will still create a box).

There is a good explanation here - better than the React docs: https://blog.logrocket.com/a-guide-to-usestate-in-react-ecb9952e406c/

brianzinn commented 2 years ago

Please re-open if you have more questions or anything is unclear.