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

euler update: use Ken Shoemake's algorithm (gemsiv/euler_angle) #128

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

recp
Copy link
Owner

@recp recp commented Mar 22, 2020

Ken Shoemake's algorithm seems to be most common one, it also handles 24 different permutations of rotation order. This must be more robust and flexible api.

I was sent an email to @erich666 (~2018 I guess) to get permission to include GraphicsGems's euler code in this repo (with a few modifications) by giving a reference to original source code.

I added this to top of cglm/euler.h header:

/* ---------- Notice for Ken Shoemake's algorithm Implementation -------------*
 | Ken Shoemake's algorithm impl. is taken from this repo by permission:      |
 |   https://github.com/erich666/GraphicsGems/blob/master/gemsiv/euler_angle  |
 |                                                                            |
 | cglm doesn't claim the ownership of GraphicsGems source codes              |
 | and the algorithm itself. But cglm may change variable names or some piece |
 | of codes in order to apply optimizations or to make it usable in cglm.     |
 |                                                                            |
 | Related issue: https://github.com/recp/cglm/issues/30                      |
 |                                                                            |
 * -------------------------- GraphicsGems EULA ----------------------------- *
 | Related EULA for GraphicsGems can be found at below, plus in CREDITS:      |
 |   http://www.realtimerendering.com/resources/GraphicsGems/                 |
 |                                                                            |
 | EULA: The Graphics Gems code is copyright-protected. In other words, you   |
 | cannot claim the text of the code as your own and resell it. Using the     |
 | code is permitted in any program, product, or library, non-commercial or   |
 | commercial. Giving credit is not required, though is a nice gesture.       |
 | The code comes as-is, and if there are any flaws or problems with any Gems |
 | code, nobody involved with Gems - authors, editors, publishers, or         |
 | webmasters - are to be held responsible. Basically, don't be a jerk, and   |
 | remember that anything free comes with no guarantee.                       |
 * -------------------------------- END --------------------------------------*/

it says that Giving credit is not required, though is a nice gesture. so cglm gives credit in both header and CREDITS file.

The old Euler API probably will be deprecated if anyone will not complain for that.


cglm applies a few changes for names. The api will be like this:

  1. void glm_eul_mat4(vec3 ea, int order, mat4 dest)

Euler order macros:

/*! Static axes */
#define GLM_EUL_XYZs    EulOrd(0,EulParEven,EulRepNo,EulFrmS)
#define GLM_EUL_XYXs    EulOrd(0,EulParEven,EulRepYes,EulFrmS)
#define GLM_EUL_XZYs    EulOrd(0,EulParOdd,EulRepNo,EulFrmS)
#define GLM_EUL_XZXs    EulOrd(0,EulParOdd,EulRepYes,EulFrmS)
#define GLM_EUL_YZXs    EulOrd(1,EulParEven,EulRepNo,EulFrmS)
#define GLM_EUL_YZYs    EulOrd(1,EulParEven,EulRepYes,EulFrmS)
#define GLM_EUL_YXZs    EulOrd(1,EulParOdd,EulRepNo,EulFrmS)
#define GLM_EUL_YXYs    EulOrd(1,EulParOdd,EulRepYes,EulFrmS)
#define GLM_EUL_ZXYs    EulOrd(2,EulParEven,EulRepNo,EulFrmS)
#define GLM_EUL_ZXZs    EulOrd(2,EulParEven,EulRepYes,EulFrmS)
#define GLM_EUL_ZYXs    EulOrd(2,EulParOdd,EulRepNo,EulFrmS)
#define GLM_EUL_ZYZs    EulOrd(2,EulParOdd,EulRepYes,EulFrmS)

/*! Rotating axes */
#define GLM_EUL_ZYXr    EulOrd(0,EulParEven,EulRepNo,EulFrmR)
#define GLM_EUL_XYXr    EulOrd(0,EulParEven,EulRepYes,EulFrmR)
#define GLM_EUL_YZXr    EulOrd(0,EulParOdd,EulRepNo,EulFrmR)
#define GLM_EUL_XZXr    EulOrd(0,EulParOdd,EulRepYes,EulFrmR)
#define GLM_EUL_XZYr    EulOrd(1,EulParEven,EulRepNo,EulFrmR)
#define GLM_EUL_YZYr    EulOrd(1,EulParEven,EulRepYes,EulFrmR)
#define GLM_EUL_ZXYr    EulOrd(1,EulParOdd,EulRepNo,EulFrmR)
#define GLM_EUL_YXYr    EulOrd(1,EulParOdd,EulRepYes,EulFrmR)
#define GLM_EUL_YXZr    EulOrd(2,EulParEven,EulRepNo,EulFrmR)
#define GLM_EUL_ZXZr    EulOrd(2,EulParEven,EulRepYes,EulFrmR)
#define GLM_EUL_XYZr    EulOrd(2,EulParOdd,EulRepNo,EulFrmR)
#define GLM_EUL_ZYZr    EulOrd(2,EulParOdd,EulRepYes,EulFrmR)

I want to make EulOrd(2,EulParOdd,EulRepYes,EulFrmR) pre-calculated in order to remove a few macro definitions

Related issue: #30

@recp recp changed the title use Ken Shoemake's algorithm (gemsiv/euler_angle) Euler Update: use Ken Shoemake's algorithm (gemsiv/euler_angle) Mar 22, 2020
@recp recp changed the title Euler Update: use Ken Shoemake's algorithm (gemsiv/euler_angle) euler update: use Ken Shoemake's algorithm (gemsiv/euler_angle) Mar 22, 2020
@recp recp self-assigned this Mar 22, 2020
Copy link

@erich666 erich666 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, and it would be helpful if you described a bit why you made these changes. It looks like a bit of cleanup, which I appreciate. Is there more to it?

I am attempting to contact Ken Shoemake about this code to have him review it.

@erich666
Copy link

erich666 commented Apr 8, 2020

I noticed that Codacy says:

Variable 'h' is assigned a value that is never used.

EulGetOrd(order, i, j, k, h, parOdd, repYes, frmR);

This would be nice to clean up,

@recp
Copy link
Owner Author

recp commented Apr 9, 2020

@erich666 thanks for review this PR,

Thanks, and it would be helpful if you described a bit why you made these changes. It looks like a bit of cleanup, which I appreciate. Is there more to it?

The main purposes are:

1. Readability:

I think, defining the order as enum WITH PRE-CALCULATED VALUES is more readable and easy to debug, debugger can show enum for parameter...

typedef enum glm_eul_order { 
   /*! Static axes */
   GLM_EUL_XYZs  = 0,
   GLM_EUL_XYXs  = 2,
   ...
} glm_eul_order;

/* vs */

/*! Static axes */
#define GLM_EUL_XYZs    EulOrd(0,EulParEven,EulRepNo,EulFrmS)
#define GLM_EUL_XYXs    EulOrd(0,EulParEven,EulRepYes,EulFrmS)
...

I have moved some macros to inside function like parOdd, repYes, frmR as variable to remove macros. Actually I like macros, but I wanted to keep things simpler and neat.

2. Code style:

cglm has its own coding style, so I changed spaces, new lines (and names maybe)...

3. Reusability:

For instance cglm has inline functions like glm_vec3_negate() and glm_swapf() so I used them to re-use existing functions. Almost all vec4 and mat4 types/functions are highly optimized in cglm with SIMD, in the future I want to optimize vec3/mat3 too, so this improvements will affect all places where they are used...

if (n==EulParOdd) {ea.x = -ea.x; ea.y = -ea.y; ea.z = -ea.z;}

/* this line is converted to: */

if (parOdd == 1)
   glm_vec3_negate(ea);

4. Code Conflicts / Name Resolve:

All things in cglm must start with glm_, cglm, GLM, or CGLM... except type names maybe. Because other frameworks, libraries may used same name, or even same codes..., so target project cannot be compiled correctly.

For instance Windows defined near and far and we cannot use it directly after windows header included. I don't want to do like that.


There are a few more things I want to do:

1. Separate Static and Rotating axes

The algorithm is great and it can handle Static and Rotating axes, plus it can handle extrinsic and intrinsics rotations. One function can provide all the functionalities.

But if users only want to Static axes and don't want to use Rotating axes then a single function will come with redundant statements. We can eliminate the redundant overhead by separate functionalities for its purpose.

For instance,

a. glm_eul_mat4() original algorithm (24 possible rotations are allowed)
b. glm_eul_mat4r() original algorithm that only supports Rotating axes (12 possible rotations are allowed)
c. glm_eul_mat4s() original algorithm that only supports Static axes (12 possible rotations are allowed)

we can even split intrinsics and extrinsic rotations e.g. XYX | XYZ ...

the main goal is here is to split functionalities to keep function and generated CPU instructions small as possible to improve performance for specific needs and keep binary small. Because all functions in cglm that start with glm_ are inline and all functions that starts with glmc_ are library called (pre-compiled). Using inline function in multiple places may increase binary size... cglm provides both and inline versions almost for all functions.

2. mat3 support

I want to provide

a. Euler to mat4
b. Euler to mat3
c. mat4 to Euler
d. mat3 to Euler
e. Euler to Quat
f. Quat to Euler

actually I will duplicate mat4 implementations for mat3 to provide these functionalities for mat3 too.

3. Optimizations (todo)

In the future SIMD can be implemented here but currently it seems we don't need it for now. In the future target architecture specific optimizations may be applied.

@recp
Copy link
Owner Author

recp commented Apr 9, 2020

I noticed that Codacy says:
Variable 'h' is assigned a value that is never used.
EulGetOrd(order, i, j, k, h, parOdd, repYes, frmR);
This would be nice to clean up,

yes IIRC, I also got a compiler warning for that. I have double checked the GraphicsGems/gemsiv/euler_angle/EulerAngles.c file and it seems h is not used in there too. So I would like to remove that param :)

@erich666
Copy link

erich666 commented Apr 9, 2020

Wow, thanks for the extensive rundown! Really, a few sentences would have been enough for me, but I honestly appreciate the full explanation for anyone that looks it over later.

So I would like to remove that param :)

Yes, please! We've cleaned out pointless code in the past in other gems. I'll wait for that addition and then give a full review. So far I have not been able to track down Ken Shoemake.

In searching, I did find this, https://math.stackexchange.com/questions/3444267/extracting-euler-angles-from-quaternion-close-to-singularity, which should interest you. Perhaps the Mike Day article is worth referencing in a comment in the code? This seems like useful knowledge for someone using this conversion. Better yet, if you're willing, perhaps fold his fix into the code, possibly with a #define/#ifdef?

@recp
Copy link
Owner Author

recp commented Apr 9, 2020

I'll wait for that addition and then give a full review

@erich666 that would be perfect 🤗

In searching, I did find this, https://math.stackexchange.com/questions/3444267/extracting-euler-angles-from-quaternion-close-to-singularity, which should interest you. Perhaps the Mike Day article is worth referencing in a comment in the code? This seems like useful knowledge for someone using this conversion. Better yet, if you're willing, perhaps fold his fix into the code, possibly with a #define/#ifdef?

many thanks for these resources 🤗, I'll check them later. I need to work on other project[s] for now, I'll back to this PR asap 🤔


FWIW, I was used this resource to implement Euler in cglm:
https://www.geometrictools.com/Documentation/EulerAngles.pdf

all glm_euler_xyz(), glm_euler_yxz()... functions are implementation of this document. I was unable to determine that if these implementations are intrinsic or extrinsic; static or rotating axes... for now. I'll try to learn more about them later.


So I was thought that Ken Shoemake's algorithm must be robust and it allows 24 different rotations... with s and r suffixes it explains that it is rotating or it is static... So I wanted to make old euler api deprecated. Maybe old api can be stay, I don't know. I'll add some tests to compare them this this api

Also one nice comment in erich666/GraphicsGems/blob/master/gemsiv/euler_angle/QuatTypes.h is

typedef float HMatrix[4][4]; /* Right-handed, for column vectors */

this convention is same as cglm. I found some euler resources, but I was unable to know if they use Right Hand rule or Left, Column vector vs Row...

@jljusten
Copy link

Would cglm continue to say the project license is MIT if this was merged? Or, would the project license become MIT and the EULA?

It doesn't sound like the EULA allows the new code to be distributed under the MIT license.

@recp
Copy link
Owner Author

recp commented Apr 30, 2020

Hi @jljusten good points,

Would cglm continue to say the project license is MIT if this was merged? Or, would the project license become MIT and the EULA?

The license must be MIT license and clear.

It doesn't sound like the EULA allows the new code to be distributed under the MIT license.

we must investigate this issue.

/cc @erich666

@erich666
Copy link

Would cglm continue to say the project license is MIT if this was merged? Or, would the project license become MIT and the EULA?

It doesn't sound like the EULA allows the new code to be distributed under the MIT license.

They sound essentially identical to me; I helped write the EULA and tracked down the authors. We came up with the Graphics Gems EULA at a time when the MIT license didn't exist, otherwise we would have likely adopted it. We do use the MIT license for code in the Journal of Graphics Tools, which is the successor to the Graphics Gems series.

Some lawyer somewhere with nothing to do could probably find some subtle difference, but given that both licenses are highly unrestricted, it seems a non-concern. The main difference I see is that you can't take the Graphics Gems code and put it in a book, claim it's your own code, and sell it. This restriction is there automatically anyhow, since all the code was published in a book. We just made it explicit, rehashing a small part of U.S. copyright law. (Way back when there was a case of someone taking another's code and foisting it off as their own.)

Compare for yourself.

Graphics Gems EULA: The Graphics Gems code is copyright-protected. In other words, you cannot claim the text of the code as your own and resell it. Using the code is permitted in any program, product, or library, non-commercial or commercial. Giving credit is not required, though is a nice gesture. The code comes as-is, and if there are any flaws or problems with any Gems code, nobody involved with Gems - authors, editors, publishers, or webmasters - are to be held responsible. Basically, don't be a jerk, and remember that anything free comes with no guarantee.

MIT License: Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.

So, long and short, I think we simply say in the code added that "this portion of the code is under the MIT License." If someone feels that the code's license is then too complex for them and need a lawyer to untangle it, ah well. Me, I'll say this: it's pretty unimaginable that anyone involved in Graphics Gems is going to go after anyone reusing this code. I'm not. Andrew Glassner (the series editor) is not - I checked with him just now. Academic Press has zero memory this code exists. Ken Shoemake (code's author) can't even be found.

@recp
Copy link
Owner Author

recp commented Apr 30, 2020

@erich666 many thanks for quick and detailed response,

So, we can use this codes (with small modifications) in MIT-Licensed cglm repository by providing link to original source codes, book and providing EULA? I hope it won't break MIT.

The EULA is already added top of cglm/euler.h file, we can append it to LICENSE file too if both are not conflicting.

cglm provides highly optimized, readable, tested (more tests will be written later) and well designed algorithms so it would be very nice to have this euler algorithm.

@erich666
Copy link

If it breaks MIT I'll go give them a talking to - they're two miles away. I give them $50 a year because they host the MIT Mystery Hunt, so I clearly have tons of clout.

Oh, you meant the MIT License. Like I say, put the license where you think it makes sense.

@recp
Copy link
Owner Author

recp commented Apr 30, 2020

Oh, you meant the MIT License. Like I say, put the license where you think it makes sense.

Thanks 🤗

@jljusten
Copy link

Would cglm continue to say the project license is MIT if this was merged? Or, would the project license become MIT and the EULA?
It doesn't sound like the EULA allows the new code to be distributed under the MIT license.

They sound essentially identical to me;

I know what you are trying to convey, but as I'm not a lawyer, and the license text is not the same, then it seems like it is a different license. So, one concern is that, even if it was not a restrictive license, it seems like a 2nd license that a user/distributor of cglm would have to consider the terms on. It is also not a "well known" license, like MIT, so users will also have the "think" more about it.

After open source started gaining in popularity, it seemed like every company decided to gather up a bunch of lawyers, and craft a nifty new open source license which best served their needs. This lead to many, many complicated "open source" licenses. One effort to deal with this issue was the creation of the open source definition. https://opensource.org/osd-annotated

This laid down some rules about what the freedom of open source was mean to provide. While this doesn't prevent the creation of new licenses, it does give it some guidelines, and probably makes it less interesting for a company to try to invent yet another one.

But, beyond whether a license meets the the open source definition, for each additional license, it just makes that much more effort for distributors and end-users. So, it is nice to try to limit the number in the software for that reason.

I helped write the EULA and tracked down the authors. We came up with the Graphics Gems EULA at a time when the MIT license didn't exist, otherwise we would have likely adopted it. We do use the MIT license for code in the Journal of Graphics Tools, which is the successor to the Graphics Gems series.

Some lawyer somewhere with nothing to do could probably find some subtle difference, but given that both licenses are highly unrestricted, it seems a non-concern.

Once again, I'm not a lawyer, but in comparing the EULA to the open source definition, https://opensource.org/osd-annotated, I had a couple comments.

One thing I noticed in the license text is the part about not being a jerk. So, now I have to wonder if I'm being a jerk. :)

Anyway, I wonder if that could be argued to "discriminate" against some people. The reason I mention that is because of point 5 of the open source definition (No Discrimination Against Persons or Groups).

Jerk also seems like a pretty subjective qualification :), which is maybe not a good idea for a license.

Another concern could be the "EULA" terminology. Doesn't "EU" there mean "end user"? So, does this license only apply to users? What about distributors? In that case, it might not meet the "Free Redistribution" point of the open source definition.

Regarding the part where you enumerate that it can be used in any program or library, but it seems to stop short of allowing it to be used for any purpose. One could interpret that as "Discrimination Against Fields of Endeavor" in the open source definition.

I think you mentioned that the part about not being able to re-sell the code is to prevent others from publishing the code in a book to make money off it. But, in the open source definition, I think the "Free Redistribution" is more about freedom, and it specifically mentions not restricting the sale of the software.

Now, I'll give my personal opinion about sample code like this. I think it is great when it is distributed as public domain, as I see in https://github.com/erich666/GraphicsGems/blob/master/gems/README.

This is very nice for re-use because it can be used under any license whatsoever.

I think that nice, non-jerks :) like @recp still love to give credit to the original author. In fact, I already see such a reference in the cglm code.

But, I completely understand where you are coming from in wanting to prevent another book from selling the code. Unfortunately, it seems like the freedom to do that is one of the core parts of the open source definition.

So, long and short, I think we simply say in the code added that "this portion of the code is under the MIT License." If someone feels that the code's license is then too complex for them and need a lawyer to untangle it, ah well. Me, I'll say this: it's pretty unimaginable that anyone involved in Graphics Gems is going to go after anyone reusing this code. I'm not. Andrew Glassner (the series editor) is not - I checked with him just now. Academic Press has zero memory this code exists. Ken Shoemake (code's author) can't even be found.

I hope most open source projects don't get into the realm of, "Am I likely to be sued or not". :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants