Overhaul command-line parameter functionality. #1428
Conversation
This comment has been minimized.
This comment has been minimized.
digisomni
added
bugfix
enhancement
digisomni
changed the title
digisomni
added
allow-build-upload
|
The following links are available: build (windows-latest, full) build (self-hosted_debian-11_aarch64, full)
build (macOS-10.15, full) build (ubuntu-18.04, full)
|
digisomni
changed the title
Penguin-Guru
force-pushed
the
FixingParameters
branch
from
a1a9705
to
87768cc
Compare
|
The following links are available: build (macOS-10.15, full) build (ubuntu-18.04, full)
|
|
The following links are available: build (ubuntu-18.04, full)
build (macOS-10.15, full) build (windows-latest, full) build (self-hosted_debian-11_aarch64, full)
|
interface/src/main.cpp
Outdated
| ); | ||
| QCommandLineOption scriptsOption( | ||
| "scripts", | ||
| "Set path for defaultScripts. These are probably scripts that run automatically.", |
There was a problem hiding this comment.
This option seems to be broken in the code. The path doesn't seem to be passed on to the script engine:
There was a problem hiding this comment.
Should we try to fix it or comment out the parameters? This one is actually in the docs but I can't tell what it's supposed to do from the description.
https://docs.vircadia.dev/interfaces/native/command-line-parameters.html
There was a problem hiding this comment.
It should be an easy fix. The main issue is that it doesn't actually pass the path to scriptEngines.
There's also the loadDefaultScripts vs loadScripts difference that is suspicious.
There was a problem hiding this comment.
These both call loadScript(), which does not seem to support directories. Maybe it does somehow.
loadScripts reads from the settings before handling "_defaultScriptsOverride", which seems strange.
https://github.com/vircadia/vircadia/blob/master/libraries/script-engine/src/ScriptEngines.cpp#L343
"_defaultScriptsOverride" is protected and only set in ScriptEngines' constructor and I'm not sure how to assign that yet. I pushed the other fixes for you to test.
There was a problem hiding this comment.
Actually, shouldn't this already be assigned here?
https://github.com/Penguin-Guru/vircadia/blob/FixingParameters/interface/src/Application.cpp#L823
There was a problem hiding this comment.
Ok. I'm having a really hard time telling these features apart. The variable names overlap a lot and I'm not sure how the parameters are supposed to be different from each other.
See how --scripts is referenced here: https://github.com/Penguin-Guru/vircadia/blob/FixingParameters/interface/src/Application.cpp#L1929
There was a problem hiding this comment.
loadScripts() does not accept a parameter, and it contains code that handles --defaultScriptsOverride in a way that completely overlaps with the specified functionality of --scripts. The variable names for --scripts suggest it serves the exact same purpose. Perhaps it was only partially implemented, intended to replace --defaultScriptsOverride and mirror the name of the settings value? But --scripts is in the web documentation and --defaultScriptsOverride is not.
There was a problem hiding this comment.
If you have confirmed that --scripts does not work, should I comment it out for now or make it a pseudonym for --defaultScriptsOverride? If we're commenting it out, it should probably also be removed from the web docs.
There was a problem hiding this comment.
I tried it, and as far as I can tell, it's not doing anything. I'll create an issue.
There was a problem hiding this comment.
Ok. I will add a warning to the --help output for now but I suggest commenting --scripts out of the code and removing it from the web docs if no other resolution is determined before this PR is merged. The main goal of this PR is to fix such inconsistencies and I think this is the only one left (hopefully).
interface/src/main.cpp
Outdated
| ); | ||
| QCommandLineOption systemCursorOption( | ||
| "system-cursor", | ||
| "Probably prevents changing the cursor when application has focus." |
There was a problem hiding this comment.
Remove "probably". I'd describe it simply as "Uses the default system cursor". Which is maybe not ideal, but should do for now.
It looks like the cursor can change to a reticle when using an HMD, this should disable that and use whatever the OS wants to use by default.
There was a problem hiding this comment.
I have attempted to fix this. Please let me know if it is resolved after I push the commit.
|
The following links are available:
build (macOS-10.15, client) build (macOS-10.15, full) build (windows-latest, full) build (self-hosted_debian-11_aarch64, full)
|
Penguin-Guru
force-pushed
the
FixingParameters
branch
from
9cc4843
to
3a2646e
Compare
|
@daleglass Should be up to date with master now. I can't test this right now but I assume it will work since there were no conflicts. |
|
The following links are available: build (macOS-10.15, full) build (ubuntu-18.04, full)
build (windows-latest, full) build (self-hosted_debian-11_aarch64, full)
|
|
Needs a merge from master to fix merge conflict. |
|
Go for it. |
|
Tested with the Pantheon Launcher on Windows, it successfully launched however my OpenVR disabling parameters did not work correctly and it launched with SteamVR anyway. https://docs.vircadia.dev/interfaces/native/command-line-parameters the two command line parameters being used are:
|
|
@namark can you see about testing out the command lines apply the necessary fixes/revisions? |
|
Hello! Is this still an issue? |
I don't use any of these so I can't really tell whether or not they're working. If they aren't working, let me know how to test the parameter that's broken and I'll try to look into the handling. Parameter order and descriptions are based on the current documentation.
#1392
#827
#835