bjornblissing / osgoculusviewer

An OsgViewer with support for the Oculus Rift
Other
106 stars 67 forks source link

Make OculusDevice position and orientation match the OculusSDK #78

Closed rickyviking closed 8 years ago

rickyviking commented 8 years ago

Hi Bjorn,

this pull request adds a method to get Oculus orientation as yaw, pitch and roll, using internal Oculus math. It can be useful when integrating the Oculus in more complex applications where you need to know the current camera offset given by the tracker, to use it for motion, selection, etc.

I've tested the method in my own project and works as expected. The guideline explaining the yaw, pitch roll reference is here: https://developer.oculus.com/documentation/pcsdk/latest/concepts/dg-sensor/#dg-sensor-head-tracking

bjornblissing commented 8 years ago

Hi @rickyviking

I have a hard time seeing the need for a special yaw, pitch & roll return function. We already return the orientation as a quaternion, from that is the user can easily extract the rotation angle around any axis. This without the need to include the Extras/OVR_Math.h.

Using Euler angles are usually discouraged due to the fact that you may end up with gimbal lock situations.

As I see this commit it adds complexity for a corner use case. So unless you have a really good selling point I do not see the benefit from merging this.

rickyviking commented 8 years ago

Hi @bjornblissing,

the main point I wanted to address is basically that the position and orientation returned by the device, are actually a pre-processed version of what the oculus return (inverted) to be used directly in the osg viewmatrix offset.

This might be confusing if you are trying to understand how the oculus is really positioned. In the attempt of extracting this info, I was actually thinking that yaw/pitch/roll would be easier to understand, but they are of course a duplicate info.

What do you think about returning position and orientation from the device as they are and inverting them in the slave camera callback?

bjornblissing commented 8 years ago

Why can you not use these two functions that are already present: https://github.com/bjornblissing/osgoculusviewer/blob/40977a2da27a5628739a7b17ff2d535247f3dd74/src/oculusdevice.h#L148-L149

rickyviking commented 8 years ago

Ok I've updated the code to better explain myself.

Position and orientation getters now return what the Oculus actually provides, and the values are inverted only in the slave callback where needed.

I believe the API is more consistent this way: as the OculusDevice class wraps the device, I'd expect it to return the device's pos and rotation as they are. What you think about it?

rickyviking commented 8 years ago

Hi @bjornblissing can you please let me know if you're inclined to merge the pull request after the latest changes? thanks, ricky

bjornblissing commented 8 years ago

Hi @rickyviking It looks sensible, but I will have to make sure that it do not break anything existing, for example: Manipulators or IPD settings. I have bad experiences with sign changes. :)

I will try to do some testing during the holidays and if everything checks out I will merge it.

rickyviking commented 8 years ago

Hi @bjornblissing did you get a chance to run some tests? I'll need to close a release shortly, just wanted to know if I can rely on the patch I sent you. Thank you!

bjornblissing commented 8 years ago

Hi @rickyviking

I cherry picked your last commit in this pull request, did some formatting and added you to the contributors list.

rickyviking commented 8 years ago

Hi @bjornblissing, thank you!