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

vrclient: Convert action manifests to unix paths #6109

Open
wants to merge 1 commit into
base: proton_7.0
Choose a base branch
from

Conversation

farmboy0
Copy link

Action manifest files contain refernces to default bindings files for different controllers. These binding urls can contain absolute windows path for proton games which need to be converted for SteamVR.

This change has been tested with
[559330] A Fisherman's Tale
]1087500] Groundhog Day: Like Father Like Son

which both become playable out of the box.

@gofman
Copy link

gofman commented Aug 22, 2022

Thanks for the patch.

I don't think we should modify game provided manifest file. I think that we better create another (temporary) file in Win temp directory (with the name derived from the original manifest file so we don't create a new distinct file each run) and pass the path to that one to Linux VR client. Only in case if conversion is really needed, otherwise just pass the original untouched file path (like it is done now).

@farmboy0
Copy link
Author

The problem with that solution is that vrmanifest files contain references to the action manifest.

@gofman
Copy link

gofman commented Aug 23, 2022

Which exactly vrmanifest files do you mean?

@farmboy0
Copy link
Author

I am talking about the xxx.vrmanifest files that games can employ to have SteamVr recognize the game uses the newer input handling and not the legacy method without the game started.
Its briefly talked about here: https://github.com/ValveSoftware/openvr/wiki/Action-manifest

@gofman
Copy link

gofman commented Aug 23, 2022

Sorry, I might be missing something but I don't quite understand what do you mean yet.

Yes, there are more ways to tell SteamVR about action manifest file path (set it up in SteamWorks so SteamVR will get it directly without interacting with the app itself, providing .vrmanifest file as the link suggests; also there is undocumented way to supply the path to manifest in vrclient starupinfo which we happen to support by now). In those cases the game won't call SetActionManifestPath() (or it least it is not required). Not immediately sure how that can be solved at all, but IMO it is not directly related to this patch and this issue. Most of the games do provide relative paths and that works (not even sure how those one you linked end up with absolute paths? Do they generate the manifest dynamically? Or are they broken on Windows if installation goes not to default folders?). If SetActionManifestPath() is called we can fix things up with a copy of the file. The other cases when SetActionManifestPath() is not even called but the path requires conversion won't be solved regardless of how our SetActionManifestPath() handles the fixup. Am I missing something?

@farmboy0
Copy link
Author

The reason I implemented my solution by overwriting the action manifest file was because there are ways to tell SteamVR where the action manifest file resides. If I didnt overwrite it but used a temp file like you suggested then those ways would refer to the unpatched action manifest which SteamVR cant use.

The games I tested create the action manifest dynamically on game start so they also overwrite the patched file on each start.

Some games are of course cleverer than these and use relative paths but even some of those could use backslashes for the paths which I dont think SteamVR can deal with either.

@gofman
Copy link

gofman commented Aug 23, 2022

Yes, there are different ways to tall SteamVR where action manifest is, but if those other ways are used SetActionManifestPath() is normally not called and your patch has not effect. On the contrary, if SetActionManifestPath() is called it takes precedence over other ways to specify manifest location, so an alternate file should be used even if the other ways are also present.

Modifying game file is bad for a number of reasons. Could work as a game specific hack maybe (while I don't yet see the reason for that here).

@farmboy0
Copy link
Author

Alright i change it then if you prefer that solution just wanted to make it understood first why i did it this way.

@farmboy0
Copy link
Author

Would you prefer putting the files into /tmp, /tmp/steamvr or ~/steamvr or something else?

@gofman
Copy link

gofman commented Aug 23, 2022

I'd put it into Windows temp dir in game prefix (what GetTempPath returns). That path requires conversion to Unix path too of course before passing to native SteamVR. I would not use a random name, would use original file name with an added extension. Please make sure that you don't create a file and alter the path if nothing in input file requires conversion, we don't want to interfere into this for the majority of games which don't use absolute paths.

Also, could you please avoid using std:: streams or any additional c++ features beyond std::string (like it is already done in json_convert_paths()), using winapi file IO (CreateFile, ReadFile, WriteFile, CloseHandle) with Windows paths? Json parser can take 'char *' string (see json_convert_paths). Yes, that is .cpp file which uses c++ json parser, but there is a very limited use of cpp in non-generated code and going further it may create issues.

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.

2 participants