Skip to content
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

Added first basic version. #1

Merged
merged 3 commits into from
May 17, 2021
Merged

Added first basic version. #1

merged 3 commits into from
May 17, 2021

Conversation

S-Dafarra
Copy link
Member

It implements a first basic skeleton to use OpenXr. I was not able to shrink it much. It turns out that setting up OpenXr requires quite a bit of code. In order to implement the first version I strongly used the following examples:

For the moment, it opens a window and draws the left part to green and the right to blue
image

I realized that the window is indeed useless. Differently from the Oculus SDK there is no way to mirror what the operator sees. It would be possible to see the layers, but not the composite frame. Nonetheless, this is not much of a deal as the runtimes already provide similar functionality:

Hence, I will probably remove the window in future PRs.

@S-Dafarra S-Dafarra requested a review from traversaro May 7, 2021 14:51
@S-Dafarra S-Dafarra self-assigned this May 7, 2021
It simply prints the screen half green and half blue.

Added configuration phase and first set of methods of the OpenXr interface.

Finished preparation phase.

Finished the rendering and closing part of OpenXr.

Initial debug.

Finished first skeleton.

Cleanup before PR.
@S-Dafarra
Copy link
Member Author

I just force pushed to remove some commented code.

CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@traversaro traversaro left a comment

Choose a reason for hiding this comment

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

Minor comment.

@traversaro
Copy link
Contributor

In general, the idea is to have a separate repo from https://github.com/robotology/yarp-device-ovrheadset and eventually copy parts that may be useful?

@S-Dafarra
Copy link
Member Author

S-Dafarra commented May 7, 2021

In general, the idea is to have a separate repo from https://github.com/robotology/yarp-device-ovrheadset and eventually copy parts that may be useful?

Yes. I will try to port most of the structure. On the other hand, some options will be probably different. For example, as mentioned above, I am thinking to not open any window. Hence, I need to figure out another way to adjust the views.

Unfortunately, almost all the code in https://github.com/robotology/yarp-device-ovrheadset is entangled with Oculus-specific calls, so I did not have much of a choice.

Also, considering that we are not the sole users of that device I was a bit afraid of introducing too many modifications.

@S-Dafarra
Copy link
Member Author

Let me know if you are ok with me (squash) merging!

@traversaro
Copy link
Contributor

Let me know if you are ok with me (squash) merging!

Ok for me.

@kouroshD
Copy link

kouroshD commented May 7, 2021

@S-Dafarra If you do not have any rush for merging, I would like to review the code next week. Let me know if it is OK to you.

@S-Dafarra
Copy link
Member Author

@S-Dafarra If you do not have any rush for merging, I would like to review the code next week. Let me know if it is OK to you.

Ok, I guess can wait. In any case, this is just the first PR with the CMake structure and a first use of OpenXr. It is pretty useless per-se. I will probably open other PRs in a couple of days or so 😉

@kouroshD
Copy link

kouroshD commented May 7, 2021

Ok, I guess can wait. In any case, this is just the first PR with the CMake structure and a first use of OpenXr. It is pretty useless per-se. I will probably open other PRs in a couple of days or so wink

That's exactly why I want to review it also :') otherwise, for the future PRs, I am lost with the codes! So, I can incrementally follow this.

CMakeLists.txt Outdated
find_package(YCM REQUIRED)
find_package(YARP 3.4 COMPONENTS os sig dev math REQUIRED)
find_package(Threads REQUIRED)
find_package(OpenXR REQUIRED)
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Tracking the exact driver version used, along with some reference to an issue that points to the driver installation will be helpful

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 34fd50d. The installation instructions will be added in a README when most of the functionalities are there. In the meanwhile, most of the installation instructions are the following

# Install monado packages with monado ppa ( https://monado.freedesktop.org/packages-ubuntu.html )
sudo add-apt-repository ppa:monado-xr/monado
sudo apt-get update
sudo apt install libopenxr1-monado build-essential meson libvulkan-dev libopenxr-dev libvulkan1 libopenxr-loader1 xxd libglm-dev glslang-tools libglfw3-dev

Then, in separate terminals run the yarpserver and monado-service. Clone, compile and install this repo. Then, launch yarpdev --device openxrheadset from the install/share/yarp folder.

Let me know if I missed something.

}

// the instance handle can be thought of as the basic connection to the OpenXR runtime
XrInstance instance = XR_NULL_HANDLE;
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Adding a space between lines here will be easy for readability

Copy link
Member Author

Choose a reason for hiding this comment

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

char resultString[XR_MAX_RESULT_STRING_SIZE];
xrResultToString(instance, result, resultString);

char formatRes[XR_MAX_RESULT_STRING_SIZE + 1024];
Copy link
Member

Choose a reason for hiding this comment

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

Is the value 1024 just a place holder for testing or something that can be moved to a config parameter ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It cannot be retrieved from conf file because it has to be static. It is just a buffer for printing errors.



uint32_t output_viewCount = m_pimpl->views.size();
XrResult result = xrLocateViews(m_pimpl->session, &view_locate_info, &(m_pimpl->view_state), m_pimpl->views.size(), &output_viewCount, m_pimpl->views.data());
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Line length to be reduced

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 34fd50d.

Copy link
Member

@yeshasvitirupachuri yeshasvitirupachuri left a comment

Choose a reason for hiding this comment

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

@S-Dafarra I added some minor comment, feel free to ignore them.
LGTM, just that adding OpenXR driver related reference such a version used can be useful going further. Thanks!

@kouroshD
Copy link

Thank you @S-Dafarra and sorry if it took some time to review it.

@S-Dafarra
Copy link
Member Author

Thank you @S-Dafarra and sorry if it took some time to review it.

No problem, thank you for rewieving. I am squashing and merging.

@S-Dafarra S-Dafarra merged commit 3fe9fee into main May 17, 2021
@S-Dafarra S-Dafarra deleted the firstVersion branch May 19, 2021 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants