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

Fix weird Quaternion default initialisation #291

Closed
cniethammer opened this issue Feb 21, 2024 · 5 comments · Fixed by #300
Closed

Fix weird Quaternion default initialisation #291

cniethammer opened this issue Feb 21, 2024 · 5 comments · Fixed by #300
Assignees
Labels
bug Something isn't working enhancement New feature or request

Comments

@cniethammer
Copy link
Contributor

Describe the bug
The Quaternion default initialization behavior was changed in 20a6b8f from 0 + 0i + 0j + 0k (Q(w=0, x=0, y=0, z=0)) to 1 + 1i + 0j + 0k (Q(w=1, x=1, y=0, z=0)). This is neither the identity quaternion 1 + 0i + 0j + 0k (Q(1,0,0,0)) nor represents a pure rotation as it is not normalized.

I would argue that we should fix this by returning to the original Q(0,0,0,0) default initialization, which is also used e.g. in frameworks like Unity (see. https://github.com/HardlyDifficult/Tutorials/blob/8a64088703065f8c6c3e8b198f987a007ca2ef23/Quaternions.md#331-quaternion-constructors)

Note: Initializing to the invalid quaternion Q(0,0,0,0) can improve code performance in comparison to the (intended?) identity quaternion Q(1,0,0,0). A simple benchmark that just allocates a vector of 100.000 default initialized quaternions compiled with recent gcc 13.2.1 and -O3 on my system gives for example:

-----------------------------------------------------------
Benchmark                 Time             CPU   Iterations
-----------------------------------------------------------
BM_Q_with_init_1      90870 ns        90778 ns         7507
BM_Q_with_init_0      81763 ns        81659 ns         7665
@cniethammer cniethammer added bug Something isn't working enhancement New feature or request labels Feb 21, 2024
@cniethammer cniethammer self-assigned this Mar 6, 2024
@HomesGH HomesGH changed the title Fix wired Quaternion default initialisation Fix weird Quaternion default initialisation Mar 20, 2024
@HomesGH
Copy link
Contributor

HomesGH commented Mar 20, 2024

I would recommend to choose the identity quaternion 1 + 0i + 0j + 0k as default. This default is used, e.g., by the CubicGridGenerator via the ParticleCellBase.
This is meaningful and does not break the code in contrast to 0 + 0i + 0j + 0k. The latter will fail during the normalization routine (at least for DEBUG builds).

Furthermore, it should be necessary to normalize the quaternions from time to time due to numerical errors during the simulation. I am not sure if this is done in our code or at least when reading/writing a checkpoint.

@HomesGH
Copy link
Contributor

HomesGH commented Mar 20, 2024

I would argue that we should fix this by returning to the original Q(0,0,0,0) default initialization, which is also used e.g. in frameworks like Unity (see. https://github.com/HardlyDifficult/Tutorials/blob/8a64088703065f8c6c3e8b198f987a007ca2ef23/Quaternions.md#331-quaternion-constructors)

I could not find the Q(0,0,0,0) on your linked Unity constructors page. Instead, it says:

"Quaternion rotations must be normalized, meaning: [...]"

and (note the last 1):

Quaternion identity = new Quaternion(0, 0, 0, 1);

@cniethammer
Copy link
Contributor Author

@HomesGH See section 3.3.1 following the link: "Note that the 'default' Quaternion is not a valid rotation, and may not be used with any rotation method: ..."

@cniethammer
Copy link
Contributor Author

I went now a bit deeper into the internalGenerator code and things around it.

I must say I am really surprised that this was implemented within the Datastructure code.
The documentation is either missing or bad - the top readXML seems just a copy of the internal TcTs generator one with minor modifications.

From my point. The problem that was fixed by changing the Quaternion and Molecule constructor defaults should have been adressed in the generator code by calling the Molecule constructor with the right arguments originally.

@HomesGH
Copy link
Contributor

HomesGH commented Mar 21, 2024

Ok, I see. Thank you for the citation!

I fully agree that the way the CubicGridGenerator works is weird. It should not use methods of the data structure classes.

@HomesGH HomesGH linked a pull request Jun 5, 2024 that will close this issue
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants