Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TrackballControls should have an API that more closely matches OrbitControls #27399

Closed
MattFerraro opened this issue Dec 19, 2023 · 2 comments
Closed

Comments

@MattFerraro
Copy link

Description

There are several differences between the API that TrackballControls provides vs what OrbitControls provides. Here's a few:

  1. enableDamping vs staticMoving (which are the same thing, just opposites)
  2. enableZoom vs noZoom
  3. enablePan vs noPan
  4. enableRotate vs noRotate
  5. zoomSpeed defaulting to 1.0 vs 1.2
  6. OrbitControls supports autoRotate and zoomToCursor while TrackballControls supports neither, but it could!
  7. dampingFactor vs dynamicDampingFactor
  8. .keys being an object in one, an array in the other
  9. .touches allowing you to configure touch support, but only for OrbitControls. It's missing on TrackballControls

Solution

I would like to see the API of TrackballControls change to conform better with the API of OrbitControls.

Alternatives

Create a new API and modify both OrbitControls and TrackballControls to match that new API. This seems worse than just modifying one, and OrbitControls seems much more popular.

Additional context

TrackballControls has known issues with touch support:
#25977

TrackballControls has known issues with reconfiguring the mouse button mapping:
#26366

@MattFerraro
Copy link
Author

Archaeological dig report:

  1. These differences were noted and corrected in this PR from Oct 31, 2019:
    TrackballControls: Refactor API. #17842
  2. But another person fixed it by merging TrackballControls and OrbitControls in this PR from Nov 3, 2019:
    TrackballControls: extends OrbitControls - API refactor #17857
  3. That same day another person also implemented the same change, just differently:
    TrackballControls: extend OrbitControls #17858
  4. On Jan 25, 2020 the work to merge OrbitControls and TrackballControls got merged!:
    CameraControls: consolidate OrbitControls, MapControls, and TrackballControls #18483
    But it was reverted 2 days later, as a release deadline was approaching too fast 😦
  5. Another PR would have achieved the same goal on March 6, 2020:
    OrbitControl: Allow camera.up to be modified at any time. #18829
    but is still open
  6. Another attempt on Aug 29, 2020:
    Implemented Controls.js superclass with pointer events and multi-touch drag tracking #20219
    was also never merged.

It seems like there is appetite to fix this mismatch!

I think that there are a few ways to resolve it:

  1. Merge OrbitControls and TrackballControls into a single combined Controls class. But this seems to be off the table because it breaks so many users' code, and these two names have been used in so many places over the years that it's hard to imagine renaming them.
  2. Keep OrbitControls and TrackballControls separate, but have them import a common core. But this seems to be off the table because in the name of simplicity, we do not want these controls files to include any dependencies. This is a design axiom.
  3. Patch TrackballControls to more closely match the API from OrbitControls. But this runs into difficulty because it seems like a wast of time to people who prefer either option 1 or 2 above, which are both disallowed.

If this summary is correct and complete, then option 3 seems to be only path forward, in which case #17842 is probably a good jumping off point.

Have I missed any other options? Before I dive in and code anything, can I get a go-ahead that option 3 really is best option we've got? Or equally good, a different suggested direction?

Cheers,
Matt

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 19, 2023

There is an existing issue for this topic: #18496

Closing as a duplicate.

@Mugen87 Mugen87 closed this as not planned Won't fix, can't repro, duplicate, stale Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants