Consolidating MapControls, OrbitControls ans TrackballControls

[mrdoob]

Continuing the discussion from #18483.

The problem with that approach is that, if we did that, why not also consolidate all loaders into a single file too?

For usability reasons I think it's better to keep this API:

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

So the proble to solve is how to make these files consolidate code without changing the API.

[sciecode]

There are two differences between doing the proposed changes and consolidating all loaders into a single file.

1. We are not trying to consolidate every control into a single one. Just the ones where the implementation is so similar that it makes sense maintaining a unified and clear code. MapControls is, for the most part, exactly as OrbitControls but with the ROTATE and PAN buttons swapped. TrackballControls only deviates in that we don't keep the camera.up fixed.

2. Loaders have many sub-dependencies and differing workflows. If it makes sense to unify some loaders, I'm all for it. But I don't think many are similar enough where it would be beneficial maintaining and delivering a single file.

We can avoid multiple changes in the API if we serve the base class from /src and serve each the individual extension class from their own file.

If we want to keep serving every class from examples, we need to export them from a single file for the time being, otherwise we create an unwanted dependency on /js extension classes.

---

As a side note, I think Loaders share enough similarity in parsing data that we could provide a DataParser.js utility class for handling this specific task. Some loaders already have something similar implemented. We should discuss this on a different thread.

But the reason why I mention and haven't filed a PR for this inclusion is because it has similar problems to what we are discussing. DataParser is essentially a utility class for example loaders, but we can't serve it on the /examples folder or we'll have problems "importing" on /js/loaders/.

[mrdoob]

One additional note: three.min.js is already too big and I really try to avoid code moving to core when possible.

[Mugen87]

DataParser is essentially a utility class for example loaders, but we can't serve it on the /examples folder or we'll have problems "importing" on /js/loaders/.

That is good point. I think we can easier implement such refactoring as soon as exmaples/js is gone. How about waiting with this issue until we git rid of exmaples/js? In this way, we can head for a cleaner solution by utilizing the ES6 module magic.

One additional note: three.min.js is already too big and I really try to avoid code moving to core when possible.

In this case, it seems not appropriate to move CameraControls into the core. However, we might want to investigate if a generic Controls class could be developed which serves as a base class for all controls. A common interface/behavior for stuff like connect(), disconnect(), dispose() and event handling might be interesting.

[randName]

I think option (A) would work

i.e. have an index.js in the controls folder that just imports each class and re-exports them

import { MapControls } from './jsm/controls/MapControls.js';
import { TrackballControls } from './jsm/controls/TrackballControls.js';
// ... other imports
export { MapControls, TrackballControls, /* ... */ }

user code:

// bundler
import { MapControls, TrackballControls } from 'three/examples/jsm/controls';

// web
import { MapControls, TrackballControls } from 'three/examples/jsm/controls/index.js';

[mrdoob]

The problem is that FirstPersonControls may not reuse the code that MapControls, OrbitControls and TrackballControls aims to reuse.

[randName]

The problem is that FirstPersonControls may not reuse the code that MapControls, OrbitControls and TrackballControls aims to reuse.

I was going to say that the index.js doesn't actually merge anything and if there was some base utilities it can be pulled out if needed...

but i'm assuming that this is a problem with having to download multiple files to get a single type of control? e.g. having to download both CameraControls.js and MapControls.js if MapControls inherits stuff from CameraControls

IIRC for SVGRenderer, it depends on Projector in the same folder, so I had to get both files (didn't clone the entire repo)

[paulmasson]

One additional note: three.min.js is already too big and I really try to avoid code moving to core when possible.

@mrdoob are you opposed in principle to having some generic controls as part of the core code? As long as you provide three.min.js there will be people, including myself, who prefer using it via vanilla JS. I should think controls of some sort would be essential to just about everyone and worthy of inclusion in the core.

[mrdoob]

@mrdoob are you opposed in principle to having some generic controls as part of the core code?

Until we're able to trim core, that'll make core bigger. So yeah, I'm opposed until then...

[jjoakim]

I just spent several hours having problems with a broken TrackballControls (#21548) and I am a bit amazed that there already was a "solution" available but it have not been resolved yet due to bad teamwork? Since I read through some of the discussion I will add my opinion on the topic.

My belief is that it is better to get things rolling than procrastinate on something. It is better to have a working implementation that can be used. It does not need to be a perfect solution on first try as long as the code gets tested. The perfect solution usually comes after a few iterations when the developers starts to understand the problems and comes up with good solutions. Without a working solution no one will be able to understand to full complexity of a given problem.

There are two ways to solve this. Both of them would try to keep consistency with how it works now because as a user it is quite simple to use and understand. Include the control you want and get rolling.

1. Add CameraControls (or similar) and export the other three controls from it. Then add three files that imports CameraControls and exports a single individual control in return The benefit of this approach is that users still can use OrbitControls etc. without any major alterations to their project. If a single file approach is desirable by the user they can simply use the CameraControls file.
2. Duplicate CameraControls (or similar) into three different files. It is an easy solution that just works. The main drawback is that it is more difficult to maintain it. But that is only until a better solution can be found. Meanwhile the code will be used and bugs can be found and new features can be added resulting in the project moving forward.

[mrdoob]

@Mugen87 I think this can be closed now?