-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[raymath] Make every normalize function similar #3847
base: master
Are you sure you want to change the base?
Conversation
@planetis-m This change modifies multiple functions and it's quite sensible, did you implement some test example to verify everything works exactly the same way? Some test cases would be really useful in this situation. |
I don't have tests cases for all the functions I changed, they're quite a lot, however my justification is that the number with square root 0 can only be 0. So delaying the call to the function sqrtf after the "if value is 0" statement doesn't make any difference in the result. In fact it was already done like that in the ClampValue functions. |
Relevant SO question: https://stackoverflow.com/questions/73988574/given-x0-is-it-possible-for-sqrtx-0-to-give-a-floating-point-error Maybe it's preferable to remove the check altogether? |
I have tried the example in the SO answer and the condition #include <stdio.h>
#include <math.h>
int main() {
double x = 0x1p-1021;
double y = 0x2p-1020; // or y = 0
double xxyy = x * x + y * y;
if (xxyy == 0) printf("caught\n");
printf("%a %a\n", x, y);
printf("%a %a\n", xxyy, sqrt(xxyy));
return 0;
} But I can switch the checks to abs(xxyy) <= EPSILON, if it's any better. |
@planetis-m This PR changes many functions and it should be carefully tested to verify every new implementation behaves exactly like previous one. Also note that comparing float values with |
I notice there are some mismatches checking the length > 0.0f some places and != 0.0f in other places. Regardless, most of these functions follow the same pattern of Calculate length using sqrtf and divide each vector member by length. In reality you may have V.X * Y / LEN, but since Y is 1.0f, it becomes V.X / Len. Each function could therefore be reduced to: // Normalize provided vector
RMAPI Vector2 Vector2Normalize(const Vector2 v)
{
const float length = sqrtf((v.x*v.x) + (v.y*v.y));
return (length > 0.0f) ? CLITERAL(Vector2){v.x / length, v.y / length} : CLITERAL(Vector2){0};
} Step 1: Calculate Length I may be wrong, so further observation is required. Edit: Currently no cliteral and compound literal would look like: // Normalize provided vector
RMAPI Vector2 Vector2Normalize(const Vector2 v)
{
const float length = sqrtf((v.x*v.x) + (v.y*v.y));
Vector2 result = {0};
if (length > 0.0f) {
result.x = v.x / length;
result.y = v.y / length;
}
return result;
} I'm not sure what your thoughts are on const-correctness and conciseness here @raysan5 , since Raylib is also for educational use, a smaller, let's call it "more clever" way of doing things is not necessarily better for teaching and learning. |
Follow up to #2982 Makes normalize functions consistent, moved sqrtf call after the if statement which causes its assembly to be slightly better in both clang 17 and gcc 13. It now uses one rsqrtss instruction instead of sqrtss + divss combo https://godbolt.org/z/5K475o88n plus in clang it's branchless.