CameraControls: consolidate OrbitControls, MapControls, and TrackballControls

[WestLangley]

This PR allows OrbitControls, MapControls, and TrackballControls to share the same code.

Usage:

import { OrbitControls } from './jsm/controls/CameraControls.js';

import { MapControls } from './jsm/controls/CameraControls.js';

import { TrackballControls } from './jsm/controls/CameraControls.js';

The behavior of OrbitControls and MapControls is unchanged.

The TrackballControls implementation is completely different from the existing version, and some users may say the "feel" is different.

The existing OrbitControls.js and TrackballControls.js can remain for awhile.

I can add a deprecated warning to those files whenever that is desired.

[Mugen87]

I don't think it's a good idea to export three different classes from the same module. I've already mentioned here #17857 (comment) that this style is confusing.

CameraControls should only export CameraControls. The documentation of the class as well as an example can then make clear how to configure the controls.

As a long term goal, I don't think that we need to have both TrackballControls and OrbitControls. A single controls class that orbits the camera around a target position in 3D space is sufficient.

I think I would rather assign the RXXX milestone to this PR since I don't think we should merge this without enough consideration. (In any event, please let @mrdoob decide when to merge the PR!)

BTW: This PR closes #17858 and #17857, right?

[sciecode]

I don't think it's a good idea to export three different classes from the same module. I've already mentioned here #17857 (comment) that this style is confusing.

Pretty sure we have other cases where we export multiple classes from a single module. I don't see this as a problem as it's very common in other frameworks as well.

As a long term goal, I don't think that we need to have both TrackballControls and OrbitControls. A single controls class that orbits the camera around a target position in 3D space is sufficient.

MapControls, TrackballControls and OrbitControls are but a specific configuration of what we, now, call CameraControls. Having these as individual classes serves as backwards compatibility, as well as ease of use. That way users don't need to remember the specific configuration for each scenario.

(In any event, please let @mrdoob decide when to merge the PR!)

Don't mean to point fingers at anyone, but we have yet to hear anything from @mrdoob on this matter.

Honestly, I really would like to see this getting merged. We have been stuck on this discussion for about 3 months without any resemblance of a resolution. Let's, at least, try to find a common ground so that we can make progress on the matter.

[WestLangley]

@Mugen87 I hoped you would have tested this and commented favorably on the contribution, but I guess that was too much to expect.

I think I would rather assign the RXXX milestone to this PR since I don't think we should merge this without enough consideration.

In other words, you want to block it, just as you have continued to block my other PRs.

We can't function as long as you act as a blocker. When you create controversy, progress stops completely, and everyone is frustrated except for you.

I am exercising my best judgment and merging this. Let's give users the opportunity to try this new implementation of TrackballControls.

[mrdoob]

Don't mean to point fingers at anyone, but we have yet to hear anything from @mrdoob on this matter.

Honestly, I really would like to see this getting merged. We have been stuck on this discussion for about 3 months without any resemblance of a resolution. Let's, at least, try to find a common ground so that we can make progress on the matter.

As always, sorry for the delay... 😅

The notifications for these discussion were being pushed down continuously. So I've not been able to read the discussion.

I agree with @Mugen87 that this approach doesn't follow the current pattern and has side effects like not being able to search for "OrbitControls" in the docs anymore.

If I understand correctly, the idea is to unify the controls and reduce code duplication. Right?

If so, I think something like this would follow the API pattern better:

src/controls/Controls.js
examples/js/controls/MapControls.js
examples/js/controls/OrbitControls.js
examples/js/controls/TrackballControls.js

So MapControls, OrbitControls, and TrackballControls would extend src/controls/Controls, just how GLTFLoader extends src/loaders/Loader.js.

That way the user-facing API wouldn't change:

import { MapControls } from './jsm/controls/MapControls.js';
import { OrbitControls } from './jsm/controls/OrbitControls.js';
import { TrackballControls } from './jsm/controls/TrackballControls.js';

But the code will be cleaner.

How does that sound?

[mrdoob]

Another option would be to have all in examples like this:

examples/jsm/controls/Controls.js
examples/jsm/controls/MapControls.js
examples/jsm/controls/OrbitControls.js
examples/jsm/controls/TrackballControls.js

