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

OrbitControls: Add missing epsilon to change check #27620

Conversation

jteppinette
Copy link
Contributor

Description

OrbitControls returns a boolean value denoting if the camera has been changed or not. To determine a change, it checks if the camera position, target, and rotation have been modified. These values have to be checked against epsilon as to not see floating point precision errors.

Thehe camera target change check is missing this epsilon limit and therefore change events are fired significantly longer than they should and after camera change has stopped.

@N8python @mrdoob I noticed this error when adding support for N8AO accumulation, since this causes accumulation to wait several (~10 seconds) after a pan event has occurred and the camera stop moving.

This contribution is funded by Protex.

@gkjohnson
Copy link
Collaborator

gkjohnson commented Jan 26, 2024

Perhaps it's something to deal with in another PR since it's already a problem but I think if we're checking for epsilons and not firing events then the camera position needs to actually not change - not change a miniscule amount. When zoomed in close on a model the changes from this epsilon value are still noticeable and can cause blurring when accumulating render samples due to camera movement.

If the momentum animation or drag events do not represent a large enough motion then maybe we can just skip updating the camera transform.

@jteppinette
Copy link
Contributor Author

@gkjohnson You are 100% correct. I’m trying a couple different solutions to this, and I will make a separate PR as soon as it’s ready, hopefully tomorrow. As you said, the current setup causes issues with accumulation since change events and camera changes aren’t perfectly aligned.

@jteppinette
Copy link
Contributor Author

jteppinette commented Jan 26, 2024

@gkjohnson I am about to make a PR about this, but thought I would comment you the idea first:

This seems to be working for me, suprisingly simple solution I landed on, basically just resetting to last variables if epsilon check doesn't pass and only decreasing Delta/Offset if camera changed. Thoughts?

Moving that "decrease delta block" to the happy path makes it so you can still move the camera when distance is < EPS, because they just compound until they pass. Fixes issue when zoomed in super close.

line 365 in OrbitControls.js

if (EPS CHECKS) {
  // EXISTING CODE (CHANGE EVENT / LAST RESET)

  if (scope.enableDamping === true) {
    sphericalDelta.theta *= 1 - scope.dampingFactor
    sphericalDelta.phi *= 1 - scope.dampingFactor
    panOffset.multiplyScalar(1 - scope.dampingFactor)
  } else {
    sphericalDelta.set(0, 0, 0)
    panOffset.set(0, 0, 0)
  }
} else {
  scope.object.position.copy(lastPosition)
  scope.object.quaternion.copy(lastQuaternion)
  scope.target.copy(lastTargetPosition)
}

@gkjohnson
Copy link
Collaborator

This seems to be working for me, suprisingly simple solution I landed on, basically just resetting to last variables if epsilon check doesn't pass and only decreasing Delta/Offset if camera changed. Thoughts?

Seems reasonable to me!

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 8, 2024

I guess the PR can still be merged? The lastTargetPosition check is now consistent to the lastPosition check (both use EPS). Otherwise it seems confusing why both checks are implemented differently.

@gkjohnson
Copy link
Collaborator

I think it can be merged since it's consistent. The current EPS solution just has limitations and not great for the accommodating the kind of accumulation referenced in the original post. But I'll leave it to @jteppinette if they'd like to look into another PR to handle the issue in a better way.

@Mugen87 Mugen87 merged commit f051f1a into mrdoob:dev Feb 8, 2024
11 checks passed
@Mugen87 Mugen87 added this to the r162 milestone Feb 8, 2024
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