feature/Lucker-misc_RoundFramework #2
Reference in New Issue
Block a user
Delete Branch "feature/Lucker-misc_RoundFramework"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
@@ -41,0 +48,4 @@"DisplayName": "Minigames Per Round","DefaultValue": "0","Description": "The number of minigames played per round","Group": "Other",Why is minimum 1 but default is 0?
Also why can't I comment on multiple lines in Gitea? LAME
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;}According to Bing Chat this can be
Please ask bing chat which version of C# it believes supports this feature
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.@@ -37,6 +56,7 @@ public partial class Lucker : Entityplayer.Owner = client as Entity;player.Name = client.Name;var camera = player.Components.Create<RTSCamera>();var stats = player.Components.Create<LuckerStats>();nit: unused locals
Should be fixed
@@ -17,0 +25,4 @@/// <summary>/// The players involved in the current minigame/// </summary>private List<Lucker> InvolvedPlayers { get; set; }I think this correctly caches the
Luckerlist 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 aLuckerin response to the client disconnecting, will the logic inCleanupPlayerPawns()fail / crash?Also, I feel like it'd be nice to standardize the naming of these variables to disambiguate references to
Players andLuckers. Might as well just name variables after the type to avoid clashing the names.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.
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 );nit: more readable on separate lines
Done
@@ -65,0 +103,4 @@LoadedMinigame.Cleanup();CleanupPlayerPawns();LoadedMinigame = null;Should we clear
InvolvedPlayershere?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.
Done
@@ -50,3 +50,3 @@private const int MinigamesPerRound = 1;private int MinigamesLeftInRound { get; set; }[ConVar.Replicated("lucker_minigames_per_round")]Shouldn't this be on
MinigamesPerRoundand notMinigamesLeftInRound?Yep, should be fixed now
@@ -80,0 +87,4 @@}else{RoundState = RoundState.NotStarted;Can we iron out the definition of
Roundnow?A
Roundis not one instance of aMinigame, but rather multiple differentMinigames, 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.
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.
Couple questions and actionable comments
reviewed with para lgtm pending para's comments
LGTM merging. I've never done this before but I'm assuming we use the default option and not squash or whatever.
Woops thought it was close -> merge and not the other way 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.