react-three-fiber: Re-rendering component with removed props does not update underlying object

If you have some code that sets a prop, but on a subsequent re-render doesn’t set that prop, the underlying element doesn’t get re-rendered. Consider this example, where the mesh should alternate from a position of x=5 to x=0 every three seconds:

const Example = () => {
    let [isOffset, setIsOffset] = React.useState<boolean>(true);
    React.useEffect(() => {
        setTimeout(() => setIsOffset(!isOffset), 3000);
    }, [isOffset]);

    let elmprop = {};
    if (isOffset) elmprop["position"] = [5, 0, 0];

    return <mesh {...props}>
        <boxBufferGeometry args={[1, 1, 1, 1, 1, 1]}></boxBufferGeometry>
        <meshBasicMaterial></meshBasicMaterial>
    </mesh>
};

What we find is that the mesh remains at x=5, and never returns to its default position.

I believe that the issue is here, where a list of props is created that have been removed since the last rendering: https://github.com/pmndrs/react-three-fiber/blob/ab1492d214df89ceae2e8c0e7a658d2b2a67ff35/src/renderer.tsx#L215

The problem here is that leftOvers is an array of the removed props, and so the keys end up being array indices. I believe that for the array to instead contain the prop names, the correct line should be:

keys = Object.values(leftOvers)

Let me know if that makes sense and if it would help for me to create a PR 🙂

About this issue

Original URL
State: closed
Created 3 years ago
Comments: 21 (8 by maintainers)

Most upvoted comments

@drcmda I think I’ve got a way to avoid the clone is most cases. I’ll pull that together along with another place I’ve spotted that needs a tweak into a new PR shortly 😃

cgauld on Feb 7, 2021

crazy how this went on for 2 years plain skipping over removed props @gaearon and no one thought about checking this 😅 @cgauld i will review later!

I would suggest starting by looking at Three and why it doesn’t provide it

i think im ok with remembering first-value props. although cloning gets too far. if the user is just providing values, as in <foo color="red" /> this will be ok. if they write classes <foo color={new THREE.Color("red")} /> it could be ok, too - i don’t think this really has to be cloned, which i really would like to avoid.

as for three, they are completely imperative of course, so i don’t think they could change something.

const foo = new THREE.Foo()
foo.bar = 123

delete foo.bar

i would honestly not expect that to go back to a default, mostly i would think it should crash/break. but the reconciler can likely help three to remember so that sounds good to me (what @cgauld has done in his draft). that the dom can do this came to a complete surprise, i really like that it does it, though.

drcmda on Jan 31, 2021

No worries 😃 I think the reason that this is indended behaviour is as follows. Consider that the function effectively alternates between returning:

<mesh position={[5, 0, 0]}>...</mesh>

and then:

<mesh>...</mesh>

Since react is declarative, there should be no ‘side-effects’ on how those mesh components appear across renders. I.e. the second return value should not know anything about the first and so position should take its default value. Obviously behind the scenes, react and r3f reuse the underlying objects as a optimization, but thats an implementation detail 😂

PR coming shortly 🙂

cgauld on Jan 31, 2021