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 TransformControl zoom for OrthographicCamera #19116

Merged
merged 2 commits into from
Apr 13, 2020
Merged

Fix TransformControl zoom for OrthographicCamera #19116

merged 2 commits into from
Apr 13, 2020

Conversation

VJigouline
Copy link
Contributor

When OrthographcCamera is used together with OrbitControls TransformControls graphical representation is not scaled correctly. This is because TransformControls was originally written for the PerspectiveCamera, where scaling is done by positioning camera further or closer to the object.
For the orthographic projection this doesn't work, because camera position is always infinitely far away from the object, so zoom factor should be used instead.
The fix contains "magical number" 1000. This was selected by experiment, but it can be changed, if there is a good reason.
Current shape of the control with control size = 1:
image

@mrdoob
Copy link
Owner

mrdoob commented Apr 13, 2020

Can you make the code like this?

var eyeDistance;

if ( camera.isOrthographicCamera ) {

	eyeDistance = 1000 / this.camera.zoom;

} else {

	eyeDistance = this.worldPosition.distanceTo( this.cameraPosition );

}

@mrdoob mrdoob added this to the r116 milestone Apr 13, 2020
@mrdoob
Copy link
Owner

mrdoob commented Apr 13, 2020

/cc @WestLangley @arodic

@WestLangley
Copy link
Collaborator

This is a comment about the formatting only -- not the patch itself.

var eyeDistance = camera.isOrthographicCamera ? 1000 / this.camera.zoom : this.worldPosition.distanceTo( this.cameraPosition );

@VJigouline
Copy link
Contributor Author

Yes. I thought about both suggested options. Which is more preferable?

@mrdoob
Copy link
Owner

mrdoob commented Apr 13, 2020

Lets go with @WestLangley's.

@VJigouline
Copy link
Contributor Author

Done and verified in my project.

@arodic
Copy link
Sponsor Contributor

arodic commented Apr 13, 2020

Looks good to me

@mrdoob mrdoob merged commit 47e1e3a into mrdoob:dev Apr 13, 2020
@mrdoob
Copy link
Owner

mrdoob commented Apr 13, 2020

Thanks!

@WestLangley
Copy link
Collaborator

The fix contains "magical number" 1000. This was selected by experiment, but it can be changed, if there is a good reason.

This is a red flag that the implementation is not correct.

For example, try the fiddle in #19158 using the dev branch (not master) and an orthographic camera. It is unusable.

/ping @arodic

@arodic
Copy link
Sponsor Contributor

arodic commented Apr 17, 2020

Yeah, it looks like magic number wont work. I'll fix.

Please re-open this issue.

@arodic
Copy link
Sponsor Contributor

arodic commented Apr 17, 2020

Submitted a fix for this issue #19163

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.

None yet

4 participants