Fix VRPN, improve tracker layout #146

chris-hld · 2019-07-15T13:11:13Z

This includes #140.

This PR fixes the VRPN tracker.

Furthermore, proposing a more uniform tracker data handling/layout across each implementation.
This is up for discussion, there are some open points, e.g. handling of YPR vs quaternions (What if a library doesn't provide quaternions?).

The basic idea is to augment the Tracker class, from which all trackers inherit.
It forces some layout by pure virtual functions.
Additionally it defines the struct Tracker_data.
The inline static Tracker_data current_data provides the exact same member variable to all child classes.

This implementation is tested under Linux with the latest Razor tracker, and under Windows with the same Razor and VRPN Optitrack.

mgeier · 2019-07-15T18:44:18Z

Thanks a lot for this! This is great, but I have a few suggestions to make this even better and probably avoid some additional work in the future.

In the last few commits to master I've declared the two-dimensional Position and Orientation types as "legacy" code.
Wouldn't it be great to get rid of them in all the tracker code?

Somebody will have to do that anyway at some point, so while you are working on the tracker API, why not change it to 3D and be done with it?

We can of course keep some (or all?) trackers operating in 2D for now, but the API could (and IMHO should!) already be in 3D.

To handle calculations with rotations, there is the ssr::quat type, see https://github.com/SoundScapeRenderer/ssr/blob/master/src/geometry.h.
This is derived from gml::quat (see https://github.com/ilmola/gml), which provides some useful conversion functions.

There are automatic conversions in place to implicitly convert to and from ssr::Rot, see also https://github.com/SoundScapeRenderer/ssr/blob/master/src/api.h.

I'm not quite sure about this, but it might make sense to use ssr::Rot in the Tracker API. I guess it would also be possible to use ssr::quat, but at some point we'll have to do the conversion anyway.

Some further questions and thoughts about the suggested Tracker API:

What is the advantage of storing the current values in Tracker::current_data? Shouldn't this be an implementation detail?
I kinda like the idea of storing the "correction" value in the base class, but this doesn't make sense for all trackers. For example, the Intersense tracker doesn't need this. For code re-use between some trackers, you could probably introduce an intermediate class that not all trackers inherit from?
I like the idea of the Tracker::update() function, but I would kinda invert how it works.
What about changing Tracker::update() to a non-virtual function (that's only implemented in the base class) that takes care of communicating with the "controller" (this would also mean that the Tracker base class would need a constructor where we are passing a reference to the "controller"). The rest of the tracker implementations would never have to mention the "controller".
Why are you exposing the _start(), _stop() and _thread() functions in the base class? I think those are implementation details. Or am I missing something?
I think the constructor should take care of starting and the destructor of stopping the whole thing. But how exactly they are doing it should be left to the implementation. Probably there is some tracker library that takes a callback function and provides its own thread? In that case the API wouldn't work.
But again, for code re-use you might consider adding an intermediate class (but I don't know if that really makes sense here).

src/tracker.h

mgeier · 2019-07-15T18:06:28Z

src/tracker.h

+    // Azimuth value at calibration in degree
+    std::atomic<double> azi_correction{0.0};
+
+    struct Tracker_data


This data type seems overkill to me. Why not just use ssr::quat?

Fix 90 deg offset Fix rebase onto master and revert additional rotations Fix mouse handling listener Fix Head tracking rotation

chris-hld · 2019-07-17T15:43:55Z

Thanks for the feedback!!
I tried to incorporate as much as I could in this new commit (only Razor and Polhemus for now).
I tried it with my Razor and it seems to work already!

In the last few commits to master I've declared the two-dimensional Position and Orientation types as "legacy" code.
Wouldn't it be great to get rid of them in all the tracker code?
Yes, let's try to get as much 3D as possible.

To handle calculations with rotations, there is the ssr::quat type, see https://github.com/SoundScapeRenderer/ssr/blob/master/src/geometry.h.
This is derived from gml::quat (see https://github.com/ilmola/gml), which provides some useful conversion functions.

I'm not sure if I used this correctly. It seems the gml functions can not be applied directly yet, because they return a gml type which can't be used within the SSR it seems.
e.q. reference_rotation_offset(inverse(_correction_rot) * inverse(_current_rot))
does not compile. Not sure how bad it is leave the type conversion (to ssr:.quat r)in the update loop.

There are automatic conversions in place to implicitly convert to and from ssr::Rot, see also https://github.com/SoundScapeRenderer/ssr/blob/master/src/api.h.

I'm not quite sure about this, but it might make sense to use ssr::Rot in the Tracker API. I guess it would also be possible to use ssr::quat, but at some point we'll have to do the conversion anyway.

I was not able to use this correctly.
Furthermore, I think we should consider atomic types. I'm not sure yet, but it seems like the structure in VRPN is not thread safe. There we have to pass a raw pointer to _current_rot which gets de-referenced, while we are updating/resetting in the main loop.
At least I could imagine cases where this goes wrong.

Some further questions and thoughts about the suggested Tracker API:
* What is the advantage of storing the current values in `Tracker::current_data`? Shouldn't this be an implementation detail?

I'll look into this again, but I think this was one way I could come up to use the VRPN callback.

* I kinda like the idea of storing the "correction" value in the base class, but this doesn't make sense for all trackers. For example, the Intersense tracker doesn't need this. For code re-use between some trackers, you could probably introduce an intermediate `class` that not all trackers inherit from?

I see what you mean. But is this really necessary? If we say that our reset function is just resetting whatever comes out of any tracker, then, this should be independent of the child classes.
Intersense could just return quaternions, exactly how every other tracker, right?

* I like the idea of the `Tracker::update()` function, but I would kinda invert how it works.
  What about changing `Tracker::update()` to a non-`virtual` function (that's only implemented in the base class) that takes care of communicating with the "controller" (this would also mean that the `Tracker` base class would need a constructor where we are passing a reference to the "controller"). The rest of the tracker implementations would never have to mention the "controller".

Yes, that is great!
I don't know how to hide controller from child tracker with the current layout and the create() constructor.

* Why are you exposing the `_start()`, `_stop()` and `_thread()` functions in the base class? I think those are implementation details. Or am I missing something?
  I think the constructor should take care of starting and the destructor of stopping the whole thing. But how exactly they are doing it should be left to the implementation. Probably there is some tracker library that takes a callback function and provides its own thread? In that case the API wouldn't work.
  But again, for code re-use you might consider adding an intermediate `class` (but I don't know if that really makes sense here).

Yes you are absolutely right.
Actually, the way the Razor provides an independent running program did not allow to overwrite it, since the actual razor class didn't even inherit from the base Tracker. I just had to override it empty, which is really bad I assume.

mgeier · 2019-07-18T14:54:04Z

I'm not sure if I used this correctly. It seems the gml functions can not be applied directly yet, because they return a gml type which can't be used within the SSR it seems.

Yes, you are using it as intended (but I'm not sure about the "inverse" stuff):

ssr/src/tracker.h

Lines 60 to 61 in 5aca323

    
           ssr::quat r = inverse(_correction_rot) * inverse(_current_rot); 
        
           _controller.take_control()->reference_rotation_offset(r);

C++ doesn't auto-convert gml::quat -> ssr::quat -> ssr::Rot.

We could theoretically add conversion operators to the ssr::Rot type, but I deliberately didn't do this, because I didn't want to have any external dependencies in the API.

Not sure how bad it is leave the type conversion (to ssr:.quat r) in the update loop.

I would hope that this gets optimized away, but it would nice to know for sure ...

I think we should consider atomic types. I'm not sure yet, but it seems like the structure in VRPN is not thread safe.

Yeah, we should think about thread-safety.
Up to now most trackers were thread-unsafe, I guess.

It's probably not a big deal with a single float, but when we use four floats, it's probably more critical.

I think we could simply use the "global controller lock" for this.
_controller.take_control() acquires this lock and anyone who calls ControlInterface::reset_tracker() also automatically acquires the same lock.

{
  auto control = _controller.take_control();
  // access "correction" value
  control->reference_rotation_offset(corrected_value)
}

This way, the "correction" value can never be accessed by two threads at the same time.
No additional atomic variable needed.

What is the advantage of storing the current values in Tracker::current_data? Shouldn't this be an implementation detail?

I'll look into this again, but I think this was one way I could come up to use the VRPN callback.

I'm not saying you shouldn't use instance variables.
I'm just saying that I don't see a reason to have an instance variable for this in the abstract API type ssr::Tracker.

I kinda like the idea of storing the "correction" value in the base class, but this doesn't make sense for all trackers. For example, the Intersense tracker doesn't need this. For code re-use between some trackers, you could probably introduce an intermediate class that not all trackers inherit from?

I see what you mean. But is this really necessary?

Well nothing is really necessary. There are always multiple ways to do things.

I think you should look at ssr::Tracker from the POV of the "controller".

It really just needs to be able to do one thing, namely reset().

It needs no other member functions nor any member variables.

Anything else you are adding is just for code re-use. It's not because it is needed in the interface.

Therefore, I think it would make more sense to create an intermediate class for code re-use.

If we say that our reset function is just resetting whatever comes out of any tracker, then, this should be independent of the child classes.

But some trackers (like Intersense) have a built-in "reset" functionality. Therefore we would add an additional, unneeded "correction" method.

Intersense could just return quaternions, exactly how every other tracker, right?

Well it just has to call reference_rotation_offset(some_rotation) at some point. How this is done can be left to the implementation, there is no reason to include any of this in the API.

I don't know how to hide controller from child tracker with the current layout and the create() constructor.

You don't really have to hide it from the child tracker. It can have an arbitrary signature in its "constructor" (whether this is the real constructor or some static function).
It just has to get an ssr::api::Publisher& in order to be able to call take_control().

But none of this has to be part of the ssr::Tracker interface.
The abstract ssr::Tracker class doesn't need to know anything about ssr::api::Publisher.

the actual razor class didn't even inherit from the base Tracker

I don't understand, it looks like it inherits from Tracker:

ssr/src/trackerrazor.h

Line 46 in 1068831

class TrackerRazor : public Tracker

mgeier · 2019-07-18T13:18:45Z

src/geometry.h

  }
 };

+// Converting from yaw (around Z), pitch (around Y), roll (around X) to quaternions.


This is ambiguous and also not true.

ambiguous: the rotations are around the local coordinate axes (after the axes themselves have been rotated). I'm not sure what's the best way to say this un-ambiguously.
I personally find it better understandable to describe it as three global rotations in a given order (first "roll" then "pitch" then "yaw").

not true: "pitch" is a rotation around a local X axis, because the default orientation is "north".
At least that's how I think it should be in the SSR, other systems will differ.

This stuff is complicated, so I might well be totally wrong about it.

I think it's best just not to create an "official" function for it, since it's a one-liner anyway.

Oh, I see... I just ran into the same problem as in the GUI head-tracker PR earlier.
We have to document this somewhere. Really.

I refer to yaw pitch roll as Tait-Bryan angles (I thought this is the most "common" definition).
More specifically, the aerospace norm/ nautical angles.
At least for me, an airplane first yaws on ground, then pitches during take-off and then rolls around its forward (nose) axis.

I'm not sure if I would understand "north" orientation just from reading it. I think less ambiguous is Y-forward, Z-up. That's at least how coordinate systems got communicated to me, when in doubt (always assuming right handed coordinate systems). Did I understand correctly and do you agree?

We should also make sure to use this convention of YPR consistently across all trackers and maybe even the SSR. I'm not sure if this is the case atm. I thought that at least the Razor uses X-forward, Z-up and the YPR definition above.

We have to document this somewhere. Really.

I agree.
My first attempt is there: https://audioscenedescriptionformat.readthedocs.io/en/latest/position-orientation.html

But this can be for sure improved, and we should mention it in the SSR docs somewhere.

I refer to yaw pitch roll as Tait-Bryan angles (I thought this is the most "common" definition).

Yeah, I prefer those, too. But there are still 6 possible conventions (not considering left-handed systems and not even considering where x, y and z point to in the real world).

I don't really like the aerospace convention because the z axis points down. I don't think that's intuitive in our situation and for our community.

I'm not sure if I would understand "north" orientation just from reading it. I think less ambiguous is Y-forward, Z-up.

I would hope that neither is ambiguous. Do you think it is?
I agree that north/east/south/west is a secondary way of looking at it, but it might still be helpful for some people.
And "north" is definitely more concise than "positive y axis".
And there is definitely a tradition of naming those things after compass directions, e.g. "ENU" and "NED" (see https://en.wikipedia.org/wiki/Axes_conventions).

I think that both are equally valid ways of describing the same thing.

We should also make sure to use this convention of YPR consistently across all trackers

For those tracker APIs that provide quaternions, this whole problem doesn't matter (except for left/right-handedness I guess).

For those that provide angles, we just have to figure out the convention used in their API and correctly convert it to ssr::quat or ssr::Rot.

But each tracker might use their own convention, therefore I don't think it makes sense to provide a general "ypr2quat()" function.

I thought that at least the Razor uses X-forward, Z-up and the YPR definition above.

Maybe. But that doesn't mean we have to use that convention for anything else.

It should really be an implementation detail of each tracker, the important thing is that we get a quaternion out.

In the meantime I've created a new page in the ASDF docs: https://audioscenedescriptionformat.readthedocs.io/en/latest/rotation-matrices.html

Please tell me your suggestions for improvements. I hope there are no ambiguities left.

At some point I'll probably make a similar page using quaternions, but I think the rotation matrix stuff might be easier to understand anyway.

I've added a page about quaternions: https://audioscenedescriptionformat.readthedocs.io/en/latest/quaternions.html

mgeier · 2019-07-18T13:23:29Z

src/geometry.h

 };

+// Converting from yaw (around Z), pitch (around Y), roll (around X) to quaternions.
+// This a lot worse than the other way around and should be avoided.


Depending on your definition of "worse", this might be exactly wrong.

This way is a unique transformation (except for the fact that each rotation can be represented by exactly two quaternions), the other way around is not unique, since there are rotations that can have infinitely many yaw/pitch/roll representations.

Really, the other way around should be avoided!

Ah, of course. This is just plain wrong.
I was trying to say stay in quaternions and don't convert back to YPR.

I was trying to say stay in quaternions and don't convert back to YPR.

Yes, that should be the advice!

mgeier · 2019-07-18T13:31:33Z

src/geometry.h

+template <typename T>
+ssr::quat ypr2quaternion(T yaw, T pitch, T roll)
+{
+  return gml::qrotate(gml::vec3{roll, pitch, yaw});


Another way to write this is:

return gml::qrotate(gml::radians(azimuth), {0.0f, 0.0f, 1.0f}) * gml::qrotate(gml::radians(elevation), {1.0f, 0.0f, 0.0f}) * gml::qrotate(gml::radians(roll), {0.0f, 1.0f, 0.0f});

See e.g. https://github.com/AudioSceneDescriptionFormat/asdfpp-jack-ecasound/blob/f6d5c019fec5520e85256fa0f5379632a0f80aff/src/asdfpp.cpp#L183-L185

I have no clue whether those two are equivalent, because the order of rotations and the order of axes is not visible in the single call to gml::qrotate().

I like having both orders visible.

I don't know which one of the alternatives is faster, would be interesting to benchmark this ...

mgeier · 2019-07-18T13:36:10Z

src/gui/quserinterface.cpp

-                                          (mouse_pos.orientation() - prev_mouse_pos.orientation()));
+    _controller.take_control()->reference_rotation_offset(_scene.get_reference().orientation +
+                                                          _scene.get_reference_offset().orientation +
+                                                          (mouse_pos.orientation() - prev_mouse_pos.orientation()));


I don't think the mouse should affect the "rotation offset".

Using the right mouse button doesn't affect the "position offset" either.

When I try rotating the head (using the binaural renderer), the orientation is jumping around eratically. Something is wrong there ...

mgeier · 2019-07-18T13:42:22Z

src/gui/qopenglplotter.cpp

+    // rotate according to reference offset position
+    glRotatef(_scene.get_reference().orientation.azimuth +
+              _scene.get_reference_offset().orientation.azimuth,
+              0.0f, 0.0f, 1.0f);


As long as the "rotation" and the "rotation offset" are indistinguishable in the interface for the binaural renderer, I agree that adding both makes sense.

But then you should also add "position" and "position offset".

mgeier · 2019-07-18T14:46:51Z

src/tracker.h

+    // Update SSR
+    virtual void update()
+    {
+      ssr::quat r = inverse(_correction_rot) * inverse(_current_rot);


Why do you call inverse() twice?

Shouldn't it be enough to call inverse() once in reset() and here only left-multiply with the already inversed value?

mgeier · 2023-07-09T10:58:36Z

In the meantime, the Qt GUI has been changed to show "reference position/rotation" and "reference offset position/rotation" (hopefully) correctly (in the horizontal plane).
The browser GUI can be used to check 3D rotations and translations.

The Polhemus tracker has been extended to 3D.

This means that many of the changes in this PR are obsolete, but the parts about the VRPN and Razor trackers are still relevant.

In the meantime, there is also a function angles2quat():

ssr/src/geometry.h

Lines 78 to 85 in aa757c9

    
           /// See https://AudioSceneDescriptionFormat.readthedocs.io/quaternions.html 
        
           inline quat angles2quat(float azimuth, float elevation, float roll) 
        
           { 
        
             return normalize( 
        
               gml::qrotate(gml::radians(azimuth),   {0.0f, 0.0f, 1.0f}) * 
        
               gml::qrotate(gml::radians(elevation), {1.0f, 0.0f, 0.0f}) * 
        
               gml::qrotate(gml::radians(roll),      {0.0f, 1.0f, 0.0f})); 
        
           }

chris-hld changed the title ~~Improve tracker layout~~ Fix VRPN, improve tracker layout Jul 15, 2019

mgeier reviewed Jul 15, 2019

View reviewed changes

Chris Hold added 8 commits July 17, 2019 13:31

Fix head rotation

7d6055a

Fix 90 deg offset Fix rebase onto master and revert additional rotations Fix mouse handling listener Fix Head tracking rotation

Merge remote-tracking branch 'upstream/master' into HEAD

a113e88

Fix VRPN Tracker. Start to introduce augmented Tracker struct.

f359fe6

Use new tracker class

edff317

Fix update SSR with new values

f8041bb

Further improvements on tracker handling, introduce data getter

3d2fd03

Fix Warning

2d78f52

Fix rebase onto upstream master

fde501c

chris-hld force-pushed the improve_tracker_layout branch from 0e59e5f to fde501c Compare July 17, 2019 10:37

Start to use quaternions, restructure

5aca323

mgeier reviewed Jul 18, 2019

View reviewed changes

mgeier mentioned this pull request May 13, 2021

3d spatialization #201

Open

mgeier mentioned this pull request Jul 9, 2023

Fix head rotation #140

Open

Fix VRPN, improve tracker layout #146

Are you sure you want to change the base?

Fix VRPN, improve tracker layout #146

Uh oh!

Conversation

chris-hld commented Jul 15, 2019

Uh oh!

mgeier commented Jul 15, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chris-hld commented Jul 17, 2019

Uh oh!

mgeier commented Jul 18, 2019

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!

mgeier commented Jul 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants