mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102.69k stars 35.37k forks source link

Use target instead of center for OrbitControls.js #3346

Closed erich666 closed 11 years ago

erich666 commented 11 years ago

I highly recommend changing "center" to "target" in OrbitControls.js. Here's why: it's then a simple drop-in replacement for TrackballControls.js, which uses "target" for the location it rotates around.

I wrote my own version of OrbitControls.js, called OrbitAndPanControls.js, back when OrbitControls.js was not properly panning. See it here: https://github.com/erich666/cs291/blob/master/lib/OrbitAndPanControls.js, and the r58 version is here https://github.com/erich666/cs291/blob/master/lib/OrbitAndPanControls.new.js. I used "target" for it.

I'd prefer just using OrbitControls.js and be done with it. Testing OrbitControls.js today (r58), it looks like pan is still not working properly (at least not for me). See

http://www.realtimerendering.com/erich/udacity/exercises/cs291-interactive-rendering/test_code/

The orbit.html code is the latest OrbitControls.js, the orbit_and_pan.html is my own version, which tracks right-mouse regardless of zoom. I made a bunch of other minor changes (such as calling "zoom" by the term "dolly", since "zoom" means "change the field of view"), but the pan control is the major one. I took out the pixel dependence in OrbitControls.js - no PIXELS_PER_ROUND.

All that said, I don't want to step on toes, I'd rather just collaborate and make a good control. I see OrbitControls.js also has keyboard controls, which mine doesn't have. I also started to add touch support for mobile devices, something TrackballControls.js has. My control does not work properly for orthographic cameras (haven't looked into that yet). Lots to be merged and fixed.

WestLangley commented 11 years ago

There is no ownership here, so don't worry about stepping on toes.

If you'd like to make a contribution, file a pull request. Afterwards, those who want to comment, will do so. If @mrdoob likes your changes, he will accept them. If not, he won't. Alternatively, he may cherry-pick pieces.

It's as simple as that.

protometa commented 11 years ago

Yeah, I was just about to submit a ticket for the pan in OrbitControls.

I would like to see OrbitControls brought up to speed with TrackballControls because I actually prefer that style of camera navigation. Where you also thinking about adding some of that nice dampening?

WestLangley commented 11 years ago

One other point... @mrdoob moved the Controls from the library and into the examples precisely because people had different opinions as to what features the Controls should have. As it stands now, the Controls must be included explicitly at the application layer, and the user is free to hack away at them as he/she sees fit.

So, the decision may be to leave them as they are, unless there is a bug, of course.

protometa commented 11 years ago

The OrbitControls pan is actually bugged. The vector gets normalized and it only increments one unit at a time. Panning slowly ends up panning more.

bhouston commented 11 years ago

Is there some way to decompose the controllers so that their individual components can be reusable? Just a thought.

Sent from my phone. On Apr 20, 2013 4:16 AM, "protometa" notifications@github.com wrote:

The OrbitControls pan is actually bugged. The vector gets normalized and it only increments one unit at a time. Panning slowly ends up panning more.

— Reply to this email directly or view it on GitHubhttps://github.com/mrdoob/three.js/issues/3346#issuecomment-16700512 .

protometa commented 11 years ago

Should Vector3.transformDirection() always normalize the result?

Otherwise, what's the quickest way to convert a Vector3 to Vector4 with a 0 4th and apply a Matrix4?

erich666 commented 11 years ago

I'll take a crack at fixing the OrbitControls.js this weekend. Like I say, I've already fixed pan, but would like it to also work right for orthographic projections. I'd also love to get the touch interface working.

On Sat, Apr 20, 2013 at 5:05 AM, protometa notifications@github.com wrote:

Should transformDirection() always normalize the vector?

— Reply to this email directly or view it on GitHubhttps://github.com/mrdoob/three.js/issues/3346#issuecomment-16701053 .

mrdoob commented 11 years ago

The only bit that concerns me about this is the target or center matter. I kind of prefer center...

erich666 commented 11 years ago

(Hmmm, how do I do paragraph breaks in this markdown language?) I'm more concerned about consistency. Having OrbitControls.js be a drop-in replacement for TrackballControls.js (a control used in 19 three.js demos, and a well-developed control) is a big advantage. My own OrbitAndPanControls.js being like TrackballControls.js means a one-word change, and now I can very easily choose which control I prefer.

Sticking with "center" means that every user has to search their code and change "target" to "center". Given that OrbitControls.js is used in just one demo in three.js itself, switching doesn't seem like a big change.

How about this: is there a clean way to make "center" (a parameter) deprecated?

FWIW: "target" is the common term when using lookat, e.g. Unity uses it: http://docs.unity3d.com/Documentation/ScriptReference/Transform.LookAt.html

On Sat, Apr 20, 2013 at 10:22 AM, Mr.doob notifications@github.com wrote:

The only bit that concerns me about this is the target or center point. I kind of prefer center...

— Reply to this email directly or view it on GitHubhttps://github.com/mrdoob/three.js/issues/3346#issuecomment-16704777 .

mrdoob commented 11 years ago

What if instead of target it would have been named a89sd09sd760sad78f0sdf? Would you still rename OrbitControls.center to OrbitControls.a89sd09sd760sad78f0sdf because there are more examples using a89sd09sd760sad78f0sdf?

So, my point is... I agree about consistency, I'm just not sure that target is the right name. And maybe it would be better to change TrackballControls's target to center.

erich666 commented 11 years ago

Of course not ;-> But "target" is the accepted term for aiming a camera. You probably missed my update that "target" is what Unity uses: http://docs.unity3d.com/Documentation/ScriptReference/Transform.LookAt.html. Sketchup has a "target" parameter on its camera, not center: http://www.sketchup.com/intl/en/developer/docs/ourdoc/camera. It's also what 3DS MAX uses: http://www.expertrating.com/courseware/3DCourse/3D-Cameras-1.asp. It's what Cinema 4D uses: http://www.youtube.com/watch?v=-uXctw54wkY. It's what people call it in Blender: http://www.youtube.com/watch?v=n7lpoGCrvwU. Users of Maya talk about setting a target for the camera: http://www.digitaltutors.com/forum/showthread.php?4131-Camera-Lock-on-Object. Basically, no modeler I could find calls it "center camera", everyone seems to call it "target camera".

mrdoob commented 11 years ago

Yep. That's a better reason :)

erich666 commented 11 years ago

I agree, better to argue it on its own merits, and I'm glad I researched it (I've called it "target" for awhile, but wasn't sure if that was just me). Also, I was thinking about it, "center" could confuse new users, "'center' is where I put my camera, it's the center from where I'm looking".

On Sat, Apr 20, 2013 at 11:10 AM, Mr.doob notifications@github.com wrote:

Yep. That's a better reason :)

— Reply to this email directly or view it on GitHubhttps://github.com/mrdoob/three.js/issues/3346#issuecomment-16705432 .

WestLangley commented 11 years ago

Actually, "target" and "center" are the "pivot" point for the controls -- not just a point at which the camera is looking. It is the "center of rotation".

three.js does not have a concept of "camera target", it has the concept of "camera orientation" as specified by a rotation or quaternion.

So @erich666, your list of examples is irrelevant IMHO.

mrdoob commented 11 years ago

Yep. That's what I was thinking. And that's why I was leaning to the center side.

protometa commented 11 years ago

I can also vouch that "target" is the more common name in 3D production.

The reason is that the camera "target" isn't just the point that the camera pivots around, it's the point that the camera is constrained to "look at". In this specific case, these controls pivot the camera around the target for navigation, but generally the target or the camera can be moved independently and the camera will follow ("aim at") the "target".

To me, the camera "center" would be the point that the camera object genuinely rotates around, i.e. it's local origin, which is never altered in these controls.

WestLangley commented 11 years ago

@protometa center is a property of OrbitControls and it is the "center of rotation"; it is not a property of the camera. There is no "camera center".

protometa commented 11 years ago

I know target/center is not a property of the camera. I think that's beside my point.

But in both TrackballControls and OrbitControls, the target/center isn't just the "center of rotation", it's generally the point that the camera is made to lookAt(). In both cases, the only thing affecting the actual orientation of the camera object is the lookAt() method.

protometa commented 11 years ago

centerTarget it is then. ;P

WestLangley commented 11 years ago

How about both?

this.center = new THREE.Vector3(); // center of rotation

