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

fix: reset focal offsets on input start and setLookAt #507

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

CodyJasonBennett
Copy link

@CodyJasonBennett CodyJasonBennett commented May 5, 2024

Fixes #303
Fixes #316
Fixes #424
Fixes #491

Applies and zeroes focal offsets into the control's spherical coordinates so features like zoom-to-cursor (e.g. setOrbitPoint and then interacting with an OrthographicCamera) and setLookAt work without interruption. I have not tested setLookAt as thoroughly, but expanded the fix to it after reading #303.

@yomotsu
Copy link
Owner

yomotsu commented May 14, 2024

Sorry for the delay...
I believe I should check the changes carefully; however, I currently have limited time to do so.
A bit more time would be very helpful...

@CodyJasonBennett
Copy link
Author

No worries. These are sweeping changes, and people can work around it if they really want to today.

@@ -723,6 +723,8 @@ export class CameraControls extends EventDispatcher {
if ( ! this._domElement ) return;
if ( ! this._enabled || this.mouseButtons.wheel === ACTION.NONE ) return;

this._resetFocalOffsets();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about moving this line to the beginning of _zoomInternal() and _dollyInternal() with if ( this.dollyToCursor )?
Then wheel zoom/dolly will reset focalOffsets.

eg

protected _dollyInternal = ( delta: number, x: number, y : number ): void => {

  if ( this.dollyToCursor ) this._resetFocalOffsets();

@@ -843,6 +845,8 @@ export class CameraControls extends EventDispatcher {

if ( ! this._enabled ) return;

this._resetFocalOffsets();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think mouse drag movement does not affect focalOffset. so, can we remove this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to call this whenever any input starts. Otherwise, we may interrupt transitions or animations. Is there a better place to put this?

src/CameraControls.ts Outdated Show resolved Hide resolved
// Applies and resets focal offsets to play nice with lookAt and zoom-to-cursor
// https://github.com/yomotsu/camera-controls/issues/491
_resetFocalOffsets() {

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we check the focalOffset value first?
If the value is 0, 0, 0, let's just return and do nothing.

@Innovgame
Copy link

Thank you very much for fixing this issue. It indeed resolves #424, and I look forward to it being merged soon.

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