Make rotation functions take unit axes

[legends2k]

Excerpt from #206 discussion:

Having APIs not perform normalization means the application code can perform it if needed; if application already has a unit quaternion, it can simply feed them to the API. However, if the API always normalizes, and the application already has unit quaternions, the normalization step by API is needless and wastes CPU cycles by normalizing an already unit vector.

Just expecting unit quaternions and leaving option of normalizing to the application seems like the optimal choice

Following functions normalize incoming vector/quaternion/matrix:

No.	Function	File
1	glm_vec3_rotate	vec3.h
2	glm_vec3_rotate_m3	vec3.h
3	glm_vec3_rotate_m4	vec3.h
4	glm_rotate_make	affine.h
5	glm_quatv	quat.h
6	glm_quat_rotatev	quat.h
7	glm_quat_for	quat.h
8	glm_quat_forp	quat.h

These should be changed to take unit vectors i.e. avoid normalizing input vector/axis and document that the input is expected to be normalized. This gives the application programmer choice to avoid the normalization step if s/he already has a normalized vector in app side.

[legends2k]

Discussion with @recp on #206:

Just expecting unit quaternions and leaving option of normalizing to the application seems like the optimal choice to me. This doesn't waste cycles needlessly.

I like your approach, let's make expectations about axes: always unit.

glm_vec3_rotate's documentation says must be unit vector but implementation calls glm_vec3_normalize_to(axis, k) 🤕

😲 🤦🏾

Based on the above rationale of giving the application the choice to normalize or skip, I'd suggest we should simply expect unit axis in all 4 functions and drop the normalize_to calls. Please let me know what you think.

Let's do it.

[recp]

@legends2k sorry for the delay, I was bussy for a while,

I hope this won't break existing projects, on the other hand this will improve performance a bit. Another options would be provide inline alternatives which do normalization then call related func but I'm not sure if this is a good idea or not.

[legends2k]

@recp That's okay.

This might break existing projects expecting cglm to normalize as they've come to expect with earlier versions. One option is to introduce unit vector expecting variants with a unit prefix e.g. glm_vec3_rotate_unit_axis (and leave glm_vec3_rotate alone). Another (cleaner) option is to call out the breaking change in the changelog/release note and make the change without introducing variants. I prefer the latter, of course, we should respect others' opinions/feedback too.

Note: I found 4 more functions which assume an unnormalized input; added it to the very first comment.

[recp]

@legends2k many thanks, let's get a few more feedbacks if there are any, then we can move on,

Third option could be bring an option for a while (for smooth transition to unit input or it could always stay there):

#define CLGM_EXPECT_UNIT_AXES

Like:

CGLM_INLINE
void
glm_vec3_rotate(vec3 v, float angle, vec3 axis) {
  vec3   v1, v2, k;
  float  c, s;

  c = cosf(angle);
  s = sinf(angle);

#ifndef CLGM_EXPECT_UNIT_AXES
  glm_vec3_normalize_to(axis, k); 
#else
  glm_vec3_copy(axis, k); /* TODO: eliminate copy? */
#endif

  /* Right Hand, Rodrigues' rotation formula:
        v = v*cos(t) + (kxv)sin(t) + k*(k.v)(1 - cos(t))
   */
  glm_vec3_scale(v, c, v1);

  glm_vec3_cross(k, v, v2);
  glm_vec3_scale(v2, s, v2);

  glm_vec3_add(v1, v2, v1);

  glm_vec3_scale(k, glm_vec3_dot(k, v) * (1.0f - c), v2);
  glm_vec3_add(v1, v2, v);
}

what do you think about the option? The drawback of this option is that compiled functions glmc_ may conflict with inline versions if the option is not used homogeneously, but that is expected behavior of course

[legends2k]

@recp I like the CGLM_EXPECT_UNIT_AXES approach. Of course, this macro should be defined to the same value for both included and compiled sources. Since common.h (or types.h) is included in all sources (both compiled and header-only), we can supply a default value to CGLM_EXPECT_UNIT_AXES there, if not already defined.

An application using cglm can define CGLM_EXPECT_UNIT_AXES to a different value either via its build system or in a source before all cglm headers are included. This will make sure all headers see the same value for the macro.

This macro would help deprecate and remove the non-unit axis approach in future versions gradually.

[recp]

@legends2k great! Let's implement CGLM_EXPECT_UNIT_AXES then. A PR would be awesome ;) but I can either do that