Differentiate between scalar and angle types.

[ghost]

If the templates allowed for a separate angle type then various functions such as matrix_rotation_axis_angle would be much more useful for integer matrix types.

[demianmnave]

Sorry for the delay, I didn't receive an e-mail notification.

Could you provide a use case? Also, are you requesting this for the master branch (CML1), or CML2?

Thanks!

[ghost]

Sure, so as I mentioned, the function matrix_rotation_axis_angle is what I'm using in particular but I'm sure theres other rotation functions with the same problem. The issue appears to be that you are using the scalar type of the matrix as the angle's type as well. If the matrix uses an integer scalar type, this renders these functions essentially unusable.

cml::vector3i axis = {0,0,1};
cml::matrix33i_c m;
cml::matrix_rotation_axis_angle(m, axis, cml::rad(90));

Adding an extra template parameter would probably be sufficient.

template < typename E, class A, class B, class L, class VecT, typename AngleT > void
matrix_rotation_axis_angle(matrix<E,A,B,L>& m, const VecT& axis, AngleT angle)

[demianmnave]

A rotation matrix must have a determinant of 1 (or -1 if you include reflections), which limits the entries of an integral matrix to +/-1 and 0. Is this really what you are looking for?

[ghost]

I'm aware, thats exactly what I'm using it for.

However, this particular edge case only serves to hi-light a larger issue. Angles and scalars are not the same thing and should be in my opinion therefore served by separate types.

[demianmnave]

As long as you know what you are getting into. :-)

The CML2 version of matrix_rotation_axis_angle() allows for any integral or float angle type that has a std::numeric_limits<> specialization, and that can be converted to the matrix scalar type. This should allow for integral matrices and floating-point angles, but that use case has never come up, so I've not tested it.

Here is the signature for good measure:

template<class Sub, class QSub, class E> void matrix_rotation_axis_angle(
  writable_matrix<Sub>& m, const readable_vector<QSub>& axis, const E& angle);

I haven't ported every CML1 function to CML2 yet, but if you'd like to give it a try and let me know which functions you need implemented, I can let you know if time would permit me to get them done.

[ghost]

Apologies for the delay, this new function looks promising; I will test it when I can but as long as the two types are separate it should in theory solve the problem I was having. Thanks!

[demianmnave]

No worries! I am going to close this issue, but please do open another if you have any more questions or problems.

[ghost]

Just an update; I attempted to install cml2; however it required disabling an error in the cmake file stating that my compiler was not recognized. Following this, there was no option to make install, so I had to copy the folder directly. It then failed to compile due to various static_asserts when I included it. I take it clang/osx is not supported at this time. :)

[demianmnave]

Doh! I don't have OS/X to test on, but I can see what happens with clang on Windows (and probably Linux). "make install" isn't done yet, as I use CML2 in my production code directly from a git clone. I'm willing to port over the "make install" step, but I can't guarantee it will work for OS/X.

Let me know what clang version you are using, and I will give it a try. I will also open a new issue so we can track this new development...

[ghost]

Sounds great, thanks!
I'm using version 7.0.2.

These were the errors I encountered when #including <cml/cml.h> if it helps any:
https://gist.github.com/Segmented/5ecb34170679cc93d1047f6d79c3c5ab

I can run the tests for you as well if you want, whenever the makefile target for those exists or if theres another way to run them heh :)

[demianmnave]

Thanks, the error messages were helpful, particularly because none were "showstoppers". In case you didn't get a message about it, I've created a new issue for updating CML2 to build with clang/XCode (#6). Let's continue there.

[demianmnave]

@segmented I'm pinging you here, as I don't know if pinging you from #6 actually works. Let me know whenever you have a change if the latest changes have resolved those errors.

[ghost]

Unfortunately, while it now accepts the int/float combo, matrix_rotation_axis_angle yields an incorrect matrix. I believe this to be due to the traits structure treating the angle parameter as an integer type in cos and sin. For example, if line 56 in traits.h was instead:

template <typename AngleT>
static inline value_type cos(const AngleT& v)

Then the correct matrix is produced for me.

[demianmnave]

@segmented, please give the latest commit a try when you have a chance (demianmnave/CML@ce7ccec), and let me know how it works for you.

[ghost]

Seems to work for me. Thanks!

[demianmnave]

Awesome, thanks for your patience, and for spurring a pretty important update. Please let me know if you have any more questions or problems.

[demianmnave]

Kamino cloned this issue to demianmnave/CML