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

Added tests, fixed a bug, and more #42

Merged
merged 1 commit into from Jan 6, 2021
Merged

Conversation

dizcza
Copy link
Contributor

@dizcza dizcza commented Aug 8, 2020

  • added first tests (partially addresses Needs a test suite #4). I skipped some primitive functions, though, in best practice, every function should be tested - I hope others will contribute!

  • added CircleCI suite for your repository that will be automatically run on each commit. All you need to do is

    • create an account on circleci.com, if you don't have it yet;
    • choose this project from a list of your projects
    • choose "Use existing config"

    In order to show the badge, I renamed README to README.md

  • fixed a bug in mat4x4o_mul_quat: the last element in each row of the resulting matrix was not set. Previously, there was junk data.

  • Reused vec4 code for quat:

    #define quat_add vec4_add
    #define quat_sub vec4_sub
    #define quat_norm vec4_norm
    #define quat_scale vec4_scale
    #define quat_mul_inner vec4_mul_inner

    Previously, only #define quat_norm vec4_norm was set.

  • Normalize axis to a unit norm in quat_rotate. An alternative implementation, when the user has to deal with normalization manually, is

    LINMATH_H_FUNC void quat_rotate(quat r, float angle, vec3 const axis) {
      float s = sinf(angle / 2);
      float c = cosf(angle / 2);
      vec3_scale(r, axis, s);
      r[3] = c;
    }

    The previous version was strange and not optimized. Let me know which one you prefer - force users to manually normalize axis (the snippet above, optimized) or take care of the normalization in the function body (current).

  • added vec##n##_dup function (used it in mat4x4_dup and mat4x4_scale_aniso functions, and several times in linmath_test.h). Previously, there was only a mat4x4_dup function. Also, now we can get rid of unsafe memcpy, which is the only function that requires <string.h> to be imported. Let me know if it's better to reuse vec##n##_dup instead of memcpy (the fix if very easy to add).

  • (minor) added const qualifiers to each function (hints). Previously, there were several functions with a const qualifier and most of them didn't have any. We should either put const every time when the argument is not modified or don't use const at all. On second thought, I'm not sure how much it makes sense - for me, the tradeoff of const versus not const has never been clear.


There is a bunch of things. Let me know if you want to include only a part of them.

Danylo


Update 1
I noticed that linmath.h sometimes is used as a submodule to build a C/C++ project. Adding linmath_test.c with a main() function will break such workflows. Does it make sense generating linmath_test.c on the fly at CircleCI runs?

Update 2
I noticed that quat_inner_product was used to compute the inner product of two quaternions. Now I redefined it as #define quat_mul_inner vec4_mul_inner. Doing so introduces a breaking change for other projects that use linmath. Let me know if I should rename it to #define quat_inner_product vec4_mul_inner.

@datenwolf
Copy link
Owner

Wow! That is a lot of stuff done. Thanks!

I'll see to it as soon as I can.

@datenwolf datenwolf merged commit 0538757 into datenwolf:master Jan 6, 2021
@iTitus iTitus mentioned this pull request Jan 31, 2021
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.

None yet

2 participants