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

Major GameScope Refactor #744

Merged
merged 31 commits into from
Feb 18, 2023
Merged

Major GameScope Refactor #744

merged 31 commits into from
Feb 18, 2023

Conversation

sonic2kk
Copy link
Owner

@sonic2kk sonic2kk commented Feb 15, 2023

This PR does three things:

  • Primarily, a major refactor of our GameScope logic. This was long overdue as the GameScope menu grew in complexity, initially it only had a dozen options at most, whereas now it has around 50 options!
    • Removed a lot of repeated grep checks for arguments, as well as the getGameScopePathArg function, with a single function that manages almost all of the GameScope argument checking/value fetching.
    • Logic for fetching these arguments was broken into a separate function, which has separate subfunctions for each GameScope category time, which just makes it easier to manage the GameScope code
  • Adds some new GameScope options added upstream.
    • Add --expose-wayland option as checkbox under Advanced Settings
    • Add --rt (realtime scheduling) option as checkbox under Advanced Settings
    • Add new itm (Inverse Tone Mapping) options under HDR settings
  • Fix some bugs introduced in Major GameScope Additions #722, where options were being mapped incorrectly.
    • --vr-overlay-explicit-name was being mapped incorrectly
    • -m (max scale factor) was being mapped incorrectly
    • --hdr-wide-gammut-for-sdr was not escaped and not being mapped correctly
    • Possibly others I have forgotten by now 😅

Aside from the four or so new options, this PR does not have a big user-facing impact. There should be virtually no user-facing impact from this, it started out as cleanup and I decided to add the new GameScope options in this PR.

TODO:

  • Fix custom cursor paths with spaces causing games not to load (related to using a space " " delimiter in gameScopeArgs -- We need a different way of parsing the GameScope arguments)
  • Fix single-quotes being wiped from GameScope argument paths when the "Save" button is used on the Game Menu
  • Remove unnecessary repeated calls to gameScopeArgs
  • Update langfiles
  • Note on the wiki that custom cursors may not have an effect in all Steam games

Order now matches the UI more closely
Now the argument parsing is separated into functions for each
GameScope 'category' on the UI, it was getting to be a bit of
a pain to manage all of these arguments.
Adds ITM (Inverse Tone Mapping) options to the HDR section.
These were added semi-recently to GameScope.
Also fixes a bug where one of the already-existing arguments was not properly escaped.
Adds '--expose-wayland' option as checkbox.
Adds the three new GameScope HDR options to UI.
Minor code refactor.
We now have a general function to do the grepping for GameScope args.
This is still WIP but has massively reduced the repeated logic for some
checkboxes.

This will be experimented with and expanded for other checkboxes, but
this should significantly simplify the GameScope argument logic.
Also has some other refactoring.
Also fixes a bug where max scale factor was using the wrong flag check.
Also improve getGameScopeArg to use Bash parameter expansion instead of sed.
no idea what this was or when it appeared...
Created setGameScopeVars to help trim down GameScopeGui.
@sonic2kk sonic2kk force-pushed the gamescope-the-adventure-continues branch from 25c545d to 5948c4a Compare February 15, 2023 21:29
@sonic2kk sonic2kk force-pushed the gamescope-the-adventure-continues branch from a93540a to a85fdcb Compare February 15, 2023 22:00
@sonic2kk sonic2kk marked this pull request as ready for review February 15, 2023 22:17
@sonic2kk
Copy link
Owner Author

Side note: I am hoping to bring down the amount of code in STL overtime, it's getting so big that it's kind of a problem. It's not going to happen all at once and this is my current compromise with breaking down STL into separate files; instead I'll try to make the code more managable, remove duplication where I find it and overall give more consideration to this when creating new features.

@sonic2kk sonic2kk mentioned this pull request Feb 16, 2023
@sonic2kk
Copy link
Owner Author

sonic2kk commented Feb 16, 2023

Passing a cursor image doesn't seem to work in this PR, will investigate before merging (stats path works, not sure about VR icon).

The cursor path appears in the GameScope arguments but it isn't read correctly when going back into the GameScope GUI. Games also don't seem to load when selecting a custom cursor. This is currently present on master from what I can see. I recall issues surrounding this before but I thought they were fixed by adding the single quotes around the path. Odd.

I will investigate and fix this in this PR.


Cursor works fine with args like -w 1280 -h 720 -W 1280 -H 720 --cursor '/blah/blah' --another-doubledash-arg

But fails with args like -w 1280 -h 720 -W 1280 -H 720 --cursor '/blah/blah' -U --sharpness-value 5

This is because it incorrectly picks up any following single-dash args (like -U) up until the next double-dash arg. And of course /path/to/cursor.png -U is not a valid path!

…ctly

If there was a single-dash flag after a double-dash flag,
for example '--cursor /path/to/cursor.png -U --sharpness-value 5',
then the cursor path would contain the following single-dash flag(s)
up until the next double dash flag, meaning the path would be read as
invalid.

This fixes the issue. It was not introduced in this PR but has been
fixed!
@sonic2kk
Copy link
Owner Author

sonic2kk commented Feb 16, 2023

Custom cursor paths without spaces don't crash games anymore and are correctly parsed, however GameScope does not set the custom cursor for some reason, even though the launch command looks valid...

The paths with spaces not working is probably an issue to do with how we use mapfile to create the arguments in gameScopeArgs: mapfile -d " " -t -O "${#GAMESCOPEARGSARR[@]}" GAMESCOPEARGSARR < <(printf '%s' "$ARGSTRING") -- Note the -d " " part, where the delimiter is being set to be a space.

