feature/Lucker-misc_RoundFramework #2

Merged
para merged 8 commits from feature/Lucker-misc_RoundFramework into master 2023-08-07 05:13:13 +00:00
Owner
  • Adds support for minigames ending
  • Adds support for round ending once all minigames in a round have been played
    • Number of minigames in a round is available as a convar
  • Adds LuckerStats component that consists of score for now
  • updates the Lucker.Entity definition. It now consists of a public property with custom getter/setter, where when set, the target entity's owner is properly set to the Lucker.
    • Also has a networked backing property with autogenerated getter/setter
  • Updates russian roulette to properly end and clean up
  • Pulls in menu files from FPS template
  • Changes minigame manager to operate on types and only create before loading a minigame, to avoid cluttering the world and worrying about constructor vs spawn
* Adds support for minigames ending * Adds support for round ending once all minigames in a round have been played * Number of minigames in a round is available as a convar * Adds LuckerStats component that consists of score for now * updates the Lucker.Entity definition. It now consists of a public property with custom getter/setter, where when set, the target entity's owner is properly set to the Lucker. * Also has a networked backing property with autogenerated getter/setter * Updates russian roulette to properly end and clean up * Pulls in menu files from FPS template * Changes minigame manager to operate on types and only create before loading a minigame, to avoid cluttering the world and worrying about constructor vs spawn
conco added the enhancement label 2023-08-04 01:36:46 +00:00
fork was assigned by conco 2023-08-04 01:36:46 +00:00
para was assigned by conco 2023-08-04 01:36:46 +00:00
conco added 5 commits 2023-08-04 01:36:46 +00:00
conco requested review from para 2023-08-04 01:36:55 +00:00
conco requested review from fork 2023-08-04 01:36:55 +00:00
conco added 1 commit 2023-08-04 04:04:00 +00:00
para reviewed 2023-08-05 03:59:40 +00:00
@@ -41,0 +48,4 @@
"DisplayName": "Minigames Per Round",
"DefaultValue": "0",
"Description": "The number of minigames played per round",
"Group": "Other",
Member

Why is minimum 1 but default is 0?

Also why can't I comment on multiple lines in Gitea? LAME

Why is minimum 1 but default is 0? Also why can't I comment on multiple lines in Gitea? LAME
Author
Owner

Should now be fixed, looks like something weird with S&box convars and setting them via GUI

Should now be fixed, looks like something weird with S&box convars and setting them via GUI
@@ -17,0 +28,4 @@
if ( value != null )
{
value.Owner = this;
}
Member

According to Bing Chat this can be

value?.Owner = this
According to Bing Chat this can be ```cs value?.Owner = this ```
Author
Owner


Please ask bing chat which version of C# it believes supports this feature

![](https://media.discordapp.net/attachments/501842091983241218/1137254962603241543/image.png) Please ask bing chat which version of C# it believes supports this feature
Member

Yeah that's what I thought. I asked it and it just confidently lies about it and doesn't admit it's wrong. I found this stack overflow article on the topic and they just suggest a setValue() method but that's kinda dumb for one-offs.

Yeah that's what I thought. I asked it and it just confidently lies about it and doesn't admit it's wrong. [I found this stack overflow article on the topic and they just suggest a `setValue()` method but that's kinda dumb for one-offs.](https://stackoverflow.com/questions/35887106/using-the-null-conditional-operator-on-the-left-hand-side-of-an-assignment)
@@ -37,6 +56,7 @@ public partial class Lucker : Entity
player.Owner = client as Entity;
player.Name = client.Name;
var camera = player.Components.Create<RTSCamera>();
var stats = player.Components.Create<LuckerStats>();
Member

nit: unused locals

nit: unused locals
Author
Owner

Should be fixed

Should be fixed
@@ -17,0 +25,4 @@
/// <summary>
/// The players involved in the current minigame
/// </summary>
private List<Lucker> InvolvedPlayers { get; set; }
Member

I think this correctly caches the Lucker list when the minigame is created, which is nice. Although I'm wondering how we deal with Luckers who leave before a minigame is ended. If there's other logic that deletes a Lucker in response to the client disconnecting, will the logic in CleanupPlayerPawns() fail / crash?

Also, I feel like it'd be nice to standardize the naming of these variables to disambiguate references to Players and Luckers. Might as well just name variables after the type to avoid clashing the names.

I think this correctly caches the `Lucker` list when the minigame is created, which is nice. Although I'm wondering how we deal with Luckers who leave before a minigame is ended. If there's other logic that deletes a `Lucker` in response to the client disconnecting, will the logic in `CleanupPlayerPawns()` fail / crash? Also, I feel like it'd be nice to standardize the naming of these variables to disambiguate references to `Player`s and `Lucker`s. Might as well just name variables after the type to avoid clashing the names.
Author
Owner

I agree with the variable naming, and will go through and do that.

I think an investigation into handling client disconnects should be done in another ticket however, as I believe there's both an investigation in how S&box handles a client disconnect by default, and also a discussion in how we handle it, such as keeping the player around for the remainder of the round but assigning them to a bot, only a minigame, leave it up to the minigames, etc.

I agree with the variable naming, and will go through and do that. I think an investigation into handling client disconnects should be done in another ticket however, as I believe there's both an investigation in how S&box handles a client disconnect by default, and also a discussion in how we handle it, such as keeping the player around for the remainder of the round but assigning them to a bot, only a minigame, leave it up to the minigames, etc.
Author
Owner

Variables should be renamed now based on the naming of 'Lucker' as opposed to player for the most part

Variables should be renamed now based on the naming of 'Lucker' as opposed to player for the most part
@@ -26,3 +39,3 @@
if (CheckForMinigames())
{
LoadedMinigame = string.IsNullOrEmpty( minigameName ) ? AvailableMinigames.OrderBy( _ => Guid.NewGuid() ).FirstOrDefault() : TypeLibrary.Create<Minigame>( minigameName );
LoadedMinigame = string.IsNullOrEmpty( minigameName ) ? TypeLibrary.Create<Minigame>(AvailableMinigames.OrderBy( _ => Guid.NewGuid() ).FirstOrDefault().TargetType) : TypeLibrary.Create<Minigame>( minigameName );
Member

nit: more readable on separate lines

nit: more readable on separate lines
Author
Owner

Done

Done
@@ -65,0 +103,4 @@
LoadedMinigame.Cleanup();
CleanupPlayerPawns();
LoadedMinigame = null;
Member

Should we clear InvolvedPlayers here?

Should we clear `InvolvedPlayers` here?
Author
Owner

we can, but I don't believe it would be necessary. Since LoadedMinigame gets nulled out (which actually should be deleted before we do so, need to fix), the manager shouldn't necessarily be doing anything with the list until a new one is loaded (along with a new list), although some additional checks would be nice.

we can, but I don't believe it would be necessary. Since LoadedMinigame gets nulled out (which actually should be deleted before we do so, need to fix), the manager shouldn't necessarily be doing anything with the list until a new one is loaded (along with a new list), although some additional checks would be nice.
Author
Owner

Done

Done
@@ -50,3 +50,3 @@
private const int MinigamesPerRound = 1;
private int MinigamesLeftInRound { get; set; }
[ConVar.Replicated("lucker_minigames_per_round")]
Member

Shouldn't this be on MinigamesPerRound and not MinigamesLeftInRound?

Shouldn't this be on `MinigamesPerRound` and not `MinigamesLeftInRound`?
Author
Owner

Yep, should be fixed now

Yep, should be fixed now
@@ -80,0 +87,4 @@
}
else
{
RoundState = RoundState.NotStarted;
Member

Can we iron out the definition of Round now?

A Round is not one instance of a Minigame, but rather multiple different Minigames, right?

Before a round: settings are configured and the lobby is populated with players waiting to start playing
During a round: multiple different minigames will be played (started and stopped)
After a round: we will show the scoreboard and announce a winner.

Can we iron out the definition of `Round` now? A `Round` is not one instance of a `Minigame`, but rather multiple different `Minigame`s, right? Before a round: settings are configured and the lobby is populated with players waiting to start playing During a round: multiple different minigames will be played (started and stopped) After a round: we will show the scoreboard and announce a winner.
Author
Owner

I'd prefer setting up a round structure in a separate PR due to it moving logic around, but I agree with the definition, along with the idea that after a round ends a new one begins at the first stage again.

I'd prefer setting up a round structure in a separate PR due to it moving logic around, but I agree with the definition, along with the idea that after a round ends a new one begins at the first stage again.
para requested changes 2023-08-05 04:02:49 +00:00
para left a comment
Member

Couple questions and actionable comments

Couple questions and actionable comments
Member

reviewed with para lgtm pending para's comments

reviewed with para lgtm pending para's comments
conco added 1 commit 2023-08-06 02:04:56 +00:00
conco added 1 commit 2023-08-06 02:09:04 +00:00
conco requested review from para 2023-08-06 02:10:38 +00:00
Member

LGTM merging. I've never done this before but I'm assuming we use the default option and not squash or whatever.

LGTM merging. I've never done this before but I'm assuming we use the default option and not squash or whatever.
para closed this pull request 2023-08-07 05:11:43 +00:00
para reopened this pull request 2023-08-07 05:11:49 +00:00
Member

Woops thought it was close -> merge and not the other way around

Woops thought it was close -> merge and not the other way around
para merged commit cc877492d5 into master 2023-08-07 05:13:13 +00:00
para deleted branch feature/Lucker-misc_RoundFramework 2023-08-07 05:13:13 +00:00
Member

Idk if we should be deleting branches after merge but I did for this one. @conco opinion?

Idk if we should be deleting branches after merge but I did for this one. @conco opinion?
Author
Owner

Idk if we should be deleting branches after merge but I did for this one. @conco opinion?

No real thoughts either way from me, the commits and PRs stick around so we aren't losing much getting rid of the branch, and it likely keeps things a bit cleaner than keeping a ton of old branches around.

> Idk if we should be deleting branches after merge but I did for this one. @conco opinion? No real thoughts either way from me, the commits and PRs stick around so we aren't losing much getting rid of the branch, and it likely keeps things a bit cleaner than keeping a ton of old branches around.
Sign in to join this conversation.
No Reviewers
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: TeamLucker/LuckerGame#2