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

Update to Oculus SDK 1.3.0 #12

Closed
wants to merge 50 commits into
base: master
from

Conversation

Projects
2 participants
@Squareys
Contributor

Squareys commented Mar 30, 2016

Hello everybody!

The first non beta version of the Oculus SDK has finally been released and I'm working on updating OvrIntegration respectively.

TODO:

  • Figure out what's going on with the layers (some removed, LayerEyeMatrix added)
  • Adapt to mirror texture changes
  • Make sure documentation is still correct
  • Other minor changes and renaming
    • Changes in ErrorType enum
    • Add TrackingOrigin enum and Hmd::setTrackingOriginType()
  • Fix FindOVR.cmake (for MSVC)*
  • Wrap input related API
    • Add ControllerType enum
    • Get controller state
    • Get tracker poses
  • Rename Hmd to Session

* Not tested yet. AppVeyor fails at downloading the Oculus SDK.

This should be done this Sunday.

Regards,
Jonathan.

Squareys added some commits Mar 29, 2016

OvrIntegration: Remove `OvrIntegration::wrap(ovrTexture)`
The `ovrTexture` structure was removed in the new SDK version.

Signed-off-by: Squareys <Squareys@googlemail.com>
OvrIntegration: Remove previously deprecated ovrHmd
Was deprecated in the last update in favor of ovrSession.

Signed-off-by: Squareys <Squareys@googlemail.com>
OvrIntegration: Rename recenterPose() to recenterTrackingOrigin()
Analog to change in SDK.

Signed-off-by: Squareys <Squareys@googlemail.com>
OvrIntegration: Remove obsolete OVR_KEY_* getters
Signed-off-by: Squareys <Squareys@googlemail.com>
OvrIntegration: Adapt to removal of ovr_ConfigureTracking
Signed-off-by: Squareys <Squareys@googlemail.com>

@Squareys Squareys force-pushed the Squareys:oculus-sdk-1.3.0 branch from 59acfc2 to 3c4e252 Mar 30, 2016

ci: Have appveyor download oculus-sdk-1.3.0
Signed-off-by: Squareys <Squareys@googlemail.com>

@Squareys Squareys force-pushed the Squareys:oculus-sdk-1.3.0 branch from 3c4e252 to a0f48a9 Mar 30, 2016

@mosra

This comment has been minimized.

Owner

mosra commented Mar 31, 2016

Wow, awesome, you are continuously doing great job! :)

Didn't look into the changes yet, just an idea that came up on my mind -- could you try enabling the library on AppVeyor for MSVC 2015? It's explicitly set to OFF because there were linker problems before. I'm curious to see whether it works already or not.

@Squareys Squareys force-pushed the Squareys:oculus-sdk-1.3.0 branch 2 times, most recently from 3025b3a to 7c5715a Mar 31, 2016

@Squareys Squareys referenced this pull request Apr 1, 2016

Closed

ovr: Adapt to recent OvrIntegration changes. #22

1 of 1 task complete
@Squareys

This comment has been minimized.

Contributor

Squareys commented Apr 2, 2016

Well, AppVeyor really does not seem to want to download the SDK...

@Squareys Squareys force-pushed the Squareys:oculus-sdk-1.3.0 branch 2 times, most recently from 4ff4317 to deb95c7 Apr 2, 2016

@Squareys

This comment has been minimized.

Contributor

Squareys commented Apr 3, 2016

In the last SDK update ovrHmd was renamed to ovrSession, which we could/should follow with Hmd (This is sane, since you require the session to get controller input state, which semantically does not make sense to get from the headset). Three options:

  • Don't rename
  • Rename, but keep an alias to not break outside code (who cares, I guess, others can do that themselves if they must)
  • Complete rename of Hmd to Session

I'd prefer the last. What do you say, @mosra?

@Squareys Squareys force-pushed the Squareys:oculus-sdk-1.3.0 branch from b8d68a5 to 7ebb79b Apr 3, 2016

@mosra

This comment has been minimized.

Owner

mosra commented Apr 4, 2016

Rename Hmd to Session.

We're not doing any more harm than Oculus itself when doing those renames, so that's okay.

@Squareys Squareys force-pushed the Squareys:oculus-sdk-1.3.0 branch from ba03b15 to f76eabe Apr 7, 2016

Squareys added some commits Mar 30, 2016

OvrIntegration: Adapt to changes to SwapTextureSet (now TextureSwapCh…
…ain)

The API now handles current index incrementation via `commit`. Texture
format is limited to a subset of texture formats of which I chose
`OVR_FORMAT_R8G8B8A8_UNORM_SRGB` to be used analog to the SDK samples.

`ovrTexture` has been removed and we now get the opengl texture id directly
with `ovr_GetTextureSwapChainBufferGL(...)`.

Signed-off-by: Squareys <Squareys@googlemail.com>
OvrIntegration: right-handedness is now default for projection matrices
`ovrProjection_RightHanded` has been renamed to `ovrProjection_LeftHanded`
as a consequence and we do not need to specify it anymore.

Signed-off-by: Squareys <Squareys@googlemail.com>
OvrIntegration: Adapt to enum changes
Signed-off-by: Squareys <Squareys@googlemail.com>
OvrIntegration: Use `Context::error()`
Signed-off-by: Squareys <Squareys@googlemail.com>
OvrIntegration: Update copyright in license header
Signed-off-by: Squareys <Squareys@googlemail.com>
modules: Update FindOVR.cmake to find Runtime version 1.0 and higher
Signed-off-by: Squareys <Squareys@googlemail.com>
OvrIntegration: Update EnumTest
Signed-off-by: Squareys <Squareys@googlemail.com>
OvrIntegration: Adapt to changes with compositor layers
LayerDirect and LayerEyeFovDepth have been removed. LayerEyeMatrix is for
compatibility with the Oculus Mobile SDK and will not be supported at this
point.

Signed-off-by: Squareys <Squareys@googlemail.com>
OvrIntegration: Adapt to changes with mirror texture creation
Signed-off-by: Squareys <Squareys@googlemail.com>
OvrIntegration: Adapt to renaming of HmdToEyeViewOffset to HmdToEyeOf…
…fset

Signed-off-by: Squareys <Squareys@googlemail.com>
OvrIntegration: Adapt to ovrSessionStatus changes
Signed-off-by: Squareys <Squareys@googlemail.com>

Squareys added some commits Apr 7, 2016

OvrIntegration: Remove now obsolete TextureSwapChain friend from Hmd
Signed-off-by: Squareys <Squareys@googlemail.com>
OvrIntegration: Rename `Hmd` to `Session`
Analog to recent changes in the Oculus SDK.

Signed-off-by: Squareys <Squareys@googlemail.com>
OvrIntegration: Rename Hmd.h/.cpp to Session.h/.cpp
Signed-off-by: Squareys <Squareys@googlemail.com>

@Squareys Squareys force-pushed the Squareys:oculus-sdk-1.3.0 branch from 109ab19 to da42860 Apr 7, 2016

@Squareys

This comment has been minimized.

Contributor

Squareys commented Apr 7, 2016

@mosra Yes, it was because I passed an aligned structure by value instead of const ref. Should have fixed it :)

message(WARNING "OVR_SDK_ROOT is not set. Will try to find headers and library in ${CMAKE_INSTALL_PREFIX}." )
else()
set(LIBOVR_ROOT ${OVR_SDK_ROOT}/LibOVR)
find_path(OVR_SDK_ROOT OculusSDK)

This comment has been minimized.

@mosra

mosra Apr 17, 2016

Owner

Minor nitpick: accidental indentation with three spaces

