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

Support pan, rotate and zoom camera operations #108

Merged
merged 17 commits into from
Nov 30, 2023
Merged

Conversation

skywhale
Copy link
Collaborator

@skywhale skywhale commented Jul 24, 2023

Rotate and zoom is achieved by moving the camera instead of modifying the target object.

For #61

@skywhale skywhale requested review from bschwind and tuna-f1sh July 24, 2023 17:36
@bschwind
Copy link
Owner

Hey thanks! I took a look at this and will get back to it more later. It mostly works but there are a few edge cases and issues to work out before merging. I'll go into more detail on a review later.

@tuna-f1sh
Copy link
Collaborator

Nice to be able to move the model at last. I also noticed probably the same oddities:

  • Perhaps just me (I often invert y axis on games) but it feels reversed to what I would expect.
  • The object can clip (for want of a better word) zooming out - not found the culprit yet.
Screenshot 2023-07-26 at 19 03 14

@skywhale
Copy link
Collaborator Author

skywhale commented Sep 9, 2023

@bschwind @tuna-f1sh I switched to use quartenion rotation and reversed the rotation orientation. PTAL 🙇

I could not figure out what is causing the clipping issue, though. The far and near parameters that I'm passing to projection do not seem related.

Copy link
Collaborator

@mkovaxx mkovaxx left a comment

Choose a reason for hiding this comment

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

@skywhale Hey Ryo, this is Máté! Just sharing some thoughts. Hope they'll help!

crates/viewer/src/camera.rs Outdated Show resolved Hide resolved
crates/viewer/src/camera.rs Outdated Show resolved Hide resolved
@bschwind bschwind force-pushed the camera-impr-v2 branch 2 times, most recently from 4a9c71e to 2699bfa Compare November 5, 2023 07:31
@mkovaxx
Copy link
Collaborator

mkovaxx commented Nov 26, 2023

I'm going to commandeer this PR! ;)

@mkovaxx mkovaxx self-assigned this Nov 26, 2023
@mkovaxx
Copy link
Collaborator

mkovaxx commented Nov 26, 2023

@bschwind I think this works reasonably well for now. Please try it out! ;)

@mkovaxx
Copy link
Collaborator

mkovaxx commented Nov 26, 2023

Rebased onto latest main.

@mkovaxx mkovaxx requested a review from bschwind November 27, 2023 14:23
Copy link
Owner

@bschwind bschwind left a comment

Choose a reason for hiding this comment

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

This is awesome, thanks so much for finishing this!

It functions almost exactly as I had imagined it would, very usable on a trackpad.

crates/viewer/src/camera.rs Outdated Show resolved Hide resolved
crates/viewer/src/camera.rs Outdated Show resolved Hide resolved
crates/viewer/src/camera.rs Show resolved Hide resolved
@mkovaxx mkovaxx requested a review from bschwind November 28, 2023 11:31
Copy link
Owner

@bschwind bschwind left a comment

Choose a reason for hiding this comment

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

Nice! I'll go ahead and make the change I suggested and merge it :)

let proj = Mat4::perspective_rh(
std::f32::consts::PI / 2.0,
self.aspect_ratio,
10.0,
Copy link
Owner

Choose a reason for hiding this comment

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

I played around a bit with this, I think 1.0 is a bit more comfortable for most of the models I tried.

crates/viewer/src/camera.rs Outdated Show resolved Hide resolved
@bschwind bschwind merged commit ff63a99 into main Nov 30, 2023
4 checks passed
@bschwind bschwind deleted the camera-impr-v2 branch November 30, 2023 14:42
@skywhale
Copy link
Collaborator Author

skywhale commented Dec 1, 2023

Thank you @mkovaxx @bschwind for iterating on this and merging. I'm sorry for not being able to get back to it, but I'm so glad that it landed :)

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.

4 participants