Move side effects from Controls classes into connect/disconnect pairs

[gaearon]

Is your feature request related to a problem? Please describe.

From what I can tell, the vast majority of Three does not have side effects in the constructor. However, some (notably, Controls like OrbitControls and some others) subscribe to the DOM node in the constructor.

This makes some coding patterns unnatural (e.g. when used from React components) because we can't safely create the objects early without triggering the subscriptions, and thus we have to delay creating them just before we're ready for the subscription to happen.

Describe the solution you'd like

Although all Controls appear to attach DOM listeners in their constructor, some of them have connect / disconnect or attach / detach method pairs. My proposal is twofold:

• Standardize on a set of convention for the method pair name across all Controls (and possibly, other objects, if any, that perform DOM subscriptions in Three).
• Behavior change: Do not connect by default — require the user to call connect.

The desired behavior is that constructing a Controls object never performs side effects, and it is safe to rely on GC to collect the object if it never gets used. Also, the desired behavior is that calling connect() and then disconnect() is equivalent to having just constructed the Controls object. You should be able to cycle them multiple times.

Regarding the API, I would propose to keep the constructor arguments as is, in order to make connect() / disconnect() a convention that can be dealt with in an abstraction without "knowing" about a particular Control being used.

For example:

let controls = new OrbitControls(camera, domElement)

controls.connect(); // subscribes
controls.disconnect(); // unsubscribes

controls.connect(); // subscribes again
controls.connect(); // can't connect while connected — I think this should throw to find coding mistakes

Describe alternatives you've considered

• Standardize on method names, but start out connected (like some Controls) already do. While this somewhat helps our use case, it's a bit convoluted to call disconnect() right after creating so it takes away from the goal to make this pattern feel first-class in React. The upside of this is that it wouldn't break existing consumers. (Even the method names could be added to classes with attach / detach as aliases until the next breaking change is viable.)
• Instead of a disconnect() API, have connect() return a disconnecting function. This is a popular pattern in some communities but Three seems to have a more traditional object-oriented API so it doesn't seem to fit.
• Have connect() accept the DOM node instead of the constructor. There is some benefit to this because it lets us create an object even before we have the DOM node if it's more natural for code organization. The downside of this is that different Controls might take different options, so the knowledge about this has to be leaked to the abstraction managing the connect / disconnect lifetime. Now it has to be involved with passing their options through. In a declarative paradigm like React, options can potentially change any time, so it would have to know how to compare the "previous" and the "next" options to avoid constantly reattaching.
• Do nothing. We have a viable workaround, but it's not ideal from the perf perspective (In StrictMode, the useState() initializer function is called twice, but one of the results is discarded facebook/react#20090 (comment)) and it's likely to bite more people in the future. So it would be good to resolve this at the upstream level.

Thanks!

[DefinitelyMaybe]

Behavior change: Do not connect by default — require the user to call connect

not sure on this part but I'm all for some TLC across the controls.

[Mugen87]

I don't think the behavior change is doable since it would break a lot of code. But renaming methods or introducing missing connect()/disconnect() methods is of course good for consistency.

[gaearon]

If the change is not doable, it would be nice to have a way to call the constructor (with an option?) to start disconnected.

[donmccurdy]

Perhaps one option could be to make the domElement argument to the OrbitControls constructor optional? For example:

class OrbitControls {

  constructor ( camera, domElement ) {
  
    // ...

    if ( domElement ) {

      console.warn( 'THREE.OrbitControls: "domElement" constructor param is deprecated; use .attach()' );
      this.attach( domElement );

    }

  }

}

Older code would continue to work as it did before, optionally showing a warning to use the new syntax.

[mrdoob]

@donmccurdy Yep!

I hadn't been able to read this conversation yet, but I had been thinking about this from time to time and reached the same conclusion:

if ( domElement !== undefined ) {

	console.warn( 'THREE.OrbitControls: The domElement constructor param has been deprecated. Use .listenToPointerEvents() instead.' );
	this.listenToPointerEvents( domElement );

}

[drcmda]

i went ahead and implemented it for most controls in three-stdlib, for instance https://github.com/pmndrs/three-stdlib/blob/main/src/controls/OrbitControls.ts#L298-L314

this is fully backward compatible since it will call connect if a domelement is given to the constructor. but at the same time it allows handling side effects via manual connect if it is not. it's been in there for more than half a year and so far so good, no adverse effects, in case you still consider it.

[makc]

@drcmda you could probably also do something like new THREE.OrbitControls( camera, { addEventListener: < your implementation >, removeEventListener: < your implementation > } ) thus getting side effects out without actually changing 3js code 🤡

[pailhead]

@donmccurdy

How about this (in general)?

let didWarnAboutConstructor = false

class OrbitControls {

  constructor ( camera, domElement ) {
  
    // ...

    if ( domElement ) {

      if(!didWarnAboutConstructor){
        console.warn( 'THREE.OrbitControls: "domElement" constructor param is deprecated; use .attach()' );
        didWarnAboutConstructor = true
      }
      this.attach( domElement );

    }

  }

}

Bonus points - if (__DEV__) {...}

[Mugen87]

This has been implemented now for all controls.