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

Use is_zero_approx and fix spelling in CameraMatrix invert #58507

Merged
merged 1 commit into from
Feb 26, 2022

Conversation

aaronfranke
Copy link
Member

This PR fixes some problems noticed in #58492 (comment)

  • Fix spelling of determinant (was "determinat").
  • Fix Math::absd being used on real_t when it should be Math::abs.
  • Use is_zero_approx() instead of absd() < 1e-7 since 1e-7 is so small of an epsilon to the point of not being very useful (float precision around 1 is 2^-23 or about 1.192e-7). It's possible that 1e-7 works for this case, but if that is true, then why it's so small needs to be explained in a comment. This part needs review from @reduz.

@aaronfranke aaronfranke added this to the 4.0 milestone Feb 24, 2022
@aaronfranke aaronfranke requested a review from reduz February 24, 2022 18:33
@aaronfranke aaronfranke requested a review from a team as a code owner February 24, 2022 18:33
Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

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

Looks good.

I'm not absolutely sure on the epsilon either though so could do with another pair of eyes, I suspect that this is to cope with the case where the matrix is very small scale, or the vectors are colinear in which case I don't think you'll get anything sensible out of it (?).

I suspect anything slightly above zero in the resolution will do the job, as you'll probably see nothing on screen with such a CameraMatrix. Willing to be corrected by someone more familiar with the maths though. 😄

@lawnjelly
Copy link
Member

Note In rocket chat reduz also thought this should probably just be CMP_EPSILON, which is what is_zero_approx() does under the hood. So should be good barring the dead code that akien asked about. 👍

@akien-mga akien-mga merged commit 234eb83 into godotengine:master Feb 26, 2022
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants