React concurrent mode breaks custom <OrbitControls /> properties

[johnmarinelli]

csb: https://9s00o.csb.app/

for some reason, using concurrent: true in the <Canvas /> will make enableZoom={false} on <OrbitControls /> not work. concurrent mode probably breaks other things too, but this is just one that I found.

[drcmda]

wa-what? :-D

It’s defined as:

export const OrbitControls = forwardRef((props: OrbitControls = { enableDamping: true }, ref) => {
  const { camera, gl, invalidate } = useThree()
  const controls = useMemo(() => new OrbitControlsImpl(camera, gl.domElement), [camera, gl])
  ...
  return <primitive dispose={null} object={controls} ref={ref} enableDamping {...props} />

i dont quite see how concurrent mode could affect the props being spread over it. would you please copy the orbit-helpers code and use it in your file to track down what it is? also better go to the latest version 5.x.

[hmans]

I'm also seeing strange behavior around MapControls (which, as I understand, is just OrbitControls with different defaults.)

I'm using the default panning.

With concurrent disabled, panning the camera also moves the control's target, as it should be.

With concurrent enabled, panning the camera no longer moves the control's target, and the camera will instead keep looking at [0, 0, 0].

Edit: My theory here is that the bug is not necessarily with Drei itself -- but that somehow, once concurrent mode is enabled, OrbitControls stops honoring arguments passed to it; or <primitive> doesn't correctly pass them on to the underlying object, or maybe nuke existing options on it. I'm going to do some digging in my project.

[hmans]

To make things even more weird, the bug completely disappears when I run the same code in production mode.

@johnmarinelli, can you check if this is also the case for you?

[hmans]

I think I've figured it out. It's possible that this only occurs when you create a custom camera on top of the one that will create by default, because this will end up invoking the useMemo call twice, without calling .dispose() on the controls instance that was created first.

My first attempt at fixing it was removing the dispose={false} in the MapControls implementation, but that gave me errors. So instead I very haphazardly rebuilt the component to do everything through useEffect instead:

import React, { forwardRef, useEffect, useState } from "react"
import { useFrame, useThree } from "react-three-fiber"
import { MapControls as MapControlsImpl } from "three/examples/jsm/controls/OrbitControls"

export const MapControls = forwardRef(
  (props = { enableDamping: true }, ref) => {
    const [controls, setControls] = useState()
    const { camera, gl, invalidate } = useThree()

    useEffect(() => {
      const c = new MapControlsImpl(camera, gl.domElement)
      c.addEventListener("change", invalidate)

      setControls(c)

      return () => {
        c.removeEventListener("change", invalidate)
        c.dispose()
      }
    }, [gl.domElement, camera, invalidate])

    useFrame(() => controls?.update())

    return controls ? (
      <primitive
        dispose={null}
        object={controls}
        ref={ref}
        enableDamping
        {...props}
      />
    ) : null
  }
)

I'm pretty sure that there is a much more straight-forward way of doing this, so I'm going to abstain from putting this into a PR. (It would also break compatibility because the passed in forward ref may now be null in some situations.)

[johnmarinelli]

@hmans nice one. depending on how busy i am this weekend, i can pick up where you left off. thanks for looking into this, it is indeed a tricky bug

[stephencorwin]

CM is awkward like that for some things. The dev vs prod issue is probably because of react strict mode. I'm not saying that this is necessarily an issue with OrbitControls, but another library that suffers from CM issues due to this is Apollo and their useSubscription hook.

Notice that this is true only if you're in development mode.
Try to run a production build and this effect will go away.

Something about Strict Mode parsing the code twice if I recall correctly

[hmans]

Still, shouldn't this particular issue be fixed if the instantiated controls get disposed properly?

[drcmda]

i still have no idea what's going on. it doesn't seem to create the controls twice and even if that were the case, i don't see how it would be of any consequence. it also definitively applies "enableZoom: false" - it is written into the object and i can see it in the debugger.

[drcmda]

in the update function of Orbitscontrols "scope.enableZoom" is false, in the mouseWheel handler it is true. what is going on, this makes no sense at all ...

[drcmda]

made a r educed repro here if anyone wants to dig in: https://codesandbox.io/s/r3f-contact-shadow-forked-e0stl?file=/src/index.js also copied orbitcontrols into a local file.

[drcmda]

seems like it is complex issue: facebook/react#20090

[gaearon]

To be clear, I don't think this issue is specific to Concurrent Mode. It has the same problem in StrictMode from what I can tell. However, due to an incorrect compilation setup (using production bundles in development), it seems like R3F bundles were always in production mode, so StrictMode had no effect. Recently they got fixed, surfacing this problem.

Here is a version with Concurrent Mode off (but Strict Mode on) that demonstrates the same problem: https://codesandbox.io/s/r3f-contact-shadow-forked-drey6?file=/src/index.js

Note I've had to put StrictMode inside <Canvas> so that it applies to the R3F tree. This seems like a potential pitfall although I'm afraid we can't solve this in React. Ideally, it would be nice if the Canvas component opted into Strict Mode automatically after the current issues in it are fixed.

[gaearon]

Haven’t yet gotten a confirmation from Paul but I think the correct way to use Controls is through effects rather than through <primitive />. Please see a possible solution here. facebook/react#20090 (comment)

[drcmda]

@johnmarinelli it is fixed: https://codesandbox.io/s/r3f-contact-shadow-forked-hefbg?file=/src/index.js