if(OVR_SDK_ROOT)
set(OVR_SDK_ROOT "${OVR_SDK_ROOT}/OculusSDK")
mark_as_advanced(OVR_SDK_ROOT)

This comment has been minimized.

@mosra

mosra Apr 17, 2016

Owner

Only variables with explicit CACHE setup or variables coming from find_*() commands need to be marked as advanced. All other variables don't go into the cache at all.

(One more occurrence below.)

This comment has been minimized.

@Squareys

Squareys Apr 18, 2016

Contributor

In this special case, I would do explicit cache setup to enable the user to specify a custom OCULUS_SDK_ROOT. Would that make sense?

This comment has been minimized.

@mosra

mosra Apr 18, 2016

Owner

Then you need to so something like set(OVR_SDK_ROOT "<some default path>" CACHE PATH "OculusVR SDK root") to make it visible in the GUI and avoid overwriting it with a different value later to reduce confusion.

find_library(OVR_LIBRARY NAMES LibOVR HINTS "${LIBOVR_ROOT}/Lib/Windows/${MSVC_ARCH}/Release/${MSVC_NAME}")
unset(MSVC_ARCH)
unset(MSVC_NAME)

This comment has been minimized.

@mosra

mosra Apr 17, 2016

Owner

I would suggest using prefixes to avoid conflicts with user variables -- so e.g. _OVR_MSVC_NAME or something like that, leading underscore suggesting that this is an internal variable.

This comment has been minimized.

@Squareys

Squareys Apr 18, 2016

Contributor

Will do, makes sense 👍

@see @ref TrackerFlags
*/
enum class TrackerFlag: Int {
/**< The sensor is present, else the sensor is absent or offline */

This comment has been minimized.

@mosra

mosra Apr 17, 2016

Owner

Superfluous < in the doc block (would cause the enum value to be undocumented).

constexpr Buttons L_BUTTON_MASK = Button::X | Button::Y | Button::LThumb | Button::LShoulder;
/** @brief Bit mask of buttons used by Oculus Home */
constexpr Buttons PRIVATE_MASK = Button::VolUp | Button::VolDown | Button::Home;

This comment has been minimized.

@mosra

mosra Apr 17, 2016

Owner

I'm not sure about the naming convention and lack of scoping here, similarly below with touch masks.

I was wondering whether it would make sense to use a class instead of a typedef for the Buttons type and put them inside:

struct Buttons: Containers::EnumSet<Button> {
    constexpr Buttons LMask = ...;
    constexpr Buttons RMask = ...;

    using EnumSet::EnumSet; // inherit the constructors
};

I think this should work and cause no weird compilation issues (but I can be always wrong).

This comment has been minimized.

@Squareys

Squareys Apr 18, 2016

Contributor

This is harder than expected, gcc will complain about "use of incomplete type" for Buttons. I'm trying to figure out how to do this, but will fix the other issues first.

This comment has been minimized.

@Squareys

This comment has been minimized.

@mosra

mosra Apr 18, 2016

Owner

Oh crap.

Since the type isn't really needed (but still would be nice to have it there for Doxygen):

struct Buttons: Containers::EnumSet<Button> {
    constexpr 
        #ifdef DOXYGEN_GENERATING_OUTPUT
        Buttons
        #else
        Containers::EnumSet<Button>
        #endif
        LMask = ...;

    using EnumSet::EnumSet; // inherit the constructors
};

This comment has been minimized.

@Squareys

Squareys Apr 24, 2016

Contributor

Done :)

MetricsNoApp = ovrError_MetricsNoApp,
MetricsOafFailure = ovrError_MetricsOafFailure,
MetricsSessionAlreadyActive = ovrError_MetricsSessionAlreadyActive,
MetricsSessionNotActive = ovrError_MetricsSessionNotActive,

This comment has been minimized.

@mosra

mosra Apr 17, 2016

Owner

Doc blocks missing here.

This comment has been minimized.

@Squareys

Squareys Apr 18, 2016

Contributor

Ah, that's because there was no doc in the SDK :D

This comment has been minimized.

@Squareys

Squareys Apr 18, 2016

Contributor

I just found lwjgl also wraps the Oculus SDK and they document these as Error codes (ovrErrorType)...

Some of these I can't even imagine what they are supposed to mean.

This comment has been minimized.

@mosra

mosra Apr 18, 2016

Owner

Huh :D What about just braindead repeat of the enum name until Oculus provides something better? E.g. MetricsNoAppMetaData -> /**< No app metadata metrics */

This comment has been minimized.

@Squareys

Squareys Apr 18, 2016

Contributor

Sure, will do that tomorrow :)

* depends on the usage of the struct.
*/
class MAGNUM_OVRINTEGRATION_EXPORT PoseState {
public:

This comment has been minimized.

@mosra

mosra Apr 17, 2016

Owner

Please indent also the public:/private: labels to be consistent with the rest of the codebase.

}
static const PoseState& wrap(const ::ovrPoseStatef& state) {
return reinterpret_cast<const PoseState&>(state);
}

This comment has been minimized.

@mosra

mosra Apr 17, 2016

Owner

Doc blocks missing, similarly for the single-parameter constructors below.

@mosra

This comment has been minimized.

Owner

mosra commented Apr 17, 2016

👍

The only concern I have are the masks for Buttons and Touches, the rest are just minor issues. First-class work again! :)

Squareys added some commits Apr 18, 2016

modules: Use prefixes for internal variables in FindOVR.cmake
Signed-off-by: Squareys <Squareys@googlemail.com>
modules: Correct indentation in FindOVR.cmake
Signed-off-by: Squareys <Squareys@googlemail.com>
modules: Handle case when user defined OVR_SDK_ROOT
Also remove `mark_as_advanced` where unnecessary.

Signed-off-by: Squareys <Squareys@googlemail.com>
OvrIntegration: Fix indentation of PoseState and InputState
Signed-off-by: Squareys <Squareys@googlemail.com>
OvrIntegration: Add missing documentation
Signed-off-by: Squareys <Squareys@googlemail.com>
@Squareys

This comment has been minimized.

Contributor

Squareys commented Apr 18, 2016

@mosra Alright, I fixed all the minor issues. The solution you proposed for Buttons and Touches does not quite work as hoped. I understand why the initial solution is not nice, so I will try to find another solution for that.

@mosra mosra referenced this pull request Apr 24, 2016

Closed

Next release #108

34 of 34 tasks complete

Squareys added some commits Apr 24, 2016

OvrIntegration: Move masks into EnumSets
Changes in Session.h/.cpp are made to avoid having to include HmdEnum.h.
Declarations in HmdEnum.cpp *are* necessary to avoid undefined references.
Testcases are provided for masks.

Signed-off-by: Squareys <Squareys@googlemail.com>
OvrIntegration: Document undocumented enum values of the Oculus SDK
... by simply repeating their name, as suggested.

Signed-off-by: Squareys <Squareys@googlemail.com>

@Squareys Squareys force-pushed the Squareys:oculus-sdk-1.3.0 branch from 28edd99 to badcd15 Apr 24, 2016

@Squareys Squareys changed the title from [WIP] Update to Oculus SDK 1.3.0 to Update to Oculus SDK 1.3.0 Apr 24, 2016

@mosra mosra referenced this pull request May 12, 2016

Closed

OVR SDK integration cleanup #13

@mosra

This comment has been minimized.

Owner

mosra commented May 12, 2016

Merged in ff7b924, sorry it took so long. I did a rough cleanup and review of the code, but because the PR was already quite big, I put the comments into a separate issue: #13. If you could go over it once you have time, that would be great. It's not extreme high priority, though, you have other things to focus on :)

@mosra mosra closed this May 12, 2016

@mosra mosra added this to the 2018.02 milestone Feb 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment