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

OVR SDK integration cleanup #13

Closed
mosra opened this Issue May 12, 2016 · 3 comments

Comments

Projects
2 participants
@mosra
Owner

mosra commented May 12, 2016

Hi @Squareys,

While merging #12 I went over the code and did a rough formatting and documentation cleanup. Few things were not clear to me as I'm not very familiar with the API, so once you get some spare time, can you go through the following issues?

  • The HMD -> Session rename is not fully done, some function parameters are still named hmd though the type is now Session
  • Similarly with texture set -> texture swap chain, types don't match function parameter names and docs
  • There are some types still named Hmd but used in the Session class, is that okay?
  • Is OvrIntegration/HmdEnum.h still named correctly? What about just OvrIntegration/Enums.h?
  • The documentation of Session class functions mentions things like
         * If you are using `ovrTrackerPoses` then you will need to call
         * `ovr_GetTrackerPose` after this, because the sensor position(s) will
         * change as a result of this.

More occurences elsewhere. I think the docs should mention the wrapper APIs and not the original OVR SDK APIs.

  • The HmdTrackingCapability and HmdTrackingCapabilities don't seem to be used anywhere and reference removed Hmd::configureTracking() function. Similarly with StatusFlag and StatusFlags. Should they be removed also?
  • The TimewarpProjectionDescription class doesn't seem to be used at all.
  • What does this mean (documentation of StatusFlag)?

The values must be the same as in enum StatusBits.

  • Shouldn't Session::pollController() return the InputState instead of passing it by reference? Isn't the method chaining useless in this case?
  • The HmdStatusFlag enum is used only internally, shouldn't it be hidden from public API (putting it in Implementation namespace or in privates of Session class, e.g.)
  • Avoid repetition by renaming Session::sessionStatus() to just Session::status()
  • Session::ovrEyePoses() returns ovrPosef* but the docs say that this is actually a two-element array. I suggest returning Containers::StaticArrayView<2, ovrPosef> instead.
  • Session::headPoseState(), Session::calibratedOrigin(), Session::eyePoses() and Session::handPoseStates() documentation is copy-pasted four times and doesn't match the code
  • Can we avoid std::string in the Error struct? What about just mirroring the original char[512]?

@mosra mosra referenced this issue May 12, 2016

Closed

Update to Oculus SDK 1.3.0 #12

12 of 12 tasks complete
@Squareys

This comment has been minimized.

Contributor

Squareys commented Jun 13, 2016

There are some types still named Hmd but used in the Session class, is that okay?

Yes, that should be okay :)

Is OvrIntegration/HmdEnum.h still named correctly? What about just OvrIntegration/Enums.h?

Jup, long overdue. Been a while since there were Hmd* enums in there only... if ever.

What does this mean (documentation of StatusFlag)?

This might be a remnant of previous SDK versions.

Shouldn't Session::pollController() return the InputState instead of passing it by reference? Isn't the method chaining useless in this case?

  • The typical use case is this, but I can imagine use cases where you would not want to get the eyePoses right after but just the tracking state. I will think about this.
@Squareys

This comment has been minimized.

Contributor

Squareys commented Aug 11, 2016

While the PR is open anyway, I could start adapting to the API changes in 1.6.0 😛

It looks like there were a bazillion error enum values removed.

@mosra

This comment has been minimized.

Owner

mosra commented Aug 15, 2016

I think we can close this now. Thanks!

@mosra mosra closed this Aug 15, 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