Dual quaternions by nanjizal · Pull Request #64 · tbrosman/hxmath

nanjizal · 2020-05-19T04:03:19Z

Just current Dual Quaternion changes

- Invert formula was wrong - Multiplication re-ordered terms (quaternion multiplication is not commutative) - addWith subtracted Also added more tests.

Fixes for DualQuaternion

nanjizal · 2020-05-19T04:07:20Z

Still WIP no additional testing yet added.

nanjizal · 2020-05-19T04:11:04Z

Stuff requested I action:

Cool, I'll try to add more documentation/references when I get time to work on this again. This is one of the more useful papers I found while refreshing my brain: https://www.cs.utah.edu/~ladislav/kavan06dual/kavan06dual.pdf It goes over how to set up the product for transforming points.

In the meantime there are a few things this PR needs:

Code style should match the rest of the library
Test for Quaternion.fromYawPitchRoll
Test for DualQuaternion.fromAxisAngle, fromTranslation (should match result of getTranslation())
Test for DualQuaternion.identity
One more thing I keep forgetting: if I include your name in contributors, haxelib associates the library with your username. It is easier for me to just credit you in the CHANGELOG.md (and class file). I found this out the hard way a year ago when someone updated my Haxe REST Client library and I tried to submit the change. Basically haxelib doesn't differentiate contributors and owners: https://lib.haxe.org/documentation/faq/

nanjizal · 2020-05-19T09:03:23Z

Getting a strange edge case which I have omitted for now.
eee2323#diff-d019ead88319c7f5c2e86f45914ddc77R163
Still some more tests to add. For moment I have put all Quaternions stuff in new Test class but can move it back to Test3D once I am happier with it.

nanjizal · 2020-05-20T11:54:20Z

@tbrosman Are these tests suitable or does it need more coverage? I am not yet sure how to deal with the one odd failure, I expect styling needs improvement I can move it back into Test3D and tidy styling if you feel the tests are suitable, and take another look at the edge case.

nanjizal · 2020-05-20T11:56:58Z

Had quite a few problems with pushing to specific branches, but my current changes should all be there.

nanjizal · 2020-05-20T13:09:49Z

I was going to ask do you use a plugin or something to apply styles, do you use the 'recommended' haxe styling, I think that has a plugin or similar but not tried it yet.

tbrosman · 2020-05-23T18:09:34Z

Test.hx

        #if (sys && !EXIT_ON_FINISH)
        Sys.stdin().readLine();
-        #else
+        #elseif !js


Should be #elseif sys. I missed that case when I wrote the test driver.

tbrosman · 2020-05-23T18:12:40Z

hxmath/math/DualQuaternion.hx

+     * @param t             The Vector4 ( translation )
+     * @return              The DualQuaternion
+     */
+    public static inline function fromAxisAngle( angleDegrees:Float


Minor: should match existing function style (in this case just put it all on the same line)

Spacing below should also match existing.

tbrosman · 2020-05-23T18:13:07Z

hxmath/math/DualQuaternion.hx

+     */
+    public static inline function fromAxisAngle( angleDegrees:Float
+                                                , axis: Vector3
+                                                , vec4: Vector4 ):DualQuaternion 


Probably should be "t" or "translation" or something (also update documentation to match).

tbrosman · 2020-05-23T18:14:22Z

hxmath/math/DualQuaternion.hx

+    {
+        var cosHalfAngle = Math.cos(theta / 2.0);
+        var sinHalfAngle = Math.sin(theta / 2.0);
+        var real = new Quaternion(cosHalfAngle,sinHalfAngle * axis.x,sinHalfAngle * axis.y,


This should live in Quaternion. Then you can call Quaternion.fromAxisRadian.

tbrosman · 2020-05-23T18:18:37Z

hxmath/math/DualQuaternion.hx

+     * @param a
+     * @return       ~a
+     */
+    @:op(~A)


Like I said in my previous comment, this might not make the most sense for choice of conjugate. Here's my reasoning:

Rotation by a unit length quaternion q is calculated as q * v * (~q) (where ~q is the complex conjugate).

With this convention, transformation by a dual quaternion q would be written q * v * (~q.dualConjugate()) which is kind of gross (also, dualConjugate doesn't exist yet, so you can't actually write this).

tbrosman · 2020-05-23T18:21:01Z

hxmath/math/DualQuaternion.hx

+    public inline function applyScale(s:Float):DualQuaternion
+    {
+        var cloned = clone();
+        cloned = cloned*DualQuaternion.scalar( s );


Why not just call scaleMultiply? Unless scalar() is inlined, this will create an intermediate DualQuaternion.

tbrosman · 2020-05-23T18:23:42Z

hxmath/math/DualQuaternion.hx

+
+    private inline function get_normal():DualQuaternion
+    {
+        var mag = dot(this, this);


This isn't actually the magnitude, but the magnitude squared. Just call .length.

tbrosman · 2020-05-23T18:26:45Z

hxmath/math/DualQuaternion.hx

+    private inline function get_normal():DualQuaternion
+    {
+        var mag = dot(this, this);
+        if( mag < 0 ) return null;


This condition is impossible to hit. Dot products are always >= 0 by definition (if it isn't, it's not a dot product). I think you want to check for when mag == 0. In that particular case you should just return the existing dual quaternion (it is the zero dual quaternion).

tbrosman · 2020-05-23T18:28:13Z

hxmath/math/DualQuaternion.hx

+    public inline function normalize(): DualQuaternion
+    {
+        var mag = dot( this, this );
+        if( mag < 0 ) return null;


Duplicates code with get_normal. Suggestion: call .clone().normalize() in get_normal. I actually don't do this correctly in my Quaternion code, so normal does fail on 0-length quaternions (oops).

tbrosman · 2020-05-23T18:30:39Z

hxmath/math/DualQuaternion.hx

+     */
+    public inline function rotate(u:Vector3):Vector3
+    {
+        normalize();


Quaternion doesn't implicitly normalize on rotate, so neither should DualQuaternion. It is up to the caller to make the trade-off between precision and speed.

tbrosman · 2020-05-23T18:31:48Z

hxmath/math/DualQuaternion.hx

+     * @param b     The other quaternion.
+     * @return      The arccosine angle between this vector and the other in radians.
+     */
+    public inline function angleWith(b:Quaternion):Float


This seems unnecessary. There's an angleWith on Quaternion, so the caller can just call dualQuaternion.real.angleWith(quaternion).

tbrosman · 2020-05-23T18:32:45Z

hxmath/math/DualQuaternion.hx

+     * 
+     * @return  The modified object.
+     */
+    public inline function applyDualConjugate():DualQuaternion


Reminder: needs to be implemented.

tbrosman · 2020-05-23T18:34:17Z

hxmath/math/Quaternion.hx

+// yaw (Z), pitch (Y), roll (X)
+@:forward
+abstract Euler( Vector3 ) from Vector3 to Vector3 {
+    public inline function new( roll: Float, pitch: Float, yaw: Float ){


Minor: order fields like existing structures (properties on top, make setter/getter implementations private and put them on the bottom).

tbrosman · 2020-05-23T18:36:35Z

test/TestStress.hx

 import hxmath.math.Matrix4x4;
 import hxmath.math.Vector2;
-
+#if !js


Should be #if sys

tbrosman · 2020-05-23T18:39:20Z

test_all_mac.hxml

@@ -0,0 +1,36 @@
+-cmd echo 'js test'


Is it not possible to reference hxml here? I admit I've never run this on a Mac. What's the difference? I see a lot of cd'ing into directories that doesn't happen on the "regular" hxml targets. Is it something to do with the path convention for running binaries/scripts?

tbrosman · 2020-05-23T18:44:36Z

Looking much better. I left a few more comments, mostly cleanup stuff (and the conjugate operator needs some work).

I'm not using haxecheckstyle or anything (I really should). One day I'll learn how to configure it to look the way I want it to.

nanjizal and others added 12 commits May 7, 2020 04:04

add DualQuaternion initial implementation

adcff5f

add YawPitchRoll feature to Quaternion

43f6dde

add euler

c2f7069

add euler

3649d51

added some tests

893987e

added some tests

f20cf27

add another test

2cd58a9

Fixed a few issues:

b99c2c8

- Invert formula was wrong - Multiplication re-ordered terms (quaternion multiplication is not commutative) - addWith subtracted Also added more tests.

Minor: rename

5d58134

Merge pull request #1 from tbrosman/dquat

f1c2e0d

Fixes for DualQuaternion

ammends

93015ae

ammends

678e3c7

nanjizal mentioned this pull request May 19, 2020

DualQuaternion implementation and YawPitchRoll and euler addition to Quaternion. #60

Closed

working on tests, special case hidden for moment

eee2323

nanjizal force-pushed the dualQuaternions branch from eee2323 to 678e3c7 Compare May 20, 2020 03:56

nanjizal added 2 commits May 20, 2020 05:24

more tests..

fc56889

more tests....

660fd2b

tbrosman reviewed May 23, 2020

View reviewed changes

test/TestStress.hx

import hxmath.math.Matrix4x4;

import hxmath.math.Vector2;

#if !js

Copy link

Owner

tbrosman May 23, 2020

Choose a reason for hiding this comment

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

Should be #if sys

tbrosman reviewed May 23, 2020

View reviewed changes

Conversation

nanjizal commented May 19, 2020

Uh oh!

nanjizal commented May 19, 2020

Uh oh!

nanjizal commented May 19, 2020

Uh oh!

nanjizal commented May 19, 2020

Uh oh!

nanjizal commented May 20, 2020

Uh oh!

nanjizal commented May 20, 2020

Uh oh!

nanjizal commented May 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbrosman commented May 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants