Defaults - phi, theta, distance

[gregtatum]

Some things I'm running into which feel unintuitive:

• You can pass in a phi and theta value initially, but it won't affect the position of the camera on the first update as its determined from the offset of the target and position.
• If you have the initial position at [0,0,0] and the target at [0,0,0] then the camera gets positioned directly up which is awkward.
• Without configuring options such as distance, the camera's position is ignored from the beginning and the controls defaults are applied, which can be awkward. You could set the default distance to the camera's distance from the target.

The potential solution to all of these is to provide the position, target, and up vectors at the beginning in order to set sane initial defaults. I'm not sure the best approach to provide those, maybe just overload the options with them. Of course once you provide them from the beginning you could just save them internally and not reference them as arguments in the update call.

Thoughts?

[mattdesl]

The potential solution to all of these is to provide the position, target, and up vectors at the beginning in order to set sane initial defaults.

This can get a bit awkward though – it may not be obvious that passing opt.position will actually mutate that vector. This is why I would prefer just "set up the controller" (constructor, no side effects) and "apply the controller" (update, with side effects). In practice this means the user should call update after setting up the controller.

1 - Yes, we should make sure the user can set phi and theta initially, so that when they call update() it works as expected. The user should be able to read/write these angles at runtime to change the camera. Also we should make sure our terminology is correct, the code was adapted from ThreeJS and I'm not sure if they are always using correct "mathematical" terms.

2 - Yes I noticed this too, things act a bit weird when they are both at origin. Maybe we need a dot() check or something to avoid this edge case? I am not sure on how to fix or what should happen in this case, will have to tinker with it a bit.

3 - I agree it's a bit clunky right now, especially because the user has to consider both position and distance to tweak how far/close the camera starts off.

[gregtatum]

Most of these issues are on the first run of the controls, so what about something like this:

function update (...) {
  if (controls.firstRun) {
    doFirstRunChecks(...)
  }
}

function doFirstRunChecks (...) {
  controls.firstRun = false

  // set the initial distance if none was passed in
  // apply phi, theta, distance defaults to the position
}

The position and offset shouldn't share the same space after a first run unless the distance were set to 0. Mutation would then only happen in that one call. A single if wouldn't add that much overhead.

[mattdesl]

I think it may cause some surprise, e.g. if the user wants to use the same orbit-controls instance to update two different "cameras" (or rays, since that is another valid use case for this module).

Just spit balling here, but another approach might be to have the orbit-controls own its own position, direction and up. Then your "camera" or "ray" classes would just copy them.

const controls = createControls({ position: [ 0, 0, 0 ], target: [ 0, 0, -1 ] })
myCamera.up.copy(controls.up)
myCamera.position.copy(controls.position)
myCamera.direction.copy(controls.direction) // position->target vector

[gregtatum]

I think this may work, I feel like those values really are internals and should be owned by the controls, especially in the context of switching between various cameras. That way you can set defaults without mutating anything. Then explicitly set the results to a camera.

const controls = createControls({ position: cam.position, target: [ 0, 0, -1 ] })
controls.update()
controls.copyTo(cam.position, cam.direction, cam.up)

console.log( controls.position === cam.position ) // false

[mattdesl]

I was also thinking of a convenience copyTo (or applyTo or apply or whatever) function.

[mattdesl]

So yeah +1 to that

[525c1e21-bd67-4735-ac99-b4b0e5262290]

Whilst you're at it:

The use of || when initialising phi, i.e. phi: opt.phi || Math.PI / 2, makes it impossible to pass a phiof 0 and have it be used.

Consider using defined here as it is used elsewhere.

[mattdesl]

Nice catch, thanks @pyrotechnick!

Closing this thread since it's all merged in thanks to @TatumCreative.