this.target = this.center;

this.object.lookAt( this.target );
mrdoob commented 11 years ago

To me, the camera "center" would be the point that the camera object genuinely rotates around, i.e. it's local origin, which is never altered in these controls.

I think that's the key problem. It's not the camera center, it is the controls center.

erich666 commented 11 years ago

which is never altered in these controls.

Well, actually, it's altered whenever you pan. If you pan, you normally move the target, too. You can try not moving the target, but then things get bizarre, your rotation logic becomes peculiar. I'm not sure what the point of tracking the center separately would be.

I think your code idea should work, WestLangley, it gives backwards compatibility.

The "target" often means two or even three things for a camera, depending on the modeler. It can mean:

1) the lookat location to view 2) the location to rotate around 3) the location that's in focus if a depth of field effect is used

In all these, it's called the "target", AFAIK, not the "center". For example, the Target Camera in 3DS MAX is designed to continue to point at a location (the target) as you orbit the camera, e.g. http://www.youtube.com/watch?v=3SW4W0jtFaM I'd like to follow what most people using modelers use as the term, namely "target". In two weeks when my course, http://bit.ly/ericity, comes out with its Camera unit, the target is the term we use.

protometa commented 11 years ago

To me, the camera "center" would be the point that the camera object genuinely rotates around, i.e. it's local origin, which is never altered in these controls.

My point there was that the the same controls could be achieve by transforming the camera origin or parenting the camera to a "center" point, and then literally applying the rotation to the camera object or parent.

But in setups where the camera is repositioned around a point with a look-at constraint, that point is generally called the "target". And this doesn't just apply when the camera has a specific "target" property (see the Blender example above).

The reason the "target" setup is more common and preferred over just rotating the camera around a center origin, is that the target and camera can easily be moved independent of each other or the controls. You can try manually repositioning controls.target in the console to see what I mean. The controls don't just have a center, they set up a genuine camera target that can be manipulated independent of any control inputs, and that's a definite advantage in many situations.

protometa commented 11 years ago

which is never altered in these controls.

I meant it's never altered relative to the camera object coordinates... but maybe that makes less sense than I thought it did...

erich666 commented 11 years ago

OK, I've improved OrbitControls.js in a number of ways. I'd definitely appreciate testing:

bit.ly/r666rtr

orbit.html is how things work currently in three.js - pan not working, touch not working.

orbit_and_pan.html is the new orbiter. Here it's called "OrbitAndPanControls.js", just for testing purposes.

I've put in a pull request, but really would love more testing. Two known bugs: there is arrow key support in OrbitControls.js (I didn't put it there), right now it pans one pixel per key press. However, sometimes keys are not enabled when the application starts - I haven't figured out what the trick is there. For multitouch, sometimes the zoom two-fingered touch zooms other things (e.g. the view as a whole). I'm definitely no touch expert, and this browser feature is not exactly well-documented online, that I could find.

Here's the pull request rundown:

Added pan for perspective and orthographic cameras. Added multitouch control: 1 finger rotate, 2 zoom, 3 pan. To make the control a (mostly) drop-in replacement for Trackball: Added "target" (same as "center", which is deprecated) Change userZoom to noZoom, etc. Change userZoomSpeed to zoomSpeed, etc. Removed unused functions and other cleanup.

Modified a few demos to use this control, where an orbit makes more sense than trackball. In other words, those with a ground plane, where there's a definite up direction.

arodic commented 11 years ago

With pan controls added this is starting to look more like EditorContorls Perhaps eventually this could be merged into a single controller. Multitouch features are really cool.

erich666 commented 11 years ago

Heh, yet another control I've never heard of until now - thanks! I wish I knew what EditorControls did (hey, I'm guilty of not documenting OrbitControls when I had a chance, so I won't complain). Actually, I would like to make documentation for these controls someday. Right now they don't seem to fit into the three.js documentation tree itself, but maybe it's an "extra"? Advice appreciated.

mrdoob commented 11 years ago

Heh, yet another control I've never heard of until now - thanks! I wish I knew what EditorControls did

EditorControls is a fork of OrbitControls. Last month I decided to replace TrackballControls with OrbitControls in the editor but these controls had a lot of things that the editor didn't need and I also wanted to experiment with different behaviours so I opted for forking it. The current state seems to be a minimal version of OrbitControls.

