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 zoom to cursor #26165

Merged
merged 17 commits into from
Jul 10, 2023
Merged

OrbitControls: Add zoom to cursor #26165

merged 17 commits into from
Jul 10, 2023

Conversation

gkjohnson
Copy link
Collaborator

@gkjohnson gkjohnson commented May 29, 2023

Fixed #22522.

Related PRs: #24983, #17145, #23254

Description

Another effort to get map-like zoom to cursor working with OrbitControls. Some of the other efforts were not sufficient from what I could tell. I tried a few different approaches and seemed to be suffering from numeric precision issues before I landed on this one.

  • Damping is not required.
  • Mouse scroll and mouse cursor position dolly works correctly.
  • Touch-zoom to cursor does not work. I would like to consider this future work.

A question:

  • If panning is disabled should zoom to cursor be implicitly disabled? I'm fine with leaving this as is and letting the user set the desired behavior.

In Progress:

  • "Jump" when reaching the max distance
  • Handle initially unset or 0 distance to target
  • Simplify zoom cases with non camera objects
  • Fix non-cursor zooming with ortho cameras

cc @WestLangley @Mugen87

Comment on lines 322 to 330
// get the ray and translation plane to compute target
const ray = new Ray();
ray.origin.copy( scope.object.position );
ray.direction.set( 0, 0, - 1 ).transformDirection( scope.object.matrix );

const plane = new Plane();
plane.setFromNormalAndCoplanarPoint( scope.object.up, scope.target );

ray.intersectPlane( plane, scope.target );
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The general approach is to move the camera such that the point under the cursor is visually consistent after zooming and the target is moved to accommodate this camera position movement.

However - when using non-screenspace panning and trying to zoom to cursor when the camera is looking parallel to the plane this portion of code fails because we're using the camera ray and plane to determine where to place the target. Any recommendations? Should we just disable zoom to cursor when the dot product of the plane normal and ray is under some threshold? Then we can recommend that users limit the angles in these cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should make it so that when you're parallel and zoom "towards the sky", we pick an arbitrary distance away and use that as if we had collided with the terrain, then use the same code. I.e. dolly the camera in that direction

Copy link
Collaborator Author

@gkjohnson gkjohnson May 29, 2023

Choose a reason for hiding this comment

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

Unfortunately this won't work since the camera always has to face the target point. This means the camera will move up into the sky and then arc down towards the view point since the target has to stay on that plane - which isn't necessarily what someone would expect.

Zoom to cursor with map controls only really makes sense when limiting the camera control angles, imo, but we should have some approach that doesn't break here. Perhaps the arcing is okay in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

btw,

since the camera always has to face the target point

how can you possibly maintain both target and the point under cursor constraints? one of them will fail as soon as you move persp. camera in any way other than rotating around the axis formed by those 2 points.

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented May 29, 2023


function clampDistance( dist ) {

return Math.max( scope.minDistance, Math.min( scope.maxDistance, dist ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could reuse the clamp function available in MathUtils

@gkjohnson gkjohnson marked this pull request as ready for review May 31, 2023 01:35
@gkjohnson
Copy link
Collaborator Author

cc @WestLangley @Mugen87 - I think this is ready for review. I have questions on the following:

  • If panning is disabled should zoom to cursor be implicitly disabled? I'm fine with leaving this as is and letting the user set the desired behavior.

And this comment: #26165 (comment)

@Mugen87
Copy link
Collaborator

Mugen87 commented May 31, 2023

If panning is disabled should zoom to cursor be implicitly disabled?

No, I wouldn't do that.

And this comment: #26165 (comment)

TBH, I'm not sure what to recommend here. Do whatever you think is best at the moment. We can still change it at a later point if necessary.

@Mugen87 Mugen87 added this to the r154 milestone Jun 1, 2023
@gkjohnson
Copy link
Collaborator Author

gkjohnson commented Jun 5, 2023

TBH, I'm not sure what to recommend here. Do whatever you think is best at the moment. We can still change it at a later point if necessary.

Okay I've just updated the controls so we don't move the camera target if zooming to cursor when the camera is 20 degrees above the horizon. This means the camera can rotate a bit during zoom (only in that corner case) but at least the target position doesn't immediately scale out of control. It's best for users to limit the altitude angle when zoomToCursor and screenSpacePanning are enabled, anyway.

Imo this is ready to merge unless someone catches something new!

@Akbar1909

This comment was marked as off-topic.

@mrdoob mrdoob modified the milestones: r154, r155 Jun 29, 2023
@nickyvanurk
Copy link

I was expecting this feature to be released in r154, any update?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 10, 2023

Let's merge this to give this implementation a chance in prod!

@alessiocancian
Copy link

I'm testing this feature and I was experiencing weird behaviors with zoom limits, was able to fix it by setting screenSpacePanning to false, maybe it should be the default value when zoomToCursor is enabled, or should at least be mentioned in the docs that for most of the use cases (I believe) it may be the correct value.

@gkjohnson
Copy link
Collaborator Author

I'm testing this feature and I was experiencing weird behaviors with zoom limits

If you don't show or specify what "weird behaviors" are then nothing can be done...

@alessiocancian
Copy link

alessiocancian commented Sep 14, 2023

If you don't show or specify what "weird behaviors" are then nothing can be done...

Yes sorry, I don't think it's really a bug but the intended behavior with screenSpacePanning enabled... the problem is that if I try to zoom horizontally I get stuck when reaching the center because distance is 0, so I need to rotate around it to explore the other side.
This was weird for me because my scene is like an open space to explore, so I didn't understand why the zoom was getting stuck, and I was able to figure it out only after checking the code of this PR and logging the distance.
When disabling screenSpacePanning the distance doesn't go to 0 when reaching the center so I can zoom to the end of the model at the opposite side.

So maybe it should be mentioned that when using zoomToCursor if the scene is like an open space screenSpacePanning should be set to false.

@makc
Copy link
Contributor

makc commented Sep 14, 2023

because distance is 0,

wait, are you saying it does not respect controls.minDistance @alessiocancian

@alessiocancian
Copy link

because distance is 0,

wait, are you saying it does not respect controls.minDistance @alessiocancian

No it does respect it, that's why I get stuck. By stuck I mean that I cannot zoom in anymore, but I can still zoom out.
I tried with a negative min distance but that didn't work either, that may be a bug of this feature but I thought it wasn't supported by OrbitControls.

@gkjohnson
Copy link
Collaborator Author

This is how OrbitControls has always worked so this shouldn't be surprising. When you zoom in within the minDistance of the target point scrolling slows and stops. If you want different behavior you can modify the target point distance based on where a user is zooming or suggest a new enhancement and submit a PR yourself.

} else if ( scope.object.isOrthographicCamera ) {

// adjust the ortho camera position based on zoom changes
const mouseBefore = new Vector3( mouse.x, mouse.y, 0 );
Copy link
Collaborator

Choose a reason for hiding this comment

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

@gkjohnson I think mouseBefore should have been instantiated once inside the closure.

Likewise, mouseAfter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops! You're right. That would be a good change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@gkjohnson Something like this.

mouseBefore.set( mouse.x, mouse.y, 0 ).unproject( scope.object );
...
mouseAfter.set( mouse.x, mouse.y, 0 ).unproject( scope.object );
...
scope.object.updateWorldMatrix( false, false ); // if you say it is needed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is a good change but this PR is already merged and I have other priorities at the moment. I'm happy to review a PR from anyone else to get it in, though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not understand the code block, so it will not be me.

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.

OrbitControls - Zoom to cursor