When setting a transform related prop to undefined r3f noops the update instead of resetting to default values

[itsdouges]

Video of the bug

Screen.Recording.2023-02-14.at.10.34.56.pm.mov

Steps to reproduce

1. Set a scene objects transform props:

function Box() {
  return <mesh position={[1,1,1]}><boxGeometry args={[1,1,1]} />
}

2. Unset the same scene objects transform prop:

function Box() {
  return <mesh><boxGeometry args={[1,1,1]} />
}

3. Scene object does not move back to the default position, r3f instead noops and does nothing.

This affects all transform related props (position/rotation/scale). I would expect them all to instead of noop reset back to the default values:

• position [0,0,0]
• rotation [0,0,0]
• scale [1,1,1]

---

If I can be pointed to where in the code base I should look at I can contribute a fix 😄.

[CodyJasonBennett]

This is handled by applyProps:

react-three-fiber/packages/fiber/src/core/utils.ts

Lines 279 to 296 in 963a83d

	// https://github.com/mrdoob/three.js/issues/21209
	// HMR/fast-refresh relies on the ability to cancel out props, but threejs
	// has no means to do this. Hence we curate a small collection of value-classes
	// with their respective constructor/set arguments
	// For removed props, try to set default values, if possible
	if (value === DEFAULT + 'remove') {
	if (currentInstance.constructor) {
	// create a blank slate of the instance and copy the particular parameter.
	// @ts-ignore
	const defaultClassCall = new currentInstance.constructor(...(currentInstance.__r3f.memoizedProps.args ?? []))
	value = defaultClassCall[key]
	// destroy the instance
	if (defaultClassCall.dispose) defaultClassCall.dispose()
	} else {
	// instance does not have constructor, just set it to 0
	value = 0
	}
	}

Not sure if #2726 is related, but I'd also try on 8.10.0 to see if it was a regression.

[itsdouges]

I've tried on both latest 8.11.1 and 8.10.0 and it still has the same behaviour unfortunately.

[drcmda]

this did work at some point.

hmr seems completely broken atm, i see memoizedProps crashing daily. generally we need to be careful because fixing undefined can create risk, for instance undefined component props.