protometa commented 11 years ago

@erich666 Yeah that's good panning in OrbitAndPanControls. Well done factoring in the dolly distance and field of view. The response is perfect. I can't test the touch though.

And I thought of another advanced feature to maybe consider: always orbiting around the camera.up vector.

erich666 commented 11 years ago

And I thought of another advanced feature to maybe consider: always orbiting around the camera.up vector.

Right now that's essentially how orbit works in the OrbitControls (vs. TrackballControls). Up is kept to be the up direction for the camera. Orbiting left-right goes along the lines of latitude, essentially, and up-down along the lines of longitude. Do you mean something different?

protometa commented 11 years ago

Right now the controls will always orbit around the y axis, which is the default "up" axis in three.js, but a given camera's up vector can be set to anything. However, doing so will break the orbit controls at this point. In your demo, try setting the z axis as "up" with camera.up.set(0,0,1) to see what I mean.

Incidentally, TrackballControls doesn't have this issue because it's always working the camera.up vector. That's why you can end up sideways and upside-down and generally disoriented with TrackballControls. :P

erich666 commented 11 years ago

Ah, gotcha, thanks for the clarification! I've been brainwashed by WebGL to have Y be up for world space; I used to be a Z-is-up person. Given that most everyone seems to abide by this "Y is up" convention when making three.js demos, I leave it to someone else to bend their brain about adding code if the camera uses something else for up. I know if it was me, I'd just reformulate the data (add a rotation) so that Y was up.

Right, the trackball is independent of this, that's its "feature/bug". Great if you want to observe something from any angle and be able to change the orientation at will, too much freedom if you'd like to maintain an "up" direction. What three.js labels OrbitControls.js we call a "turntable" (though you're really moving the camera and not the model). It's a bit like you're rotating the model on a lazy Susan and bobbing your head up and down.

stemkoski commented 11 years ago

Just tried the touch controls -- using Ludei's CocoonJS to launch the website http://stemkoski.github.io/Three.js/Touch-Controls.html, works beautifully. Well done, Eric! :)

revengeoftheants commented 11 years ago

Hi, Eric. I've been relying on OrbitAndPanControls ever since I took your Udacity class (by the way, nice job and thank you!). Today I just realized that the arrow key controls do not work for the canvas because, as I discovered, key presses only fire on elements that are in focus, and the canvas can never be in focus. I fixed this by adding lastMouseDownElementTarget = event.target; to the onMouseDown handler and if ( scope.domElement !== lastMouseDownElementTarget ) {return;} to the top of onKeyDown. Finally, I attached the listener to the document rather than to the canvas. I don't have a ton of experience working with the DOM, so I'm not sure if that's the best solution or not. Thought I would let you know.

erich666 commented 11 years ago

Revenge: could you please update my pull request with your own code? It sounds like you have much more of a clue about DOM than I do (not hard - graphics I know, DOM I don't). Or send me your new code and I'll fold it into my request. Thanks so much for figuring this out, the "sometimes the arrow keys work" thing was confusing to me. I simply lifted the arrow keys code from the existing OrbitControls and didn't know how to improve it.

zakdances commented 11 years ago

What's the status of this? Has a pull request been filed yet?

erich666 commented 11 years ago

Funny you should mention, I was writing Ricardo Cabello just an hour or two ago and mentioned that I really need to fold in Revenge's code fix and other fixes mentioned. I hope to do that tonight. AFAIK it hasn't been folded in to any mainstream code area.

erich666 commented 11 years ago

OK, I spent way too much time this morning trying the various variants: Greggman's pull request, revengeoftheants, etc. I found Greggman's didn't hurt anything :) and makes sense to me (though I'm fairly clueless about scope's use). revengeoftheant's change, if I implemented it correctly, seems to sometimes turn off the use of arrow keys altogether; I'm guessing it works for him, but I couldn't get it to go myself for the three.js examples.

You can see the variants at http://www.realtimerendering.com/erich/udacity/OrbitControlVariants/ if you want to test them out yourself.

Anyway, I feel this latest pull request is correct and worth checking in, as it integrates the two previous pulls, and I added documentation for the class.