-
Notifications
You must be signed in to change notification settings - Fork 111
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
WIP: Linux support #90
base: master
Are you sure you want to change the base?
Conversation
…ities linux builder Now runs on linux warning crushing warning crushing Add subhook submodule Adding some override markers Misc small changes Improved comms driver now starts under linux still not hooking functions, but can call virutal functions with direct function pointers Kind of works now with hooking Linux port now functional Reducing delta to windows version adding linux to version string
If someone volunteers to maintain Linux support, this seems like a valuable addition. |
BTW I got my Linux port working a week ago https://github.com/gyroninja/OpenVR-SpaceCalibrator to work. It does require you to manually create the addon folder and use vrpathreg to add it.I do assume that ~/.config/OpenVR-SpaceCalibrator and "/.local/share/OpenVR-SpaceCalibrator exist. I did not bother with keeping Windows support in my fork so props to you for doing this. It's funny how we both did this at almost the name time despite this not having been ported for years. |
@gyroninja Going with those are you using ALVR? |
Removing unused module Have CMake pull the submodules Added Linux CMake installer Removing unused code Partially working auto installer.
fixes issue locking up on start up in some cases
At this point there are no known deficiencies in either the Windows or Linux build caused by this merge. Let me know if there's anything else I can do to push this forward. |
@@ -142,17 +152,77 @@ static void WriteProfile(CalibrationContext &ctx, std::ostream &out) | |||
out << profilesV.serialize(true); | |||
} | |||
|
|||
#ifdef __linux__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be a bit worried that the use of #ifdef
scattered about the codebase will create maintenance problems in the future. Usually the way this is approached in larger codebases would be to refactor out the code that needs to differ into its own function, then have a set of windows-specific or linux-specific .cpp files. Having it scattered everywhere makes it hard to see what differs between the platforms, and because the surface area that differs is so ad-hoc, it makes it too easy to forget to carry across a change between the two variants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can swap things over to that. I can see how it would be a better solution.
stop = true; | ||
|
||
#ifdef __linux__ | ||
//NOP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we don't support unloading on linux? If the driver .so is unloaded we'll end up crashing vrserver afterward...
OpenVR-SpaceCalibratorDriver/Comms.h
Outdated
|
||
serverAddr.sin_family = AF_INET; | ||
serverAddr.sin_port = htons(COMM_PORT_SERVER); | ||
serverAddr.sin_addr.s_addr = INADDR_ANY; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably better to use a unix domain socket, as there's no reason for this to be accessible remotely.
#ifdef __linux__ | ||
#include "StaticConfig.h" | ||
#else | ||
#define DRIVER_LOG_FILE "space_calibrator_driver.log" | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this different on different platforms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StaticConfig.h.in contains paths set up in CMake to contain the directories to log to and the ones where the config will be stored. StaticConfig.h is a generated file so I'm not sure if or how it would be used in a windows build. Without swapping the Windows build to CMake I'm not sure what another good solution would be.
void * nothing; | ||
} OVERLAPPED; | ||
|
||
typedef void* HANDLE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of thing makes me very worried that future changes will break linux support in subtle ways, simply because they might introduce Windowisms without having random #ifdef
s scattered about. A proper port needs to refactor out the OS-specific portions of the code into a well-contained area, and needs to ensure the build will fail, hard, if OSisms like HANDLE
leak out into shared code.
#define COMM_PORT_SERVER 5473 | ||
#define COMM_PORT_CLIENT 5474 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a unix domain socket will avoid the risk of conflicting with other users of these ports.
SetDeviceTransform(uint32_t id, bool _enabled, vr::HmdVector3d_t _translation, vr::HmdQuaternion_t _rotation, double _scale) : | ||
openVRID(id), enabled(_enabled), updateTranslation(true), updateRotation(true), updateScale(true), translation(_translation), rotation(_rotation), scale(_scale) { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why these refactors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was getting warnings due to shadowing. My IDE settings might be too aggressive. Let me know if you'd want them removed.
Build is failing as of |
Those looking for working Linux support, you can try this fork here: https://github.com/galister/OpenVR-SpaceCalibrator It's pieced together from the contributors of this thread, and I plan on maintaining this, but not going to try and merge it upstream. |
Working Linux support
It's a little rough, but now feels like time to start a conversation about if this kind of thing is valuable and how turn key it should be for Linux users.
The state as of generating the PR is that there are undocumented dependencies, automatic calibration does not work quite right, and the VR UI works incorrectly. The base functionality does work if you can get it built however.