Setting null-initialized property causes unexpected behavior due to mutation

[bigmistqke]

The following snippet causes unexpected behavior (imo) due to r3f mutating the target value instead of setting the property. I can only reproduce this behavior when the value is null initialized.

minimal reproduction

class CustomMaterial extends THREE.MeshBasicMaterial {
    color = null;
}

function Box() {
    const [material] = useState(() => new CustomMaterial());
    const [colors] = useState(() => [
        new THREE.Color("red"),
        new THREE.Color("white")
    ]);
    const [index, setIndex] = useState(0);

    useEffect(() => {
        setInterval(() => {
            setIndex((index) => (index + 1) % 2);
        }, 1000);
    }, []);

    return (
        <mesh>
            <boxBufferGeometry />
            <primitive
                object={material}
                color={colors[index]}
                attach="material"
            />
        </mesh>
    );
}

1. add Color('red') to the CustomMaterial's (null initialized) color-prop
2. after a second, call setIndex(1)
3. Color('white') is passed to the color-prop
4. the content of the second color is copied over to the first color
5. now both colors are “white"
6. the interval goes on for eternity, copying the properties of 1 white material over in the other white material

The comment above the condition that is responsible for this behavior mentions that it is likely that the property is read-only, but I am not sure why this assumption.

Any specific reason why not to do an Object.getOwnPropertyDescriptor check? Too expensive?

[bigmistqke]

related issue: #3520

[krispya]

The typical case this is designed for is a material. It gets created with all the primitive objects, many of which are read-only (Three design), and the correct behavior is to copy values into them. However, your case shows that something is not working quite right here where not all the colors are copying into the built-in one.

With that said, I would also like an API for handling assignment instead copy but I don’t know what that would be declaratively. Typically this is done via mutation.

[bigmistqke]

However, your case shows that something is not working quite right here where not all the colors are copying into the built-in one.

Yes, the bug specifically occurs when the property is null initialized, as then it can not .copy on the initial handling of the prop:

1. renders: `
2. handle prop: myCustomMaterial's color is null initially, so when handling the color-prop it sets the property first to colors[0] (red)
3. renders: index increments to 1: <primitive ... color={colors[1]} />
4. handle prop: myCustomMaterial is now not null anymore, so when handling the color-prop it copies its properties 👉 it copies colors[1] (white) into colors[0]

and now they are both white.

Some parameters are null-initialized in threejs: e.g. Scene.background. I initially came across this bug in solid-three when setting <Canvas scene={{ background: colors[i] }} /> but setting a scene's background color like this does not seem to be supported by r3f.

If the goal of the .copy is to handle read-only properties I think it could use a better heuristic: for example using getOwnPropertyDescriptor.

It could be cached with a WeakMap if performance/memory is a concern:

const PROPERTY_DESCRIPTOR_CACHE = new WeakMap<object, Map<string, PropertyDescriptor | undefined>>()

function isWritable(object: object, propertyName: string) {
  let cacheMap = PROPERTY_DESCRIPTOR_CACHE.get(object)

  if (!cacheMap) {
    cacheMap = new Map()
    PROPERTY_DESCRIPTOR_CACHE.set(object, cacheMap)
  }

  const cache = cacheMap.get(propertyName)

  if (cache) {
    return cache.writable
  }

  const result = Object.getOwnPropertyDescriptor(object, propertyName)
  cacheMap.set(propertyName, result)

  return result?.writable
}

In solid-three I am currently doing the above but cached on the object.constructor. It is technically possible to change propertyDescriptors with Object.defineProperty, but I mean, then you are already doing some pretty wacky stuff, so caching on object.constructor as key will cover the 99.9999% usecase.

[EDIT] For react caching on obstructor will also be necessary because of the new references on re-render. Maybe isWritable is overengineered and simply doing Object.getOwnPropertyDescriptor(target, prop)?.writable in the copy-condition is enough.

[bigmistqke]

The added issue with null-initialized properties is that you can not do

return <customMaterial color={[1, 0, 0]} />

because as customMaterial.color is initially null there is nothing to .set to.

The API forces you to pass a THREE.Color, which is currently bug prone and possibly less performant: if you would do <customMaterial={new Color(...)}/> it will create a new Color-instance on each render.

Maybe the following API could solve this issue:

return <customMaterial color={setOrCreate(Color, [1, 0, 0])} />

where it prefers .set(...), but if the property is currently null/undefined it will new Constructor(...) instead?

[krispya]

We don't want to do a property descriptor check because this will create esoteric control flow. Even more esoteric than it already is! Instead of copying on all objects with a copy method, we would only copy on objects that have a little known property set. And as you noticed the shortcuts assume the objects exist so that they can be set.

Frankly, the way you are using the null initialized property is an anti-pattern with how R3F has worked for about 5 years. TypeScript should stop this from being possible since the property type would be seen as null. Technically it would be better for the shape to keep the color prop monomorphic and add an additional flag for whatever null represents.

[bigmistqke]

We don't want to do a property descriptor check because this will create esoteric control flow. Even more esoteric than it already is! Instead of copying on all objects with a copy method, we would only copy on objects that have a little known property set.

I see.

Frankly, the way you are using the null initialized property is an anti-pattern with how R3F has worked for about 5 years.

I agree that it's an anti-pattern, but it's an anti-pattern three.js sometimes makes, e.g. see Scene. My example was simply a minimal reproduction to illustrate the unexpected behavior.

In my version of solid-three I can recreate the bug with <Canvas scene={{ background: colors[i] }} /> (because Scene.background is null initialized), but this is not possible in r3f for unrelated reasons: the scene-prop does not seem to be reactive, it only sets the initial color.

TypeScript should stop this from being possible since the property type would be seen as null.

Not if the type is null | Color. Typescript has no way of declaring a default value, so we cannot infer this information.