-
Notifications
You must be signed in to change notification settings - Fork 57
Add Transform limits to the camera position #136
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
base: master
Are you sure you want to change the base?
Conversation
…bit_transform could probably be better replaced witha match statement.
Plonq
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for PR. Initial review, haven't tested it yet.
| /// Adding maximum x bound for the camera transform. Default value of None has no bound. | ||
| pub camera_transform_x_max: Option<f32>, | ||
| /// Adding minimum x bound for the camera transform. Default value of None has no bound. | ||
| pub camera_transform_x_min: Option<f32>, | ||
| /// Adding maximum y bound for the camera transform. Default value of None has no bound. | ||
| pub camera_transform_y_max: Option<f32>, | ||
| /// Adding minimum y bound for the camera transform. Default value of None has no bound. | ||
| pub camera_transform_y_min: Option<f32>, | ||
| /// Adding maximum z bound for the camera transform. Default value of None has no bound. | ||
| pub camera_transform_z_max: Option<f32>, | ||
| /// Adding minimum z bound for the camera transform. Default value of None has no bound. | ||
| pub camera_transform_z_min: Option<f32>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason not to use a single Cuboid? You could potentially use closest_point() to do the constraining.
| new_transform.rotation *= yaw_rot * pitch_rot; | ||
| new_transform.translation += focus + new_transform.rotation * Vec3::new(0.0, 0.0, radius); | ||
| if (x_max.is_some()) && (new_transform.translation.x > x_max.unwrap()) { | ||
| return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning bool seems unnecessary. Just clamp/constrain it - if the values end up being the same as before, it doesn't matter.
This is a pretty gross initial edit I dashed out in about an hour. I am not proud of the list of If statements in update_orbit_transform, but its what made sense and seemed easiest to debug when I was writing. The list of If statements could probably be replaced with a match statement that returns the appropriate boolean. Also, it might be nicer to just have the update return the new_transform and then do these comparisons around lib.rs line 825 or so. I wasn't sure how I would go about testing these edits, so they are completely untested at this time.