function Foo({ color }) {
  return <meshBasicColor color={color} /> 

the problem is that three has no idea what identity is for most classes. not to mention that identity is contextual in some cases, like "up", or "scale". scale is a vec3, it's identify is not 111 but 000, it's all just super confusing.

[drcmda]

one way could be to completely remove each and every trace of memoizedProps and start from scratch in v9.

we either could ignore undefined with values "sticking", like

<meshBasicMaterial color="red" />
HMR: <meshBasicMaterial />
---> leaves it red

because after all this is how threejs fundamentally works. the whole problem seems to be that we take for granted how the dom works and assume three must behave similar.

or, we find a really good model that makes it happen, but until now nothing worked a 100%.

[drcmda]

an idea would be to accumulate a global value cache of prototype classes and their identity values on a need to know basis, for instance createInstance could quickly populate this.

THREE.Object3d.prototype
  + position (v3d) 000
  + scale (v3d) 111
  + up (v3d) 010
  ...
  
THREE.Material.prototype
  + color (col) "#ffffff"
  ...

it still would never cover all cases, things like matrixAutoUpdate etc, but perhaps be "good enough".

also getting cold feet ... what if custom object foo has a field that holds 2 gigabyte of data, should we copy this so that we have a default fallback?

class Foo {
  bar = Float32Array(100000000000)
}

extend({ Foo })

<foo bar={undefined} /> // 💀

and then there's what mrdoob suggested here mrdoob/three.js#21209 (comment) a whitelist

const DEFAULTS = {
  'Color': new Color(),
  'Object3D': new Object3D(),
  'Vector3': new Vector3()
}

ofc this wouldn't work with anything 3rd party or custom. but probably could be good enough for all three classes. although it could be burden to run around with a big value list.

this prototype hack seems good, too mrdoob/three.js#21209 (comment)

const thingsNeededForReact  = [Object3D, Color, Vector2, Vector3, Matrix3, ...moar_stuff]
thingsNeededForReact.forEach(thing=>{
  thing.prototype.reset = (function(){
    const _default = new thing()
    return function(){ this.copy(_default) }
  })()
})

[drcmda]

made a test branch, removed every trace of memoProps and implement defaults like so

export const DEFAULT = '__default'
export const DEFAULTS = new Map()

    if (value === DEFAULT + 'remove') {
      if (currentInstance.constructor) {
        // create a blank slate of the instance and copy the particular parameter.
        let ctor = DEFAULTS.get(currentInstance.constructor)
        if (!ctor) {
          // @ts-ignore
          ctor = new currentInstance.constructor()
          DEFAULTS.set(currentInstance.constructor, ctor)
        }
        value = ctor[key]
      } else {
        // instance does not have constructor, just set it to 0
        value = 0
      }
    }

the value will then be object.copy(value)'d by the code following, ensuring that the value isn't double-used.

...
      // Test again target.copy(class) next ...
      else if (
        targetProp.copy &&
        value &&
        (value as ClassConstructor).constructor &&
        targetProp.constructor.name === (value as ClassConstructor).constructor.name
      ) {
        targetProp.copy(value)
      }

Screen.Recording.2023-02-14.at.15.35.50.mov

looks good imo

[krispya]

So, we added a test to v9 for nooping on undefined. This was because at least one Drei component relied on this behavior to avoid compiling junk shaders if a material was passed undefined. The current behavior of applyProps in v9 handles events then exits and does not process the prop if it undefined:

export function applyProps<T = any>(object: Instance<T>['object'], props: Instance<T>['props']): Instance<T>['object'] {
  const instance = object.__r3f
  const rootState = instance?.root.getState()
  const prevHandlers = instance?.eventCount

  for (const prop in props) {
    let value = props[prop]

    // Don't mutate reserved keys
    if (RESERVED_PROPS.includes(prop)) continue

    // Deal with pointer events, including removing them if undefined
    if (instance && /^on(Pointer|Click|DoubleClick|ContextMenu|Wheel)/.test(prop)) {
      if (typeof value === 'function') instance.handlers[prop as keyof EventHandlers] = value as any
      else delete instance.handlers[prop as keyof EventHandlers]
      instance.eventCount = Object.keys(instance.handlers).length
    }

    // Ignore setting undefined props
    if (value === undefined) continue

    /// ...

Is instead the expected behavior that a value of undefined should reset to some default? How would we know what the default value should be?

Also a side note, it looks like the more Cody and I work on v9 the more it is drifting significantly away from v8. Maybe we should consider a freeze on v8 some time soon. It's already becoming an issue where I got to work on v9 and there are changes I didn't know pushed to v8 that need to get ported to v9 slowing things down.

[krispya]

made a test branch, removed every trace of memoProps and implement defaults like so

export const DEFAULT = '__default'
export const DEFAULTS = new Map()

    if (value === DEFAULT + 'remove') {
      if (currentInstance.constructor) {
        // create a blank slate of the instance and copy the particular parameter.
        let ctor = DEFAULTS.get(currentInstance.constructor)
        if (!ctor) {
          // @ts-ignore
          ctor = new currentInstance.constructor()
          DEFAULTS.set(currentInstance.constructor, ctor)
        }
        value = ctor[key]
      } else {
        // instance does not have constructor, just set it to 0
        value = 0
      }
    }

I'm not sure I follow. Is this creating a new instance on HMR and copying only the values that are explicitly defined so that the new instance inherits whatever defaults would be set by the class? If so, I think this is the best option so we don't need to keep any kind of internal record of what default values should be and non-three class defaults would also be supported.

the value will then be object.copy(value)'d by the code following, ensuring that the value isn't double-used.

...
      // Test again target.copy(class) next ...
      else if (
        targetProp.copy &&
        value &&
        (value as ClassConstructor).constructor &&
        targetProp.constructor.name === (value as ClassConstructor).constructor.name
      ) {
        targetProp.copy(value)
      }

looks good imo

AFAIK constructor.name is not reliable because it gets removed with minification. instanceof should be preferred.

[CodyJasonBennett]

HMR works by looking for a variable which was completely removed between renders, which would not include undefined (e.g. key in props, props[key] === undefined). I don't believe there will be any further regression if we keep this distinction.

Indeed, constructor.name is not safe. Compare by reference via constructor or look for a compatible prototype chain like https://github.com/pmndrs/react-ogl/blob/main/src/utils.ts#L16.

[drcmda]

Is this creating a new instance on HMR and copying only the values that are explicitly defined so that the new instance inherits whatever defaults would be set by the class?

it creates an instance (only one for each class prototype), takes the string key of the removed prop, grabs the default off the instance with it.

AFAIK constructor.name is not reliable because it gets removed

this code hasn't been touched, it's what is currently in fiber. but makes sense, i will change it to instanceof. it is this bit that would now take the value from above and object.copy() it into the element.

i'll report back with a PR.

[drcmda]

here's the pr #2757

[krispya]

HMR works by looking for a variable which was completely removed between renders, which would not include undefined (e.g. key in props, props[key] === undefined). I don't believe there will be any further regression if we keep this distinction.

Okay so this HMR fix won't affect a prop being set to undefined since the variable still exists. However it will affect if the prop is removed completely, in which case the property on the instance is set to a default value. Is that correct?

it creates an instance (only one for each class prototype), takes the string key of the removed prop, grabs the default off the instance with it.

Okay I think I understand, but I am foggy brained today and I want to be sure. We create a reference instance of each constructor that gets mounted with R3F using empty args and then look up values from this reference instance for what the default should be. What happens if the constructor requires args?

[drcmda]

we have access to args, but im scared of what this could unleash. this is my nightmare scenario, and very realistic, too

<bufferGeometry usage={THREE.DynamicDrawUsage} attach="attributes-position" args={[A500mbFloatArray, 3]} />

not something we would want to copy and hold on to in cache indefinitively. that would require the map key to be the class constructor and a deep-clone of the args. in this case removing usage would wreck memory hard.

tbh im much more at peace with not messing with args, like withholding them, classes should mostly be side effect free anyway, and all use cases i can think of would work without args, usage for instance, no problem. but let's see what @CodyJasonBennett thinks.

[itsdouges]

I'm currently leaning on memoizedProps in TRIPLEX - when you click on a scene object I traverse down the three.js tree from the event object looking for usage of the __r3f memoized props - I need to find the scene object that has its transform props set via react and memoizedProps allowed me to do that (...mostly, it isn't a perfect solution). I can look into alternatives if we think getting rid of it is the path forward.

There's definitely a distinction with it being position={undefined} and just.. not actually present. In my example since it's a custom component it still is present on the actual mesh (just now undefined instead of a value). I could also as a path forward add a react key keyed by the props I want to ensure get unset... 🤔. I'm really only impacted by transform related props currently.

Also is this really just HMR? In regular runtime you could have the transform props set behind state and have the same beiavour happen? When HMR is mentioned are we really just talking about React updates?

function HelloWorld() {
  const [pos, setPos] = useState([2,2,2]);

  const unset = () => setPos(undefined);

  return (
    <mesh><boxGeometry onClick={unset} position={position} args={[1,1,1]} /></mesh>
  );
}

[drcmda]

There's definitely a distinction with it being position={undefined} and just.. not actually present.

undefined as a noop is at least expected behaviour, it was deliberate. could be made into identity, not sure if worth it because blowing an undefined into a class from user-land imo is not something anyone should do, nor expect it to have a certain outcome. react-dom disallows it as well since 16 i believe. removal w/o hot reload would be more like this:

const [yes, set] = useState(true)
const props = yes ? { position: [1,1,1] } : {}
return <mesh onClick={() => set(false)} {...props} />

[itsdouges]

Oh so for that use case it's to be expected (where a position prop for example changes from [1,1,1] to undefined?

[drcmda]

Oh so for that use case it's to be expected (where a position prop for example changes from [1,1,1] to undefined?

so far it's treated like a noop. undefined doesn't qualify as a prop removal so to my knowledge it never ran into identity. i discussed this use case with mrdoob back then.

[itsdouges]

Ah okay, well this is the issue I was having, not not complete removal just changing to undefined. I'm not sure how common it will be in practice TBH maybe im just running into it because of testing a bunch of different things. It would be nice if it did reset though. What was mrdoobs thinking around it?

[drcmda]

it comes from this issue: #274

the problem is that you are not doing obj.position = undefined, which is what an actual prop removal would do, you are doing obj.position.set(undefined). as per fiber rules everything with a set or copy is being set or copied. according to mrdoob set(undefined) should always be a noop.

not saying it must be so, an argument could be made for that to change, though i would like that to be v9 if that were the case.

[itsdouges]

Hmm ok that makes sense from the rules at least. But it is a little confusing when in react. Do you have any thoughts for when in the context of an editor I can get the behaviour of undefined setting to arbitrary values (default) instead of nooping?

I could imagine either making undefined impossible with ast transformation or remounting with react keys but not ideal. 🤔

[krispya]

If the idea is to copy the behavior of React DOM elements then I just tested it and setting a visual value (such as fill to 'red' on an svg) then setting its value to undefined with HMR results in it resetting to its default value and not a noop.

The problem case we have is something like ShaderMaterial where vertexShader does not have a default value. Anything passed in it will try to compile, even undefined, and likely crash. The way to handle this prop when it does not have a string is to ignore it, which is what the noop was doing. However, I think this is a special case and a user could understand that if you wanted the vertexShader prop to be conditional then it has be to be fully removed and not merely set to undefined.

[CodyJasonBennett]

Closing as fixed with #2757. memoizedProps is left unchanged, but I plan to refactor it into a lazy getter to cut down on memory since we only use this in development (ff538dd). It should be a part of R3F rather than elsewhere since it relies on react and reconciler internals which are defined by R3F.