OrbitControls: refactoring

[sciecode]

Based on #16803 discussed changes.

@WestLangley is this what you had in mind, when you suggested defining MapControls in the same file?

Would appreciate if someone could test *.ts, as I'm not familiar with it.

This PR requires a build update, because of constants.js changes.

[WestLangley]

@WestLangley is this what you had in mind, when you suggested defining MapControls in the same file?

Exactly. We would add a note to the migration guide.

@sciecode I will be able to check this later this week, but if it is what I expect, it will be awesome. :-)

[Mugen87]

In the meantime, consider to fix the new lgtm errors introduced by this PR, see https://lgtm.com/projects/g/mrdoob/three.js/rev/pr-698532000338fcc35a1f7bc7776474e32d7321da

[sciecode]

@Mugen87 those aren't fixable errors.
OrbitControls module can't detect TOUCH constant because I'm not pushing updated build files.
Once build is updated, the errors will get fixed automatically.

[Mugen87]

Um, I don't think it's good to modify the core for this change. three.js (the core lib) should no know anything about its examples files.

[sciecode]

I'm not sure what would be the best way to expose these constants to the user, if we don't alter core.
modularize.js doesn't export non-scoped variables, so we would need to set these as member props. Which doesn't sound right to me.

[Mugen87]

modularize.js doesn't export non-scoped variables, so we would need to set these as member props.

We are already doing this in other files for example in SAOPass (BTW: This pattern was introduced even before the ES6 module migration). We can improve this once the ES6 modules become the canonical versions of the code.

three.js/examples/jsm/postprocessing/SAOPass.js

Lines 195 to 201 in a642a21

	SAOPass.OUTPUT = {
	'Beauty': 1,
	'Default': 0,
	'SAO': 2,
	'Depth': 3,
	'Normal': 4
	};

[sciecode]

Update PR with suggested constants changes.
I'm not very confident about OrbitControls.d.ts btw.

[WestLangley]

As I said in #16803,

Please do your best to make sure the behavior is not changed. This is a refactoring only.

I was referring to backwards-compatibility in general, and the API in particular.

OribtControls already accesses THREE.MOUSE. I would like to address @Mugen87 's issues in a later PR. The code base is in flux anyway.

An API change can be implemented is a smaller PR with proper user warnings included. Please revert the last commit.

[sciecode]

done... 🙄 👀

[WestLangley]

@mrdoob Let's merge this ASAP. Please rebuild concurrently.

@sciecode If there are problems with the TS files, they can be fixed easily enough. Thanks!

[WestLangley]

@sciecode If you can fix the conflict, I think we can go ahead and merge this.

[sciecode]

Should be ready to merge.

[WestLangley]

OK, let's go for it! Thanks!

@mrdoob This PR requires a rebuild.

[mrdoob]

Rebuilt 👌

[WestLangley]

Thanks!

[EliasHasle]

When MapControls are basically OrbitControls with a different button mapping, it makes sense that they share code/file. But I would actually expect from something named MapControls that dollying follows a ray through the pointer screen position. Would this be easy to implement, maybe as an option, in the refactored version? Maybe it would even be an interesting feature to have in OrbitControls too?

[sciecode]

But I would actually expect from something named MapControls that dollying follows a ray through the pointer screen position.

Are you familiar with .screenSpacePanning flag? Is this what you are referring to?

[EliasHasle]

No, not screen space panning. I mean when you zoom. Try Google Earth. When you zoom with the mouse wheel, the map position of the mouse pointer is conserved. This holds even if you rotate the view with the middle mouse button. Very convenient. I have made something similar for 2D views, that I shared in a new repository a few days ago: https://github.com/EliasHasle/map-view-controls (no examples included, and the code may need a small overhaul)

[sciecode]

I see what you mean. It would require some tweaking to adjust to the current way we're handling dolly zoom, but it's definitely possible.

Just not sure if it fits the current "general purpose" idea behind the example files, or if it's more of application-level feature. There has been a bit of reluctance against adding new features, especially because of the need to keep things simple. I don't particularly have any opinion about it to be honest.

[EliasHasle]

I think it is an essential feature for something called "Map controls". There is no hurry, though. :-)

[WestLangley]

When you zoom with the mouse wheel, the map position of the mouse pointer is conserved.

@EliasHasle I think that is a great idea. I hacked it into Orbit/MapControls and the result is beautiful! :-)

[pepe-escrich]

When you zoom with the mouse wheel, the map position of the mouse pointer is conserved.

@EliasHasle I think that is a great idea. I hacked it into Orbit/MapControls and the result is beautiful! :-)

@WestLangley This is just what I was looking for ...Thank you very much!