MapControls, OrbitControls, and TrackballControls would import Controls internally. But I don't know how to make this work with examples/js/.

It's tempting to delete examples/js/controls/* but it may be too soon?

[mrdoob]

A third option would be to use a different name:

examples/jsm/controls/CameraControls.js
examples/jsm/controls/MapCameraControls.js
examples/jsm/controls/OrbitCameraControls.js
examples/jsm/controls/TrackballCameraControls.js

That way, we can just ignore examples/js as the names no longer collide. We can also use the oppotunity to do extra clean up if needed.

[WestLangley]

I am fine with any of those options.

[donmccurdy]

It's tempting to delete examples/js/controls/* but it may be too soon?

Recalling #16920 (comment). 😅

But r113 would certainly be too soon, now, let's at least allow time to write detailed migration options.

---

Two thoughts:

(1) If we'd rather have separate imports, long-term ... this wouldn't be a problem if we were only supporting examples/jsm, as #18483 (comment) points out. Users could import as they do now, and the shared code would be imported automatically. It'd be great to avoid a breaking change to those imports if we're just planning to change it back in the near future.

(2) If we'd rather consolidate imports ... under a single root file, let's make sure we use a consistent naming convention. UpperCamelCase suggests a class, to me, and I wouldn't expect it to have multiple exports. Instead:

// (A) These are equivalent in some bundlers, but only the second works on the web.
import { OrbitControls } from 'three/examples/jsm/controls/'
import { OrbitControls } from 'three/examples/jsm/controls/index.js'

// (B) Works anywhere. Lowercase to disambiguate from classes.
import { OrbitControls } from 'three/examples/jsm/controls/controls.js';

In my opinion, either of the patterns above would also benefit folders like nodes/ and curves/.

[Mugen87]

In other words, you want to block it, just as you have continued to block my other PRs.

TBH, I'm very disappointed about the way you behave in this thread. I'm sorry but this shows a poor capacity for teamwork. I've blocked your PR because I believe it's the wrong approach. If you can't accept this, well, then it's your problem. Software projects can't work if the devs do whatever they want. I explicitly asked you to wait for @mrdoob 's feedback what you have ignored.

So I suggest no one of the collaborators merges PRs anymore. That includes of course me. In this way, it's at least possible to avoid such solo actions.

So MapControls, OrbitControls, and TrackballControls would extend src/controls/Controls, just how GLTFLoader extends src/loaders/Loader.js.

That was always my preferred goal! However, depending on how Controls is developed, the class might not be as generic as Loader. Meaning other controls like DeviceOrientationControls or FirstPersonControls will not be able to derive from it. In that regard, the core is probably not the right place for this class.

I also vote to revert the PR since I doubt we can manage to properly implement the other approach within two days. That might be something for the upcoming releases.

[Mugen87]

It's tempting to delete examples/js/controls/* but it may be too soon?

Since the editor is now module based, I'm not aware of any dependencies to js. I think we should delete js in one of the next releases. In the first step, I would announce this change in the files (with a console.log) and then remove them two releases later. Then we can implement option 2. However, I think the name CameraControls is still too generic if controls like FirstPersonControls can't use it. We might need to think for a different name.

[sciecode]

What if we move OrbitControls (acts as the base class) to source and extend MapControls and TrackballControls from that? I think Orbit is likely the best description of the module and it doesn't conflict with any other controls.

src/controls/OrbitControls.js
examples/jsm/controls/MapControls.js
examples/jsm/controls/TrackballControls.js

I think the community might have a positive response to this ( given how it is commonly used on many projects ) and we don't run into the problem of having to

(1) Worry about examples/js imports.
(2) Go through many changes in the API.

Cause I don't see anyway we can avoid making several changes to the API/imports if we choose to keep the module on examples and also have the classes in individual files.

[mrdoob]

So seems like it's still not clear what approach we should go with here.

Considering we only have 2 days until release, I'm going to merge @Mugen87's revert but I'm going to add examples/jsm/controls/CameraControls.js back so the work is not lost and we can continue studying options and iterating.

@Mugen87 no need to include this in the change log.

[mrdoob]

Done. 1646da3