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

Editor: Fixed wrong aspect ratio for non default cameras on window resize #28425

Merged
merged 1 commit into from
May 20, 2024

Conversation

ycw
Copy link
Contributor

@ycw ycw commented May 19, 2024

The issue: non default cameras' aspect ratio doesn't get updated on resize:

non.default.camera.has.wrong.aspect.mp4

With this PR:

non.default.camera.has.correct.aspect.mp4

Preview: https://raw.githack.com/ycw/three.js/editor-shading-with-correct-aspect-ratio/editor/index.html

@mrdoob mrdoob added this to the r165 milestone May 20, 2024
@mrdoob mrdoob merged commit a620f3f into mrdoob:dev May 20, 2024
11 checks passed

const camera = editor.cameras[ uuid ];

if ( camera.isPerspectiveCamera ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't all cameras be resized? The issue still happens with orthographic cameras.

Copy link
Contributor Author

@ycw ycw May 20, 2024

Choose a reason for hiding this comment

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

Correct, this PR only handled the perspective cameras' aspect ratio. To keep consistent with this

} else if ( viewportCamera.isOrthographicCamera ) {
// TODO
}
, should I also add else if ( camera.isOrthographicCamera ) { // TODO } for clarification?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This check should be removed and orthographic cameras resized like perspective cameras. The aspect needs to be honored in the left and right property otherwise the projection matrix is incorrect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, thanks for pointing out that, would you mind file a PR or add //TODO for now because ortho camera is not in my todo list :'(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make sure that we're on the same page, this is the issue we didn't fix:

orthographic.camera.projection.matrix.not.updated.on.reisze.mp4

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