Closed BlackYps closed 1 year ago
codecov[bot]
commented
1 year ago Merging #2945 (41e989f) into develop (5c88d82) will increase coverage by
0.14%. The diff coverage is72.91%.:exclamation: Current head 41e989f differs from pull request most recent head 2e408b4. Consider uploading reports for the commit 2e408b4 to get more accurate results
BlackYps
commented
1 year ago Seems to be working now. Still needs tests
Garanas
commented
1 year ago There are more places where you can have issues:
(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
commented
1 year ago No, can you elaborate how to take these into account?
Garanas
commented
1 year ago 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
commented
1 year ago 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
commented
1 year ago I thought with game files you were referring to just the files in the gamedata directory, miss understanding indeed 😄
Sheikah45
commented
1 year ago 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
commented
1 year ago 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
commented
1 year ago I didn't think you wanted to terminate the replay
BlackYps
commented
1 year ago 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
BlackYps
commented
1 year ago 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
commented
1 year ago 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
commented
1 year ago 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
commented
1 year ago 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
commented
1 year ago 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?
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.