We'll need to find another way of splitting the arguments, as a space delimiter won't work when we're dealing with paths.

@sonic2kk
Copy link
Owner Author

sonic2kk commented Feb 16, 2023

Update: the custom cursor seems to not be set for Steam either, but it works outside of Steam. How strange!

I tested this by passing this as a launch argument to a game (Cookie Clicker), and launching it with Proton 7.0-6: gamescope -w 1280 -h 720 -W 1280 -H 720 --cursor '/path/to/cursor' -- %command%

However, from the command line I tried gamescope -w 1280 -h 720 -W 1280 -H 720 --cursor '/path/to/cursor' -- supertuxkart (and also vkcube) and they both worked fine.


I also discovered an issue related to cursor paths as well:

  1. Set a custom cursor in the GameScope UI and save it
  2. Go to the Game Menu and press the "Save" button
  3. The GameScope arguments displayed on the Game Menu will show --cursor /path/to/cursor without the single quotes!

That is probably going to be a nasty bug to try and fix as I suspect it's related to how the GameScope options are written out to the file again, and I would guess it just generically parses GameScope options and values without considering that they may be wrapped in quotes. Very annoying.

We can't use backslashes either, as they won't show up in the first place on the Game Menu textbox, so they have no effect. Double quotes also have the same problem.


So the current priority is fixing custom cursor paths with spaces.

Once that is fixed, I will look into the quotes issue, but I don't have high hopes of fixing it yet.

Previously we used 'mapfile', which we can't use now because
some arguments may have spaces in their path.

We use new logic now using readarray and some sed magic to
break up the gamescope arguments and build each argument and its value
separately.
@sonic2kk
Copy link
Owner Author

I tried a solution with eb897a4, but this meant that regular gamescope argument values (e.g. --sharpness-value 5) would cause crashing.

I don't know of a good way to solve this. I guess we'll just note that you shouldn't use a path with spaces?

We use some fancy sed and grep commands to do this.
ChatGPT actually generated the sed command to surround unquoted paths with quotes.
@sonic2kk
Copy link
Owner Author

Phew, jeez. After a lot of fiddling I managed to resolve the two remaining issues:

  • We now forcefully quote any paths with single quotes if they do not already have single quotes
  • As a result, we can now pass file paths with spaces... so long as that path doesn't have single quotes!

I actually ended up using ChatGPT to generate the sed command: sed "s:\(/\S* \S*\):'\1':g" - I tried writing it myself, then tried adapting solutions from StackOverflow, and I couldn't do it. After about 10 minutes of trying to get ChatGPT to understand the problem, it did and came up with a solution that worked.

I am a little unhappy about the file paths not being able to use single quotes, but it's a trade-off and I don't feel like spending more time on this. I think it's better to allow spaces but not single quotes in the end, and we probably would've had to have disallowed single quotes anyway if we didn't allow for paths with spaces. The English langfile has been updated to note that paths should not contain special characters, and it's an edge case anyway imo.

Great progress was made today at the cost of a lot of sanity :-) I will continue to test over the weekend to ensure various cases work. The code I wrote to allow for this is not the cleanest, I may revisit and try to clean it up before I consider merging. There are some repeated checks that could be cleaned up at the very least, and I am not the most pleased with having to change the IFS value. The core of the rest of the functionality is probably a necessity, like using a copy of the gamescope array to edit the values in the array, and having to use unset to clear the arrays before we try to reset them. But I will revisit nonetheless, and as well I will do thorough testing to ensure "regular" GameScope functionality still works.

What an adventure this has been :-)

@sonic2kk
Copy link
Owner Author

I want to try and update the logic for making GameScope calls to make it less redundant, it seems like we make a lot of calls out to gameScopeArgs. Not sure yet how feasible this is as STL could call it in various places for good reason (to make sure it's always set regardless of what "entry point" there may be).

@sonic2kk
Copy link
Owner Author

After some investigation, it seems like this is called twice because of how we call buildCustomCmdLaunch and setCommandLaunchVars. It's not a huge deal, we can leave this for now and potentially fix it in future. It doesn't seem to be impacting anything massively at the moment.

I will do some final testing before merging.

@sonic2kk
Copy link
Owner Author

sonic2kk commented Feb 18, 2023

I noticed Terraria does not exit cleanly with GameScope enabled, but that happens here and on master so it likely isn't a regression caused by this PR. I will still investigate though.

EDIT: Tested Hammerwatch (another native game) and To the Moon (a native game that relies on the SLR to work) and both exit cleanly without STL, with STL master, and with this PR. So I think Terraria was just an edge case :-)

@sonic2kk
Copy link
Owner Author

sonic2kk commented Feb 18, 2023

Just pushed a commit that updates all the langfiles. It also notes in the tooltip concisely enough that GameScope may not use the custom cursor for all games, which should eliminate the need to document this on the wiki.

This seems ready to merge :-) I would've liked a better way to refactor the gameScopeArgs function that builds the parameters now, but I think we'll have to live with it for now. Maybe in future someone could contribute improvements, or future-sonic2kk if he's feeling brave.

@sonic2kk sonic2kk merged commit fe5d86b into master Feb 18, 2023
@sonic2kk sonic2kk deleted the gamescope-the-adventure-continues branch February 25, 2023 14:42
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.

1 participant