TrackballControls: extends OrbitControls - API refactor

[sciecode]

Based on @WestLangley's #17842 (comment)

Similar to what was done to MapControls on #16961

---

Notes:

• Introduces fixedUp which controls whether or not to maintain camera.up direction on rotation.

• Noticeable rotationSpeed changes, due to the different mouse coordinates used in OrbitControls.

• Implements @Mugen87's backwards compatibility changes on TrackballControls: Refactor API. #17842

[WestLangley]

I posted #17858 just to show how I implemented it. It may, or may not, be a better implementation.

I'd only change OrbitControls js, jsm, and one example file at this point, however. Don't remove the original TrackballControls. Why change 32 files when you can change 3 for now?

[sciecode]

I'd only change OrbitControls js, jsm, and one example file at this point

I will force-push a simpler PR, no problem.

Don't remove the original TrackballControls.

We did a similar thing when extending OrbitControls for MapControls, how's this case different?
Is it because TrackballControls is more broadly used? Given that we implement the backwards compatibilities changes proposed by @Mugen87 on his PR (which I did here), I don't see this being an issue.

[sciecode]

As far as implementations go, I feel like both PRs are handling the TrackballControls behavior adequately. However, I noticed the following differences:

This PR:

• Modifies camera.up.
• Works when user changes camera.up after controls instantiation (dev also doesn't support this, it's just an added bonus)

West PR:

• Doesn't modify camera.up.
• Fails to handle autoRotate = true option.
• Fails to handle screenSpacePanning = false option (correctly).

I don't know how impactful is keeping camera.up constant in the overall scheme, nor do I know if it's easy to adapt his PR to work with autoRotate = true and screenSpacePanning = false.

[WestLangley]

We did a similar thing when extending OrbitControls for MapControls, how's this case different?

The sensitivity and keyboard control are different from the original TrackballControls. I think we should keep the old code around for awhile since the new TrackballControls is different.

Fails to handle autoRotate = true option.

IMO, auto rotate does not make sense with TrackballControls because there is no natural 'up direction'.

Fails to handle screenSpacePanning = false option (correctly).

Since there is no natural 'up direction', only screen-space panning makes sense for TrackballControls, IMO.

[sciecode]

The sensitivity and keyboard control are different from the original TrackballControls. I think we should keep the old code around for awhile since the new TrackballControls is different.

Makes sense 👍

GitHack - Trackball Example

[Mugen87]

I really think this code and the definition of MapControls should go in its own module file. The following code looks weird, IMO:

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

Yes, I'm aware that the JS versions would have a dependency to OrbitControls but I think this is still better than having strange imports.

[sciecode]

Hmm, not sure actually. This is more of a preference / library consistency kind of deal, so I think that's up to you guys. I'll be happy to change if we decide on that course of action.

[sciecode]

However, if we choose to do it. Keeping the old API available won't be an option and we will need to apply backwards compatibility.

The sensitivity and keyboard control are different from the original TrackballControls.

^ this could be a problem for the users.

[Mugen87]

I don't think it's necessary to keep the old API around. Users can checkout the repo at any release and use the respective version of TrackballControls if necessary. There are even live links to older versions:

https://rawcdn.githack.com/mrdoob/three.js/r109/examples/js/controls/TrackballControls.js

[WestLangley]

Come on, @Mugen87. We tell users to always use files from the same release.

[Mugen87]

Sorry, I really don't understand your argumentation. We normally remove legacy/obsolete classes with very few exceptions. If we would keep all files, the repository would not be manageable anymore.

If users can't invest time in migration, it's okay to use one of the previous releases. Hence, I don't get your goal with retaining the API of TrackballControls. It's good to align it without keeping the old stuff around. Moreover, as demonstrated by my PR, API changes can be often done gracefully (meaning backwards compatible).

[WestLangley]

If we would keep all files, the repository would not be manageable anymore.

Come on, dude.

[WestLangley]

I don't get your goal with retaining the API of TrackballControls.

#17858 (comment)

[Mugen87]

@sciecode I think this can be closed now. See the discussion in #18483 for more information.