-
Notifications
You must be signed in to change notification settings - Fork 91
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
Fixed various issues #37
Conversation
Check this out and see if it woks on your build @bvisness |
If you merge #38 we can have Travis run it! 😄 |
(I am checking it out and running it locally in the meantime.) |
|
||
HMMDEF float HMM_DotVec2(hmm_vec3 VecOne, hmm_vec3 VecTwo); | ||
HMMDEF float HMM_DotVec3(hmm_vec3 VecOne, hmm_vec3 VecTwo); | ||
HMMDEF float HMM_DotVec4(hmm_vec3 VecOne, hmm_vec3 VecTwo); |
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.
These function signatures all use hmm_vec3
.
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.
Also, let's add C++-style overloaded versions of these called just HMM_Dot
. (We could operator overload too, but I'm not sure there is a widely-accepted operator for this.)
I am not a contributor to this repo, so rather than do some complicated thing with branches and merging, here is a Gist with some changes that should be made to the tests: https://gist.github.com/bvisness/88a9b8b10095e600476bf3c80cdbaf85 |
@bvisness Oh my. Here ill make you a contributor now |
@bvisness You shouldve gotten a email or something now that i added you |
@bvisness Wanna do a final check on this before i merge this into master, i think were good but i would like someone else to check on it. |
I found a quick problem with the tests that I'm fixing. Give me just a bit. |
Can Travis run a MSVC build also? |
Sadly, no. :( It only supports Make with clang and gcc at the moment. Still, that should be enough to verify that everything works in general (although warnings may vary.) |
Makes sense, everything good to merge in now? |
{ | ||
EXPECT_FLOAT_EQ(HMM_Clamp(-2.0f, 0.0f, 2.0f), 0.0f); | ||
EXPECT_FLOAT_EQ(HMM_Clamp(-2.0f, -3.0f, 2.0f), -2.0f); | ||
EXPECT_FLOAT_EQ(HMM_Clamp(-2.0f, 3.0f, 2.0f), 2.0f); |
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.
Why did you remove the test for HMM_Clamp
?
Besides the test for |
Merged |
No description provided.