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

ArcballControls: Fix drifting when zooming with the mouse cursor #23838

Merged
merged 1 commit into from
Apr 5, 2022

Conversation

Tirzono
Copy link
Contributor

@Tirzono Tirzono commented Apr 4, 2022

Description

The intention of passing in this._gizmos.position to this.scale is to scale around the gizmo position. This means that in this.scale the _scalePointTemp should be (0, 0, 0), and the camera position translation should result in (0, 0, 0) as well. However, there are situations where this does not happen and that's when the this._gizmos.position gets updated. Because in this.scale this happens:

_scalePointTemp.copy( point );

[...]

this._v3_1.setFromMatrixPosition( this._gizmoMatrixState );	//gizmos position

[...]

_scalePointTemp.sub( this._v3_1 );

const amount = _scalePointTemp.clone().multiplyScalar( sizeInverse );
_scalePointTemp.sub( amount );

this._m4_1.makeTranslation( _scalePointTemp.x, _scalePointTemp.y, _scalePointTemp.z );

This means that if this._gizmos.position and this._gizmoMatrixState are out-of-sync, a translation happens which propagates and causes the camera position to drift and at some point grow massively.

By passing in the position coming from this._gizmoMatrixState this issue would be resolved.

Note that in other calls to this.scale where this._gizmos.position is passed we always call updateTbState, which syncs the this._gizmos.position and this._gizmoMatrixState (I tried to do that here as well, but it causes an issue with zooming, probably because it would also update this._zoomState and the function doesn't expect that).

@mrdoob mrdoob added this to the r140 milestone Apr 4, 2022
@mrdoob
Copy link
Owner

mrdoob commented Apr 4, 2022

@danielefornari @cignoni looks good?

@danielefornari
Copy link
Contributor

Sounds ok to me

@mrdoob mrdoob changed the title Fix drifting in ArcballControls when zooming with the mouse cursor ArcballControls: Fix drifting when zooming with the mouse cursor Apr 5, 2022
@mrdoob mrdoob merged commit ca92701 into mrdoob:dev Apr 5, 2022
@mrdoob
Copy link
Owner

mrdoob commented Apr 5, 2022

Thanks!

abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
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 this pull request may close these issues.

3 participants