fix(steamvr_launcher): find steam folder by using steamlocate-rs library #2842
Conversation
Meister1593
requested review from
The-personified-devil,
shinyquagsire23 and
zmerp
|
This is brilliant, do you think the steamlocate crate could be used to simplify some other code in alvr_server_io? (Especially the one that is common between windows and linux) |
Definitely can try doing so, yeah, it should work on both platforms. |
|
Took way longer than i expected lol... |
|
I have maybe only one concern (which is probably not an issue) - is that, if steamvr is on multiple drives, this would get first available and use it |
|
This seems such a weird edge case. I'm sure a lot of other stuff breaks if this is the case |
|
I would also vote on angrily shaking fists at anyone with SteamVR on two drives |
|
At least on linux I know that steam forces steamvr to be installed in the main steam location, not in a secondary one. Tho I'm not sure if that's guaranteed to be the main drive. |
| Err(e) => { | ||
| error!("Couldn't detect openvr or steamvr files. \ | ||
| Please make sure you have installed and ran SteamVR at least once. \ | ||
| Or if you're using Flatpak Steam, make sure to use ALVR Dashboard from Flatpak ALVR. {e}"); |
There was a problem hiding this comment.
I'm really not happy with losing the flatpak steam information, because I feel that that's quite useful. We should find some way to still print it.
There was a problem hiding this comment.
Or actually, would two competing installations of flatpak steam steamvr and normal steamvr trip up steamlocate?
Since we need to make sure it'll find the directory of the one from flatpak steam, and this is actually realistic enough to occur on linux with users fucking around to see what actually works
There was a problem hiding this comment.
Gonna test specifically flatpak usecase, but steamlocate-rs does support finding even multiple installations, including flatpak (i thought it will probably find it relative to $home first, not from flatpak, but will check anyway)
There was a problem hiding this comment.
i realized that we might have a problem/solution for the flatpak problem
So in a nutshell, steamlocate-rs finds flatpak steam first, instead of native.
And we can launch either native or flatpak alvr (driver) from dashboard (which can be also launched from either flatpak or natively)
i wonder if we should allow user to choose from where to launch it?...
There was a problem hiding this comment.
i will make quick poc in this pr and see you opinion about this
There was a problem hiding this comment.
...not gonna make a quick poc, because RLS is not recognizing types for steamlocate::SteamDir::locate_multiple and even specifying them manually, doesn't make it work still... (compiler does work fine though)
There was a problem hiding this comment.
So in a nutshell, steamlocate-rs finds flatpak steam first, instead of native.
Does this also occur if ran from native and not just in flatpak? If so that's a straight up blocker (at least for using it to locate the steamvr root, failing to do mess around with the unblocking isn't too bad)
| debug!( | ||
| "steamvr_vrsettings_path: {}", | ||
| steamvr_vrsettings_path.display() | ||
| ); |
There was a problem hiding this comment.
Don't know if we should be keeping this debug print and if we do we should add some more context.
There was a problem hiding this comment.
main purpose for debug prints is to debug it with users if they are wrong later on (like, user downloads debug build and tests his issue with it - has this print and developer already knows required details).
This one specifically useful if one of the paths were used, but missing, and leading to an error.
There was a problem hiding this comment.
Well you should make it a bit nicer, this feels very temp debug thrown in, e.g. Found steamvr.vrsettings with path {}
|
Sorry if I missed this in the comments, but why was the test removed? |
it was testing mostly std filesystem and steamlocate-rs crate instead of alvr functionality |
I don't think this is a bad idea, since we rely on this working properly. Maybe the test could be moved to steamlocate-rs itself but we need to make a PR to it. |
Issue IMO is that... test basically created own conditions for own satisfaction to test that filesystem, some files exist Most of this (except for specifically steamvrsettings) is just testing filesystem existence for something |
|
All other tests (related to finding steam, steamvr (specifically - any app really)) are already in steamlocate-rs crate |
|
Got it, it's cool then |
There was a problem hiding this comment.
Good on my side, waiting for @The-personified-devil 's comment threads
|
Needs a bit of rework considering issues with finding incorrect stuff in different places, might scale down pr a bit |
4 participants
Was dependent on WilliamVenner/steamlocate-rs#94
Closes #2777
Also started adding tests, thought it's better late than never to do it