Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upRelax the threshold when testing whether a quaternion is normalized #342
Conversation
|
r? @kvark or anyone |
|
Since this is clearly not an ultimate solution, did you consider just not asserting on the quaternion nomralization? Instead, we could do something like this: if cfg!(debug_assertions) && !quat.is_normalized() {
warn!("{:?} in function {} is not normalized", quat, "my_func");
} |
|
I am not opposed to replacing the assertion with a warning, although I would still not want to warn in cases we know to be correct (but the precision issue cause us to mislabel as not normalized). So I would still relax the normalized check. |
|
How do you know that it's still correct if there is an arbitrary threshold involved? |
|
It's the same story with most assertions involving floats (unfortunately). The motivation is to catch bad cases where the quaternion isn't normalized but because of numerical precision issues there isn't an exact definition of normalized (or rather the exact requirement would never be met in practice) so we have to play with the threshold. the previous threshold was also arbitray, it just didn't appear to give us enough headroom in practice according to #341 |
|
@bors-servo r=kvark |
|
|
Relax the threshold when testing whether a quaternion is normalized Supposedly fixes #341 Unfortunately since this depends on how much numerical precision was lost from previous operations I don't have a good recipe for choosing a constant that fits all workloads. This changes it from `1e-6` to `1e-5` which I think is still small enough to easily detect cases where the quaternion is obviously not normalized while being less sensitive to precision errors than the previous threshold. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/euclid/342) <!-- Reviewable:end -->
|
|
nical commentedJun 6, 2019
•
edited
Supposedly fixes #341 Unfortunately since this depends on how much numerical precision was lost from previous operations I don't have a good recipe for choosing a constant that fits all workloads. This changes it from
1e-6to1e-5which I think is still small enough to easily detect cases where the quaternion is obviously not normalized while being less sensitive to precision errors than the previous threshold.This change is