Management of Rotations (direction and angles)

[plgarcia]

See Forum :
https://forum.freecadweb.org/viewtopic.php?t=23563
and
https://forum.freecadweb.org/viewtopic.php?f=10&t=24662

This change keep track and the direction and the angle that has been set by the user, whatever the value of the angle is given. The quaternon is also evalutated for any calcuation based on them.
When seting a Rotation with the quaternion, the angle chosen is the one in ]-180, 180] what is more convienent in most cases than [0, 360[.

This allow to use angles in formulas without modulos, if -5 * 2 is equivalent to 355 * 2,
-5 /2 is definitively not the same as 355 /2!

Explanation:
Rotation:

• Add a private attribute Vector to store the direction of the rotation, and manage not to erase this direction when the angle id 0.
• Add a private attribute to store the angle as defined (no modulo etc)
• Keep the quaternion for calculations

PropertyGeo:

• Saves the rotation with angle and direction instead of saving the quaternion.
• Attribute name chosen: Ox, Oy and Oz for the coordinates of the axis and A for the angle in radians. This has to be validated.
• Backward compatibility with the saved files with quaternion (test presence of A to determine which of the Quaternion (old way) or the direction and angle is stored (new way). New files can be opened by old FreeCAD and vice-versa.

The only side effect I can imagine is that it was possible to set a vector to 0, 0, 0 if the angle was not 0, what is somehow non sense. Now when setting to 0, 0 0 the last not null vector is kept. The vector can not be null any longer.

Thank you for creating a pull request to contribute to FreeCAD! To ease integration, please confirm the following:

• Branch rebased on latest master git pull --rebase upstream master
• Unit tests confirmed to pass by running ./bin/FreeCAD --run-test 0 (see comment)
• Commit message is well-written
• Commit message includes issue #<id> or fixes #<id> where <id> is the associated MantisBT issue id if one exists

And please remember to update the Wiki with the features added or changed once this PR once it is merged.

---

[plgarcia]

It seems that the run-test error is due to loss of precision when converting from rad to degrees and vice versa:
AssertionError: 0.17453292519943295 != 0.1745329251994329

Error < E-14 degrees! 5E-7 Angstrom at 1 meter. The angle one can see 1 atom at 2000 km.

I updated the test to eliminate these precision problems.

[wwmayer]

Some comments:

1. IMO it's a waste of memory to use three doubles to store the axis direction. One could also simply store the length of the axis and change its sign where needed.

2. When already saving these additional information then it would be good not to normalize the axis. This means when in the property editor the user enters (1,1,1) as axis it should restore this and not change it to (0.57, 0.57, 0.57)

3. This PR does NOT fix issue 3281 at all. When I follow the instructions of the video then the rotation also gets overwritten by the old rotation as soon as the translation vector changes.

[plgarcia]

1 - I aggree it is more memory consuming. But when angle is 0 the vector is completely lost. But there is a way I think to optimize this. 3 double by rotaion make 6 double per part, 24Kb for 1000 parts. Are there rotations linked to other objects (faces, segments, points ?)

2 - Not normalizing the vector is a good idea so no confusion for the user.

3 - I made the test with and without the PR and for me the issue 3281 is solved. I will make more tests.

[plgarcia]

Sorry I changed my mind for the normalization of the axis.
Because I do not know all the details of the software, I am not sure some functions do not rely on having a normalized vector. That is why I implemented the PR this way.
This PR implements a better behavior, and I made some changes to have the vector initialized and not to lose information in all cases. At some point I replaced all commits by one.
I believe the good thing to do is to take the PR as is because it is working, and plan a further optimization and changes.

[wwmayer]

1 - I aggree it is more memory consuming. But when angle is 0 the vector is completely lost. But there is a way I think to optimize this. 3 double by rotaion make 6 double per part, 24Kb for 1000 parts. Are there rotations linked to other objects (faces, segments, points ?)

OK, that's a valid point. Then we can also leave your PR as is. An alternative might be to store axis and angle instead of the q's but the downside is that the q's always need to be recomputed when needed which causes a loss of performance.

2 - Not normalizing the vector is a good idea so no confusion for the user.
Sorry I changed my mind for the normalization of the axis.
Because I do not know all the details of the software, I am not sure some functions do not rely on having a normalized vector. That is why I implemented the PR this way.
This PR implements a better behavior, and I made some changes to have the vector initialized and not to lose information in all cases. At some point I replaced all commits by one.
I believe the good thing to do is to take the PR as is because it is working, and plan a further optimization and changes.

Actually it was never claimed that the returned axis will be normalized and thus client code shouldn't assume it. But nevertheless there is an easy way to avoid any problems: The _axis member should be stored as is and when retrieved via getValue(axis, angle) the reference can be normalized. Then we can have an additional method where we get the axis as is without normalizing it.

[plgarcia]

I believe quaternions are used for transformations everywhere and you are probably right when you say that it is a bad idea to remove them for performances.
I will make the change to store the axis as you proposed. In a different commit, so it will be very easy to select wich of the implementation we want to keep.
Note nevertheless that is some cases the axis has to be evaluated with quaternions and rotations, and in this case it will be normalized. Should we in this case get the length before update and multiply the produced axis by the operation before storing?

[plgarcia]

OK change is made. Just the time to recompile and make some tests. The PR should be ready tomorow.

[wwmayer]

3 - I made the test with and without the PR and for me the issue 3281 is solved. I will make more tests.

As said above this PR doesn't fix issue 3281. When testing the issue it's important not to de-select the object after using the axis cross for rotation because then internal values are set correctly and the whole test is useless.

The issue is caused by the implementation of PropertyPlacementItem::assignProperty which computes the cross product of the internally stored axis and that of the rotation. If the cross product is the null vector then the two axes are parallel but they can still have different orientation. So, it's not enough to only check for the length of the cross product but also the dot product must be checked and if this is negative the internally stored axis must be flipped.

[plgarcia]

yes you are right.

[plgarcia]

Templates do not take the un-normalized axis. I do not think it is important, but a difference is detected during tests.
I do not know anything arround templates so I have to analyse to uderstand what to do.

[plgarcia]

OK the problem is solved. The templates were only built with quaternion. Now they can be made still with quaternion but also with axis and angle with, if both defined a priority the the axis and angle definition.

[plgarcia]

Note that even if I defined the function :
void getValueNormalized(Vector3d & axis, double & rfAngle) const;

It is not used yet, and there is no capability defined to use it in python.

[wwmayer]

Note that even if I defined the function :
void getValueNormalized(Vector3d & axis, double & rfAngle) const;

I rather meant the opposite: make sure that getValue(Vector&, double) normalizes the axis and then offer a second method which returns the axis as is. This is to avoid to change behaviour and because in the Path module there is indeed assumed that the length is 1 and now the unit tests fail because of this.

For now I merged your first and second commit and reworked the third one.

[plgarcia]

When I tested I must say that is quite peasent to find the values in the GUI. 1 1 1 is still 1 1 1.
A future evolution of the gui I beleive.

Thanks for having taken the time for that!