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

setOrbitPoint breaks setLookAt #303

Open
jbeard4 opened this issue Sep 8, 2022 · 4 comments · May be fixed by #507
Open

setOrbitPoint breaks setLookAt #303

jbeard4 opened this issue Sep 8, 2022 · 4 comments · May be fixed by #507

Comments

@jbeard4
Copy link

jbeard4 commented Sep 8, 2022

Describe the bug

Setting setOrbitPoint seems to change the behavior of setLookAt. I would expect setLookAt to be idempotent, which is to say, when given identical parameters, it should move the camera to the same position and point it at the same target. Calling setOrbitPoint in between calls to setLookAt seems to break this assumption, as subsequent calls to setLookAt with the same parameters move the camera to a new position/target.

What is the best way to think about the relationship between setOrbitPoint and setLookAt? Should the code integrating camera-controls keep track of the last set orbit point, and use this when calculating the values to pass into setLookAt? E.g. use the last value passed to setOrbitPoint as an offset to setLookAt?

Thank you for your time and attention to this.

To Reproduce

Steps to reproduce the behavior:

  1. Go to https://yomotsu.github.io/camera-controls/examples/basic.html
  2. In developer console, execute cameraControls.setLookAt(3,5,2,3,0,-3)
  3. Execute cameraControls.setLookAt(3,5,2,3,0,-3) again, and observe that the camera positions doesn't change.
  4. Execute cameraControls.setOrbitPoint( 0, 0, 0 );
  5. Execute cameraControls.setLookAt(3,5,2,3,0,-3) again, and observe that the camera positions now changes.
  6. Repeat steps 4-5

Code

No response

Live example

No response

Expected behavior

  • Calls to setOrbitPoint should not affect setLookAt
  • setLookAt should be idempotent

Thank you for your time and attention to this.

Screenshots or Video

Screen Capture_select-area_20220907224516 mp4

Device

Desktop

OS

Linux

Browser

Firefox

@yomotsu
Copy link
Owner

yomotsu commented Sep 15, 2022

Thanks for your detailed report, it's really helpful, and reproduced the problem.
I will take a look later... when I can have some time.

@yomotsu
Copy link
Owner

yomotsu commented Sep 16, 2022

@jbeard4

Seems, setLookAt does not respect FocalOffset which is set by setOrbitPoint.
setLookAt only changes the target(center) position and the orbit(camera) coords.

below is what setLookAt() is doing:

this._targetEnd.copy( target );
this._sphericalEnd.setFromVector3( position.sub( target ).applyQuaternion( this._yAxisUpSpace ) );

but i think setLookAt should reset FocalOffset as well.

In fact the steps below work. (Can you try please just in case?)

  1. Go to https://yomotsu.github.io/camera-controls/examples/basic.html
  2. cameraControls.setLookAt(3,5,2,3,0,-3);
  3. cameraControls.setLookAt(3,5,2,3,0,-3);
  4. cameraControls.setOrbitPoint( 0, 0, 0 );
  5. cameraControls.setFocalOffset( 0, 0, 0); cameraControls.setLookAt(3,5,2,3,0,-3);

The above should be work around so far, and I will add cameraControls.setFocalOffset( 0, 0, 0) in setLookAt.
Sounds good?

@yomotsu
Copy link
Owner

yomotsu commented Sep 17, 2022

Here is what I tried but unfortunately, FocalOffset cannot be calculated during setLookAt() transition.

	/**
	 * Make an orbit with given points.
	 * @param positionX
	 * @param positionY
	 * @param positionZ
	 * @param targetX
	 * @param targetY
	 * @param targetZ
	 * @param enableTransition
	 * @category Methods
	 */
	setLookAt(
		positionX: number, positionY: number, positionZ: number,
		targetX: number, targetY: number, targetZ: number,
		enableTransition: boolean = false,
	): Promise<void> {

		this._camera.updateMatrix();

		_xColumn.setFromMatrixColumn( this._camera.matrix, 0 );
		_yColumn.setFromMatrixColumn( this._camera.matrix, 1 );
		_zColumn.setFromMatrixColumn( this._camera.matrix, 2 );

		_xColumn.multiplyScalar(   this._focalOffset.x );
		_yColumn.multiplyScalar( - this._focalOffset.y );
		_zColumn.multiplyScalar(   this._focalOffset.z );

		const offset = _v3C.copy( _xColumn ).add( _yColumn ).add( _zColumn );
		const target = _v3A.set( targetX, targetY, targetZ );
		const position = _v3B.set( positionX, positionY, positionZ );

		this._targetEnd.copy( target ).sub( offset );
		// this._targetEnd.copy( target );
		this._sphericalEnd.setFromVector3( position.sub( target ).applyQuaternion( this._yAxisUpSpace ) );
		this.normalizeRotations();

		this._needsUpdate = true;

		if ( ! enableTransition ) {

			this._target.copy( this._targetEnd );
			this._spherical.copy( this._sphericalEnd );

		}

		const resolveImmediately = ! enableTransition ||
			approxEquals( this._target.x, this._targetEnd.x, this.restThreshold ) &&
			approxEquals( this._target.y, this._targetEnd.y, this.restThreshold ) &&
			approxEquals( this._target.z, this._targetEnd.z, this.restThreshold ) &&
			approxEquals( this._spherical.theta, this._sphericalEnd.theta, this.restThreshold ) &&
			approxEquals( this._spherical.phi, this._sphericalEnd.phi, this.restThreshold ) &&
			approxEquals( this._spherical.radius, this._sphericalEnd.radius, this.restThreshold );
		return this._createOnRestPromise( resolveImmediately );

	}

Like, you need to run the bellow twice at least.

1.mp4

Thus, the FocalOffset ( or the OrbitPoint) can't be reserved after setLookAt(), and has to be reset in setLookAt().
So, I will add a solution: #303 (comment)

@timothyallan
Copy link

I've been running into this issue as well, trying to keep the same orbit point while using setTarget to look at different objects, then re-setting the orbit point. Was wondering why the camera would slowly go crazy, but I watched the focalOffset thanks to this issue, and found it would slowly keep adjusting itself!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants