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

Clean up pause time counter logic #784

Merged
merged 1 commit into from
Jul 26, 2022
Merged

Clean up pause time counter logic #784

merged 1 commit into from
Jul 26, 2022

Conversation

nickdnk
Copy link
Collaborator

@nickdnk nickdnk commented Jul 21, 2022

  1. Only use one timer for tracking pauses.
  2. Ensure fixed pause configuration takes precedence over combined max pause time.
  3. Remove ability to call mp_pause_match or mp_unpause_match directly as those mess with the pause logic
  4. Renamed a lot of g_ variables to be more consistent and removed some that are no longer needed.
  5. Renamed g_MaxTechPauses to have g_MaxTechPausesCvar like the rest of cvars
  6. Fixed missing Danish translations
  7. Use PrintHintText() instead of in-game pause counters and timers. This makes the code a lot easier to manage and means all pauses are consistently presented the same way. It also allows for unpausing fixed-length pauses, which was otherwise impossible, even for admins.
  8. Add pause_type to the unpause event object
  9. Don't allow calling !unpause if a pause was called during a round; you must now wait until freezetime and the pause count will be correctly consumed. The opposing team, however, can call !unpause immediately if they just want to unpause as soon as the pausing team does.
  10. Don't allow spectators or team_none to ever pause or unpause anything
  11. Removed the "each half" suffix from translations related to pauses resetting. It's not consistently applicable across all languages, and I would suggest we replace it with an additional message which contains a universally translate-able string such as "Note that pauses reset on halftime."
  12. Make sure to unpause when loading a match config.
  13. Don't allow stop command use while admin has paused the game.
  14. Fix [Bug] Pausing During Warmup Countdown To Knife #647

scripting/get5/pausing.sp Outdated Show resolved Hide resolved
@PhlexPlexico
Copy link
Collaborator

PhlexPlexico commented Jul 22, 2022

Okay was testing the pauses and first thing broke lol

Test Case One

Relevant pause vars:

get5_max_pause_time "300"
get5_max_pauses "0"
get5_fixed_pause_time "0"

This should make all pauses last to a total of 300 seconds, and require intervention on both users to resume, or have the timer run its course. Afterwards, it should now allow anymore pauses for the team. Right now, it allows for 1 timeout of 60 seconds on each side, and trying to call !pause again just simply resets the countdown to round start timer to 15, and does not allow for any more timeouts.

Calling a tech pause does the same thing, and the TechPause timer goes up when in freeze time, but stops during round, the match is not paused.

Attached is the server log, will continue with attempting fixed timer max pauses. I will attach this to the issue once I'm done testing as well
serverlog test case one.log

Also, admins force pausing the match have it failed to pause even with using sm_pause:

sm_pause
L 07/21/2022 - 22:16:21: [get5] Calling Get5_OnMatchPaused()
L 07/21/2022 - 22:16:21: [G5WS] Our URL is: https://phlex.avidgamers.me/api/match/%s/pause
L 07/21/2022 - 22:16:21: [G5WS] Trying to create request to url https://phlex.avidgamers.me/api/match//pause
L 07/21/2022 - 22:16:21: [get5] Calling Get5_OnEvent(data={
    "matchid": "3833",
    "team": null,
    "event": "game_paused",
    "map_number": 0,
    "pause_type": "admin"
})
[♣Get5☺] An admin force paused the match.
Preventing call to mp_pause_match. Administrators should use sm_pause/sm_unpause.

Test Case Two

45 second fixed !pause with no limits on pausing is working as expected. However, tech pauses remaining broken in this test, as well as admin/rcon servers calling sm_pause. Attached is log file.

get5_max_pause_time "0"
get5_max_pauses "0"
get5_fixed_pause_time "45"

serverlog_fixed_pause_time_45.log

Test Case Three

get5_max_pause_time "0"
get5_max_pauses "4"
get5_fixed_pause_time "45"

The first timeout is incorrect as it's showing 0/4 timeouts (this currently happens in development!), second timeout shows 2/4. Admins are unable to pause and tech pauses do not happen but the counter is shown to be counting upwards in the LogDebugs until match start, or until someone else calls a !pause.

Attached below is the log file.
serverlog_fixed_pause_time_45_max_4.log

I think hooking into the mp_pause/unpause may not be the play here, due to the fact ServerCommand will always call the hook which immediately prevents it from working as intended. I believe all of the following test cases should work as intended:

get5_max_pause_time "300"
get5_max_pauses "0"
get5_fixed_pause_time "0"
  • This should allow an unlimited number of timeouts, up to 300 seconds total (and stops when the team who had called the pause calls !unpause). Tech pauses and otherwise should not be included in this as they are separate cvars, that only interact with the base (old stock) pause and unpause calls. I believe this one can not use the built in UI, due to the fact we have the timer running and waiting on chat feedback, plus I don't think you can cancel the timeout call once it's on the UI (I've not tested this thoroughly, though).
get5_max_pause_time "0"
get5_max_pauses "0"
get5_fixed_pause_time "45"
  • This should allow unlimited 45 second fixed pause timers. While similar to the above case, this will not allow users to call !unpause, and is truly unlimited. I do believe that the two latter options play hand-in-hand, while the former option is independent of the two.
get5_max_pause_time "0"
get5_max_pauses "0"
get5_fixed_pause_time "0"
  • This should essentially act as a "tech" timeout so to speak as users both need to confirm to continue, or be unpaused by an admin. I don't think the UI/CS:GO supports this, so this would have to be done with chat commands.
get5_max_pause_time "0"
get5_max_pauses "4"
get5_fixed_pause_time "0"
  • This should allow 4 unlimited time pauses, where both users must call !unpause, or be unpaused by an admin.
get5_max_pause_time "300"
get5_max_pauses "4"
get5_fixed_pause_time "0"
  • Last test case, is that a user should have a total pause time of 300, but be able to resume whenever (since UI doesn't support this?), up to 4 times total. If they don't use the 300 seconds, but the four pauses, they can no longer pause (as the code stands in development).

The issue right now with this PR as well, is that the underlying stock bool Pause(... and stock bool Unpause(... are both modified, which inherently changes how the tech pausing works in game as well. I think we should maybe take a step back and try altering only that pause timer for tech pauses and get that working, then maybe continue to experiment with what CS:GO has to offer for tech pauses as well.

I hope this test case helps, and if you need me to test more I can rig up these cases again and keep testing for intended behaviour :)

Edit: Was thinking about this further, and I think we just have to make a design decision as well in regards to these three cvars - get5_fixed_pause_time, get5_max_pause_time and get5_max_pause_time. Right now in development, it currently stands that if get5_fixed_pause_time is set, that takes precedence over get5_max_pause_time, which I think is something we should also just keep, and have it noted. But, get5_max_pauses is still tied to both options, where you can set the total number of timeouts. It's a fairly complicated system since there are so many flexible ways of allowing pauses right now, and it may be difficult pairing it down, but something that I think should be done to make it easier.

Thanks!

@nickdnk nickdnk force-pushed the fix_pause_logic branch 4 times, most recently from 9a9ef34 to f251f4a Compare July 22, 2022 15:54
Copy link
Collaborator

@PhlexPlexico PhlexPlexico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! After discord talks and testing, we even squashed a few bugs in relation to the UI timeout not updating properly. Looks a lot cleaner and everything is working as intended.

@nickdnk
Copy link
Collaborator Author

nickdnk commented Jul 22, 2022

So I'm fairly certain everthing works as we want it to now. Please give this a run, @PhlexPlexico. I've tested it pretty extensively myself. I've managed to merge the timer callback function for both types of pauses and reduce a lot of code that was essentially the same.

Remember to copy in translations; I added a bunch more.

Also: Squashed and force-pushed.

Edit: Something to consider. We currently just track a tech pause's length and reset these trackers on unpause. There is no "combined tech pause" time, which is slightly inconsistent compared to tactical pauses, but if this is what we want, then it does work as intended.

Edit 2: With the PrintHintText solution I use now for pauses with timers that don't use the in-game pause state, we might be able to get rid of those entirely. It would make it easier for us to manage our own pause state and would also allow admins or users to unpause fixed-duration pauses. Edit again: I implemented this and it works beautifully.

@nickdnk nickdnk force-pushed the fix_pause_logic branch 2 times, most recently from 4595e3e to bfb4aaa Compare July 23, 2022 00:10
@nickdnk nickdnk requested a review from PhlexPlexico July 23, 2022 00:10
@nickdnk nickdnk force-pushed the fix_pause_logic branch 3 times, most recently from 90e3f6c to a91af96 Compare July 25, 2022 02:34
@PhlexPlexico
Copy link
Collaborator

Overall this still looks good to me. I'll test this tomorrow.

In regards to the inconsistencies with tech pause time tracking and max tactical pauses, this is intentional. I believe the tracked timing is more of an independent Get5 features, where technical pauses already had precedence in the way it's implemented (right now, it's identical to ESEA), and was requested to be implemented like this by users. So I believe we should keep it as is :)

@nickdnk
Copy link
Collaborator Author

nickdnk commented Jul 25, 2022

Okay, so we can get rid of the array-logic for technical pause time and just use a single counter like we do with the fixed pause timer. If it's not supposed to track time used per team, we don't need another variable for it.

@nickdnk nickdnk force-pushed the fix_pause_logic branch 2 times, most recently from 003c5a6 to ae4c189 Compare July 25, 2022 18:19
Copy link
Collaborator

@PhlexPlexico PhlexPlexico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@nickdnk nickdnk force-pushed the fix_pause_logic branch 2 times, most recently from 7c469a7 to 3b49042 Compare July 26, 2022 01:48
@nickdnk nickdnk requested review from PhlexPlexico and splewis and removed request for PhlexPlexico July 26, 2022 02:43
@nickdnk nickdnk self-assigned this Jul 26, 2022
@@ -1,8 +1,17 @@
public bool Pauseable() {
return g_GameState >= Get5State_KnifeRound && g_PausingEnabledCvar.BoolValue;
return g_GameState == Get5State_Live && g_PausingEnabledCvar.BoolValue;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Game can now only be paused when live. If you want a pause during knife, you must wait until live freezetime. I'm not 100% sure we want this, but it seems like calling a pause right after you readied your team is stupid. Admin can always pause.

Things can change in the 20 seconds between the match officially begins and the knife freeze time. I would rather this change not be made.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's also fine to let this go back to >=knife. I originally made this change to reduce logical complexity when refactoring, but it's been cleaned up so much that I don't see a problem with this. I'll just have to test it once.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think one other test that should be done is to check the warmup countdown right after the knife decision, as that reduces the amount of fixed pauses a team has per half, as reported here, without pausing the game - #647 so maybe test this as well, just in case this still happens then? I think with this refactoring, that may no longer be an issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that the game is restarted when the knife round ends, which I think unpauses? Will have to test.

It might actually be a bit of a problem to work around if that is in fact the case. @splewis is it a big deal to just let people pause in the freeze time after going live? They're going to end up paused at the same stage anyway, they just can't do it while still knifing for sides.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, the problem is of course if you want to pause in the knife freezetime? Because yea, we wouldn't allow that currently.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be solvable by changing the if to if (g_GameState >= Knife && g_GameState != WaitingForKnifeDecision).

Copy link
Collaborator Author

@nickdnk nickdnk Jul 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@splewis Fixed. You can pause during live, knife, waiting for knife decision and going live. I tested for #647 and it works in all cases I can think of.

scripting/get5/pausing.sp Outdated Show resolved Hide resolved
@nickdnk nickdnk linked an issue Jul 26, 2022 that may be closed by this pull request
Remove g_PauseTimeUsed
Only fire necessary timers
Ensure fixed pause configuration takes precedence over combined max pause time
Redirect sm_tech to admin pause if used in console
Prevent direct calls to mp_pause_match and mp_unpause_match, force sm_pause/sm_unpause
Remove g_TeamGivenTechPauseCommand array
Add g_PausingTeam and g_PauseType to debug output
Prevent multiple pauses from being triggered at the same time
Prevent spectators and team none from ever calling pause or unpause
Renamed g_MaxTechPauseTime to g_MaxTechPauseDurationCvar for consistency
Renamed g_MaxPausesCvar to g_MaxTacticalPausesCvar for consistency
Renamed g_TechPausedTimeOverride to g_TechnicalPauseTimeUsed, as that's what it is
Renamed g_TeamPauseTimeUsed to g_TacticalPauseTimeUsed
Renamed g_TechPauseTimeUsed to g_TechnicalPauseTimeUsed
Renamed g_TeamPausesUsed to g_TacticalPausesUsed
Renamed g_TeamTechPausesUsed to g_TechnicalPausesUsed
Add print-to-all localized hints for all pauses, ditched the in-game pause state as it's buggy
Add util function for converting seconds to minutes:seconds
Ensure unpausing when loading a match
Refactored translations and removed redundant chat-text
Allow admin to pause even if pausing is disabled
Add team name to max pause/pause time used.
Add backup pause type hint
Add stop command not enabled translation
Don't allow stop command during admin pause
Automatically unpause if max pauses get set to a lower value than already consumed while a pause is active
Adjust danish translations
Adjust game state pause permissions
@nickdnk nickdnk merged commit aea2bab into development Jul 26, 2022
@nickdnk nickdnk deleted the fix_pause_logic branch July 26, 2022 23:44
PhlexPlexico added a commit to PhlexPlexico/get5 that referenced this pull request Jul 27, 2022
* Progress with documentation

* link get5_scrim to scrim template

* Fix "Client x is not in game" error on player disconnect

* Minor doc updates

* Use user ID in player object (splewis#776)

* Use the user ID in the stat-tracking logic for grenades, as this is unique within the scope of the server. Avoids having to compare steamID strings.

* Fix coaching problems generated by `ChangeClientTeam` in 0.9 (splewis#775)

Fix coach placement problems related to auto-joining of teams.

* Also remove player from coaches when removed from teams (splewis#778)

* Fix typo in translation string (splewis#779)

* Add !tac alias for !pause (splewis#782)

* Adjust EnsurePausedWarmup and renamed it to EnsureIndefiniteWarmup (splewis#777)

* Remove extraneous calls to mp_warmup_pausetimer 1 and mp_do_warmup_period 1 in EnsurePausedWarmup
* Rename EnsurePausedWarmup to more appropriate EnsureIndefiniteWarmup since no pausing takes place

* Doc progress

* Attempt to fix relation resolution in docs index page (/get5 problem)

* Hook bots to damage taken (splewis#785)

* Fix suicide logic (again) (splewis#787)

Add bomb parameter to player death event
Make attacker and weapon nullable on player death event
Add self to array of victims for grenades

* Suicide stat was not being used at all
Refactor suicide logic again

* Once again fixing suicide because molotovs are weapon id 0 (thanks valve)
Fix bad hooking of SDKHook_OnTakeDamageAlive when bots are in the game

* SDKHook on plugin reload (splewis#789)

Allow bots to be in damage report
Don't increment stats for bots
Fix SDK rehooking on plugin reload

* Clean up pause time counter logic: (splewis#784)

Remove g_PauseTimeUsed
Only fire necessary timers
Ensure fixed pause configuration takes precedence over combined max pause time
Redirect sm_tech to admin pause if used in console
Prevent direct calls to mp_pause_match and mp_unpause_match, force sm_pause/sm_unpause
Remove g_TeamGivenTechPauseCommand array
Add g_PausingTeam and g_PauseType to debug output
Prevent multiple pauses from being triggered at the same time
Prevent spectators and team none from ever calling pause or unpause
Renamed g_MaxTechPauseTime to g_MaxTechPauseDurationCvar for consistency
Renamed g_MaxPausesCvar to g_MaxTacticalPausesCvar for consistency
Renamed g_TechPausedTimeOverride to g_TechnicalPauseTimeUsed, as that's what it is
Renamed g_TeamPauseTimeUsed to g_TacticalPauseTimeUsed
Renamed g_TechPauseTimeUsed to g_TechnicalPauseTimeUsed
Renamed g_TeamPausesUsed to g_TacticalPausesUsed
Renamed g_TeamTechPausesUsed to g_TechnicalPausesUsed
Add print-to-all localized hints for all pauses, ditched the in-game pause state as it's buggy
Add util function for converting seconds to minutes:seconds
Ensure unpausing when loading a match
Refactored translations and removed redundant chat-text
Allow admin to pause even if pausing is disabled
Add team name to max pause/pause time used.
Add backup pause type hint
Add stop command not enabled translation
Don't allow stop command during admin pause
Automatically unpause if max pauses get set to a lower value than already consumed while a pause is active
Adjust danish translations
Adjust game state pause permissions

* Bring Danish translations up to speed.

* CI Integration For Releases/Nightlies (splewis#773)

Add support for automatic pre-release and release builds on GitHub Actions

* Update version number to include a commit hash through Actions. (splewis#790)

CI adjustments
Add changelog

* Nightly tags (#24)

* Add release.yml to generate release notes.

* Adjust changelogs.

* Nightly tags (#25)

* Add release.yml to generate release notes.

* Adjust changelogs.

* Test changelogs.

* Add release.yml to generate release notes. (#26)

Co-authored-by: Nicolai Cornelis <nickdnk@hotmail.com>
Co-authored-by: Nicolai Cornelis <nickdnk@users.noreply.github.com>
@nickdnk nickdnk added this to the 0.9 milestone Jul 29, 2022
nickdnk added a commit that referenced this pull request Jul 30, 2022
* Progress with documentation

* link get5_scrim to scrim template

* Fix "Client x is not in game" error on player disconnect

* Minor doc updates

* Use user ID in player object (#776)

* Use the user ID in the stat-tracking logic for grenades, as this is unique within the scope of the server. Avoids having to compare steamID strings.

* Fix coaching problems generated by `ChangeClientTeam` in 0.9 (#775)

Fix coach placement problems related to auto-joining of teams.

* Also remove player from coaches when removed from teams (#778)

* Fix typo in translation string (#779)

* Add !tac alias for !pause (#782)

* Adjust EnsurePausedWarmup and renamed it to EnsureIndefiniteWarmup (#777)

* Remove extraneous calls to mp_warmup_pausetimer 1 and mp_do_warmup_period 1 in EnsurePausedWarmup
* Rename EnsurePausedWarmup to more appropriate EnsureIndefiniteWarmup since no pausing takes place

* Doc progress

* Attempt to fix relation resolution in docs index page (/get5 problem)

* Hook bots to damage taken (#785)

* Fix suicide logic (again) (#787)

Add bomb parameter to player death event
Make attacker and weapon nullable on player death event
Add self to array of victims for grenades

* Suicide stat was not being used at all
Refactor suicide logic again

* Once again fixing suicide because molotovs are weapon id 0 (thanks valve)
Fix bad hooking of SDKHook_OnTakeDamageAlive when bots are in the game

* SDKHook on plugin reload (#789)

Allow bots to be in damage report
Don't increment stats for bots
Fix SDK rehooking on plugin reload

* Clean up pause time counter logic: (#784)

Remove g_PauseTimeUsed
Only fire necessary timers
Ensure fixed pause configuration takes precedence over combined max pause time
Redirect sm_tech to admin pause if used in console
Prevent direct calls to mp_pause_match and mp_unpause_match, force sm_pause/sm_unpause
Remove g_TeamGivenTechPauseCommand array
Add g_PausingTeam and g_PauseType to debug output
Prevent multiple pauses from being triggered at the same time
Prevent spectators and team none from ever calling pause or unpause
Renamed g_MaxTechPauseTime to g_MaxTechPauseDurationCvar for consistency
Renamed g_MaxPausesCvar to g_MaxTacticalPausesCvar for consistency
Renamed g_TechPausedTimeOverride to g_TechnicalPauseTimeUsed, as that's what it is
Renamed g_TeamPauseTimeUsed to g_TacticalPauseTimeUsed
Renamed g_TechPauseTimeUsed to g_TechnicalPauseTimeUsed
Renamed g_TeamPausesUsed to g_TacticalPausesUsed
Renamed g_TeamTechPausesUsed to g_TechnicalPausesUsed
Add print-to-all localized hints for all pauses, ditched the in-game pause state as it's buggy
Add util function for converting seconds to minutes:seconds
Ensure unpausing when loading a match
Refactored translations and removed redundant chat-text
Allow admin to pause even if pausing is disabled
Add team name to max pause/pause time used.
Add backup pause type hint
Add stop command not enabled translation
Don't allow stop command during admin pause
Automatically unpause if max pauses get set to a lower value than already consumed while a pause is active
Adjust danish translations
Adjust game state pause permissions

* Bring Danish translations up to speed.

* CI Integration For Releases/Nightlies (#773)

Add support for automatic pre-release and release builds on GitHub Actions

* Update version number to include a commit hash through Actions. (#790)

CI adjustments
Add changelog

* Add release.yml to generate release notes. (#791)

* Remove Weapon nullability because of molotovs being ID 0. (#792)

* Consider "scrim" and "manual" matchID to be the same as no match ID in MySQL context (#795)

* Adjust Danish

* Simplify suicide logic (#798)

Refactor suicide logic to be simpler.

* Add build output from pull requests (#799)
Adjust output filenames to match version
Set commit length to 7 instead of 6 to match changelog tags
Pr head sha for PR build

* Add function to check for new version of get5 (#786)

Add dev/prerelease warning

* Docs almost done (#800)

* Fix coach game prop when reconnect or calling !stop from match. (#802)

Co-authored-by: PhlexPlexico <flexing@phlexplexi.co>
nickdnk added a commit that referenced this pull request Jul 31, 2022
Remove g_PauseTimeUsed
Only fire necessary timers
Ensure fixed pause configuration takes precedence over combined max pause time
Redirect sm_tech to admin pause if used in console
Prevent direct calls to mp_pause_match and mp_unpause_match, force sm_pause/sm_unpause
Remove g_TeamGivenTechPauseCommand array
Add g_PausingTeam and g_PauseType to debug output
Prevent multiple pauses from being triggered at the same time
Prevent spectators and team none from ever calling pause or unpause
Renamed g_MaxTechPauseTime to g_MaxTechPauseDurationCvar for consistency
Renamed g_MaxPausesCvar to g_MaxTacticalPausesCvar for consistency
Renamed g_TechPausedTimeOverride to g_TechnicalPauseTimeUsed, as that's what it is
Renamed g_TeamPauseTimeUsed to g_TacticalPauseTimeUsed
Renamed g_TechPauseTimeUsed to g_TechnicalPauseTimeUsed
Renamed g_TeamPausesUsed to g_TacticalPausesUsed
Renamed g_TeamTechPausesUsed to g_TechnicalPausesUsed
Add print-to-all localized hints for all pauses, ditched the in-game pause state as it's buggy
Add util function for converting seconds to minutes:seconds
Ensure unpausing when loading a match
Refactored translations and removed redundant chat-text
Allow admin to pause even if pausing is disabled
Add team name to max pause/pause time used.
Add backup pause type hint
Add stop command not enabled translation
Don't allow stop command during admin pause
Automatically unpause if max pauses get set to a lower value than already consumed while a pause is active
Adjust danish translations
Adjust game state pause permissions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Pausing During Warmup Countdown To Knife
3 participants