Allow watching replays while in queue #2945

BlackYps · 2023-03-16T03:22:05Z

We store game files for replay watching separately if the related option is enabled. This prevents the problems that would occur when the game gets launched with the wrong game version once a match is found.

I also deleted the canUpdate method that always returned true.

codecov · 2023-03-16T04:42:41Z

Codecov Report

Merging #2945 (41e989f) into develop (5c88d82) will increase coverage by 0.14%.
The diff coverage is 72.91%.

❗ Current head 41e989f differs from pull request most recent head 2e408b4. Consider uploading reports for the commit 2e408b4 to get more accurate results

Additional details and impacted files

@@              Coverage Diff              @@
##             develop    #2945      +/-   ##
=============================================
+ Coverage      64.58%   64.72%   +0.14%     
- Complexity      4886     4894       +8     
=============================================
  Files            556      556              
  Lines          20193    20187       -6     
  Branches        1080     1077       -3     
=============================================
+ Hits           13042    13067      +25     
+ Misses          6532     6498      -34     
- Partials         619      622       +3

Impacted Files	Coverage Δ
...ver/client/patch/SimpleHttpFeaturedModUpdater.java	`16.66% <0.00%> (ø)`
...client/patch/SimpleHttpFeaturedModUpdaterTask.java	`0.00% <0.00%> (ø)`
...orever/client/preferences/ForgedAlliancePrefs.java	`81.53% <ø> (-1.32%)`	⬇️
...ever/client/preferences/ui/SettingsController.java	`81.65% <ø> (+2.85%)`	⬆️
...rever/client/patch/GameBinariesUpdateTaskImpl.java	`47.82% <62.50%> (+0.95%)`	⬆️
...in/java/com/faforever/client/game/GameService.java	`70.56% <69.81%> (+6.23%)`	⬆️
...va/com/faforever/client/patch/GameUpdaterImpl.java	`87.50% <94.11%> (+5.83%)`	⬆️
...orever/client/config/FeaturedModUpdaterConfig.java	`100.00% <100.00%> (ø)`
...com/faforever/client/fa/ForgedAllianceService.java	`81.42% <100.00%> (+1.74%)`	⬆️
...va/com/faforever/client/preferences/DataPrefs.java	`100.00% <100.00%> (ø)`

... and 2 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c88d82...2e408b4. Read the comment docs.

BlackYps · 2023-03-16T05:04:41Z

Seems to be working now. Still needs tests

Garanas · 2023-03-16T11:05:46Z

There are more places where you can have issues:

(1) Shader cache
(2) Preference file
(3) Executable

(1) and (3) can be a problem when you were watching a replay from a different featured mod. (2) can be a problem where one game is writing to it (because you're closing it, for example) while the next game is trying to read it (because you're starting it).

Are these taken into account?

BlackYps · 2023-03-16T11:19:24Z

No, can you elaborate how to take these into account?

Garanas · 2023-03-16T11:27:00Z

For (1) and (2) the solution would be to delay starting the ladder instance until we're sure that the replay instance has no remaining file handles lingering anywhere. We kill the process, wait a second or two and then launch the game to join ladder. I assume this already happens to some degree.

(3) is more difficult, if I'm not mistaken then the files are updated as you join the queue. This includes the executable. Updating the executable would therefore always fail when it needs to be changed: either when you start the queue while in a replay or when you start the replay while in a queue. To overcome this you'd need to not just separate the game data files, but also separate the executable. This is not limited to watching replays of different featured mods, it can also happen between game versions (the few days after a release)

BlackYps · 2023-03-16T12:17:09Z

I see there was a misunderstanding. The executable is copied over to a separate directory as well. So 3) shouldn't be an issue. Killing the replay before starting the match seems like a good idea.

Garanas · 2023-03-16T12:44:20Z

I thought with game files you were referring to just the files in the gamedata directory, miss understanding indeed 😄

Sheikah45 · 2023-03-16T13:51:46Z

I think the pref file might still be a concern though. Would that be a problem if a replay is open when the game starts?

BlackYps · 2023-03-16T14:04:37Z

Ideally people don't fiddle with their pref file when they watch a replay anyway, but I understand that this is not always the case. Shouldn't this be addressed by terminating the replay before starting the ladder match?

Sheikah45 · 2023-03-16T15:06:49Z

I didn't think you wanted to terminate the replay

BlackYps · 2023-03-16T15:24:12Z

I didn't originally think of that and it's not implemented yet, but it sounds like the best solution. We circumvent these file issues, it frees up system resources and prevents that people miss that the game is starting because they are in the replay

src/main/java/com/faforever/client/game/GameService.java

Sheikah45

Still needs some changes so people can actually queue with one or more replays started.

Also I don't love the passing of the useReplaysFolder parameter but also not sure how to make it better at the moment.

src/main/java/com/faforever/client/game/GameService.java

src/main/java/com/faforever/client/fa/ForgedAllianceService.java

src/main/java/com/faforever/client/game/GameService.java

src/main/java/com/faforever/client/patch/GameBinariesUpdateTaskImpl.java

BlackYps · 2023-03-18T00:56:34Z

I realized that tracking when a replay is started separately from when an online game is started leads to complicated situations when we still want to make e.g. allowing to join a queue while a replay is running dependend on the preference from the settings.
I'm starting to wonder if it is even worth it to keep this option at all. Having this options introduces if statements all over the place. The additional files in the replay folder is about 200MB per featured mod. So in the worst case we have about 800MB of additional disk space, assuming people actually watch replays from fafdevelop and fafbeta. The cache folder can easily grow to multiple GBs. Each client version alone is just short of 200MB and there are possibly multiple game versions cached as well. We don't really take steps to keep this folder small either, so it seems inconsistent to warn about a feature that will actually have less impact on disk usage than the cache.
We could maybe store our game files for replay watching in the cache folder as well, so they can be easily cleaned and just enable the feature for all people, making the code much nicer.

Sheikah45 · 2023-03-18T01:35:44Z

Yes I was wondering as well if we should just always have the split folders. Then they could be truly separated in the code.

BlackYps · 2023-03-18T10:38:05Z

Sounds good. Why is the patch to the game prefs needed? Do we still need to do this then?
And one more thing I realized. When people are in a custom game lobby and watch a replay they already have the situation that two instances of the game are running. How would we prevent any game prefs issues? On the other hand people have used this feature for a while and so far I can't remember any complaints about issues with that.

Sheikah45 · 2023-03-18T12:06:45Z

Yes the patch to the game prefs is to allow multiple instances of the game to run at once.

And with the situation where people are already in a game the first game instance has a lock on the files so no other instance should be able to touch them. And that is the instance with the actual game they will play so has priority. When starting a game with a replay started the replay would have the lock which might not have the settings the player wants for the game itself which is the issue.

BlackYps · 2023-03-18T13:36:17Z

When we remove the game.prefs edit from the settings we have to check and edit automatically. Where would be a good place for this check?

src/main/java/com/faforever/client/fa/ForgedAllianceService.java

src/main/java/com/faforever/client/game/GameService.java

src/main/java/com/faforever/client/preferences/DataPrefs.java

src/main/java/com/faforever/client/patch/GameUpdaterImpl.java

Sheikah45 · 2023-03-19T14:21:33Z

src/main/java/com/faforever/client/game/GameService.java

+        .thenCompose(gameLaunchResponse -> downloadMapIfNecessary(gameLaunchResponse.getMapName()).thenCompose(aVoid -> {
+              // We need to kill the replay to free the lock on the game.prefs
+              if (isReplayRunning()) {
+                gameKilled = true;


This shouldn't be gameKilled should it?

from my understanding gameKilled prevents the error message to pop up when the game - in this case the replay - terminates with non-zero exit code. This is what we want. Once the game starts gameKilled gets set to false again.

But to me then this should be some other variable. As it is right now this just seems a little confusing since most of the other processes between game and replay are separated

do you want to have this variable renamed to something like faKilled or a second one introduced?

A second one introduced would probably be better

I added a second variable, you can have a look but I am not really convinced that this is a good way to handle it. I think it would be more straightforward to have one variable that is used for games and replays (and named accordingly).

Sheikah45 · 2023-03-28T22:32:17Z

src/main/java/com/faforever/client/game/GameService.java

+    if (exitCode != 0 && !(gameKilled || replayKilled)) {
+      if (exitCode == -1073741515) {
+        notificationService.addImmediateWarnNotification("game.crash.notInitialized");
+      } else {
+        notificationService.addNotification(new ImmediateNotification(i18n.get("errorTitle"), i18n.get("game.crash", exitCode, logFile.map(Path::toString)
+            .orElse("")), WARN, List.of(new Action(i18n.get("game.open.log"), event -> platformService.reveal(logFile.orElse(operatingSystem.getLoggingDirectory()))), new DismissAction(i18n))));
+      }
+    }


The main thing with breaking apart the gameKilled and replayKilled was so that replays would not interfere with this notification for games that crashed.

BlackYps force-pushed the watch-replay-in-queue branch from e09c28b to a99ff28 Compare March 16, 2023 04:54

BlackYps requested a review from Sheikah45 March 16, 2023 13:48

BlackYps commented Mar 17, 2023

View reviewed changes

src/main/java/com/faforever/client/game/GameService.java Outdated Show resolved Hide resolved

Sheikah45 requested changes Mar 17, 2023

View reviewed changes

BlackYps force-pushed the watch-replay-in-queue branch from f248651 to 7497829 Compare March 18, 2023 14:22

BlackYps requested a review from Sheikah45 March 18, 2023 14:28

Sheikah45 requested changes Mar 18, 2023

View reviewed changes

BlackYps requested a review from Sheikah45 March 19, 2023 12:13

Sheikah45 requested changes Mar 19, 2023

View reviewed changes

BlackYps added 5 commits March 27, 2023 12:45

Remove unused canUpdate

a858ae1

Use separate game files for replay watching

e66ca0e

Fix tests

f0c1e1e

Remove experimental label

70a4e7f

Start replay with correct exe

c47d482

BlackYps added 15 commits March 27, 2023 12:47

Fix fa_path.lua location

2bce314

Add tests

fd8c300

Kill fa process on match found

95e14da

Refactor codacy issue

1a185f7

Refactor

afab710

Track offline games separately

e98ebc9

Always use cache folder for replays

064d0ca

Always enable watching single replay

b338a00

Improve tests

fdbf948

Fix test

2c664ac

Automatically check game.prefs on startup

4e9cc39

Store game files for replays in dedicated directory

e45306f

Refactor featuredModUpdater

d99b1d4

Remove allowReplaysWhileInGame option

a9c93c3

Add replayKilled variable

d77d9e5

BlackYps force-pushed the watch-replay-in-queue branch from 2e408b4 to d77d9e5 Compare March 27, 2023 10:50

Fix tests

4a5e90e

BlackYps requested a review from Sheikah45 March 28, 2023 21:14

Sheikah45 requested changes Mar 28, 2023

View reviewed changes

Separate logic for crash messages

a8bbbc4

BlackYps requested a review from Sheikah45 March 28, 2023 23:23

Sheikah45 approved these changes Mar 28, 2023

View reviewed changes

Sheikah45 merged commit b65435b into develop Mar 28, 2023

Sheikah45 deleted the watch-replay-in-queue branch March 28, 2023 23:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow watching replays while in queue #2945

Allow watching replays while in queue #2945

BlackYps commented Mar 16, 2023

codecov bot commented Mar 16, 2023 •

edited

BlackYps commented Mar 16, 2023

Garanas commented Mar 16, 2023

BlackYps commented Mar 16, 2023

Garanas commented Mar 16, 2023 •

edited

BlackYps commented Mar 16, 2023

Garanas commented Mar 16, 2023

Sheikah45 commented Mar 16, 2023

BlackYps commented Mar 16, 2023

Sheikah45 commented Mar 16, 2023

BlackYps commented Mar 16, 2023

Sheikah45 left a comment

BlackYps commented Mar 18, 2023

Sheikah45 commented Mar 18, 2023

BlackYps commented Mar 18, 2023

Sheikah45 commented Mar 18, 2023

BlackYps commented Mar 18, 2023

Sheikah45 Mar 19, 2023

BlackYps Mar 19, 2023

Sheikah45 Mar 23, 2023

BlackYps Mar 23, 2023 •

edited

Sheikah45 Mar 25, 2023

BlackYps Mar 25, 2023

Sheikah45 Mar 28, 2023

Allow watching replays while in queue #2945

Allow watching replays while in queue #2945

Conversation

BlackYps commented Mar 16, 2023

codecov bot commented Mar 16, 2023 • edited

Codecov Report

BlackYps commented Mar 16, 2023

Garanas commented Mar 16, 2023

BlackYps commented Mar 16, 2023

Garanas commented Mar 16, 2023 • edited

BlackYps commented Mar 16, 2023

Garanas commented Mar 16, 2023

Sheikah45 commented Mar 16, 2023

BlackYps commented Mar 16, 2023

Sheikah45 commented Mar 16, 2023

BlackYps commented Mar 16, 2023

Sheikah45 left a comment

Choose a reason for hiding this comment

BlackYps commented Mar 18, 2023

Sheikah45 commented Mar 18, 2023

BlackYps commented Mar 18, 2023

Sheikah45 commented Mar 18, 2023

BlackYps commented Mar 18, 2023

Sheikah45 Mar 19, 2023

Choose a reason for hiding this comment

BlackYps Mar 19, 2023

Choose a reason for hiding this comment

Sheikah45 Mar 23, 2023

Choose a reason for hiding this comment

BlackYps Mar 23, 2023 • edited

Choose a reason for hiding this comment

Sheikah45 Mar 25, 2023

Choose a reason for hiding this comment

BlackYps Mar 25, 2023

Choose a reason for hiding this comment

Sheikah45 Mar 28, 2023

Choose a reason for hiding this comment

codecov bot commented Mar 16, 2023 •

edited

Garanas commented Mar 16, 2023 •

edited

BlackYps Mar 23, 2023 •

edited