Created attachment 1157 [details]
no-join warmup period

I wasn't sure what g_restarted was used for other than keeping track of whether a warmup should be done, so I kept it to be safe.

Created attachment 1339 [details]
Tournament style warmup

This patch adds full Quake 3, tournament style warmup functionality. Stub cvars were resurrected were applicable:

* g_doWarmup 1|0 -- Enable warmup, will only take effect for the next match.
* g_warmup [msec] -- Time to wait before starting the match after players have joined or ready'd up.
* g_doWarmupReady 1|0 -- Require players to \ready before starting the match. There is also a corresponding \notready command to cancel ready state.

In order to implement this, a new session cvar had to be added (sess.warmupTeam). This patch includes client and server-side changes.

Created attachment 1340 [details]
Tournament style warmup (2)

Modified !allready to work in warmup.
Spectators no longer see the warmup prompt.

Created attachment 1341 [details]
Tournament style warmup (3)

Modified the patch to disallow damage against buildables and deconstruction during warmup.

This looks pretty good, but I have a couple of questions:

1) I know it is pre-existing, but won't CS_CLIENTS_READY overflow as a 32-bit integer?  If it is what I think it is, it could use the recently added BG_ClientList*() functions.

2) When using g_doWarmupReady, there should be a way for a client to ready-up without having to issue the console command "/ready".  Maybe this means just adding a default bind (e.g. Enemy Territory uses F3), but it would be better imo to hijack a mouse button (maybe mouse3) when in warmup mode and indicate it in the cg_tutorial.c text.

(In reply to comment #5)
> 1) I know it is pre-existing, but won't CS_CLIENTS_READY overflow as a 32-bit
> integer?  If it is what I think it is, it could use the recently added
> BG_ClientList*() functions.

If there is an issue with this part you may want to revise the intermission ready code because I stole it straight from there. It won't overflow because the loop has the: if(i < 16) part in it. If that happens though, some clients won't have their ready status correctly reported. This is the worst that can happen though.

> 2) When using g_doWarmupReady, there should be a way for a client to ready-up
> without having to issue the console command "/ready".  Maybe this means just
> adding a default bind (e.g. Enemy Territory uses F3), but it would be better
> imo to hijack a mouse button (maybe mouse3) when in warmup mode and indicate it
> in the cg_tutorial.c text.

This *is* the Tremulous way but any mouse button we hijack would be unusable for whatever it was used in-game. If we hijack mouse3 for instance, sniping advanced goons will ready up unintentionally. I can't think of a _cleaner_ way to do it other than the standard ET F3/F4.

I patched this into Haven of Relics mod 7 and with the ready mode enabled, people were pretty confused. If there is a default bind then it will work fine but right now it says "Press \ready to ready up" which is very strange. In a match setting where this is intended for use I don't imagine it will pose any problems.

> Quake 3-style warmup times don't really apply to Tremulous
Apparently someone disagreed. Changed summary.


>+  if( level.warmupTime != 0 ) {
>+    if( level.numAlienClients < 1 || level.numHumanClients < 1 ) {
>+      if( level.warmupTime != -1 ) {
>+        G_LogPrintf( "Warmup:\n" );
>+        level.warmupTime = -1;
>+      }
>+      trap_SetConfigstring( CS_WARMUP, va("%i", level.warmupTime) );
This happens in SP_worldspawn

>+      return; // still waiting for team members
>+    }
>+
>+    if( level.warmupTime == 0 )
>+      return;
level.warmupTime can't be 0 here.

>+    // if the warmup is changed at the console, restart it
>+    if( g_warmup.modificationCount != level.warmupModificationCount ) {
>+      level.warmupModificationCount = g_warmup.modificationCount;
This already happens in G_UpdateCvars.

>+      level.warmupTime = -1;
>+    }
>+
Will this even happen?

>+    // if the warmup time has counted down, restart
>+    if( level.time > level.warmupTime ) {
That should probably be ">=" (not that it makes a huge difference).

>+        if( cl->pers.connected != CON_CONNECTED ||
>+            cl->ps.stats[ STAT_PTEAM ] == PTE_NONE )
>+          continue;
>+
That should probably be pers.teamSelection instead

>+      level.warmupTime += 10000;
Huh?

G_admin_allready stuff: blah. Just use readyToExit for both intermission and warmup and change
if( !level.intermissiontime )
to
if( !level.intermissiontime || level.warmupTime != -1 )
and make the message more generic. If the logic is right, CheckTournament should detect that warmup has ended the next frame.

>+  // if the match is starting, join the selected team
>+  if ( !level.warmupTime && client->sess.warmupTeam != PTE_NONE ) {
>+    G_ChangeTeam( ent, client->sess.warmupTeam );
>+    client->sess.warmupTeam = PTE_NONE;
>+  }
>+
This should probably be in ClientBegin, not ClientConnect.

>+  if( ready )
>+    trap_SendServerCommand( -1, va( "print \"%s^7 is ready\n\"",
>+                                    ent->client->pers.netname ) );
>+  else
>+    trap_SendServerCommand( -1, va( "print \"%s^7 is NOT ready\n\"",
>+                                    ent->client->pers.netname ) );
UnnamedPlayer^7 is ready
UnnamedPlayer^7 is NOT ready
UnnamedPlayer^7 is ready
UnnamedPlayer^7 is NOT read
x100


You ignore g_doWarmup and introduce g_doWarmupReady instead. That kind of makes g_doWarmup meaningless.

Thanks for looking at the patch. I have to point out that most of it is from ioquake3 and appendix Tremulous code. My addition is to add it back in and the Ready system.

(In reply to comment #7)
> >+  if( level.warmupTime != 0 ) {
> >+    if( level.numAlienClients < 1 || level.numHumanClients < 1 ) {
> >+      if( level.warmupTime != -1 ) {
> >+        G_LogPrintf( "Warmup:\n" );
> >+        level.warmupTime = -1;
> >+      }
> >+      trap_SetConfigstring( CS_WARMUP, va("%i", level.warmupTime) );
> This happens in SP_worldspawn

This part resets the warmup countdown if, at any stage of warmup, all players leave one team. SP_worldspawn happens at the start of the map only.

> >+      return; // still waiting for team members
> >+    }
> >+
> >+    if( level.warmupTime == 0 )
> >+      return;
> level.warmupTime can't be 0 here.

That's true...

> >+    // if the warmup is changed at the console, restart it
> >+    if( g_warmup.modificationCount != level.warmupModificationCount ) {
> >+      level.warmupModificationCount = g_warmup.modificationCount;
> This already happens in G_UpdateCvars.
> 
> >+      level.warmupTime = -1;
> >+    }
> >+
> Will this even happen?

This resets the warmup countdown if g_warmup is changed via console.

> >+    // if the warmup time has counted down, restart
> >+    if( level.time > level.warmupTime ) {
> That should probably be ">=" (not that it makes a huge difference).

That's a millisecond of difference.

> >+        if( cl->pers.connected != CON_CONNECTED ||
> >+            cl->ps.stats[ STAT_PTEAM ] == PTE_NONE )
> >+          continue;
> >+
> That should probably be pers.teamSelection instead

Yes.

> >+      level.warmupTime += 10000;
> Huh?

The mechanism by which Quake 3 resets the map but does not start a new warmup still isn't entirely clear to me. This code comes from ioquake3 and I can't really answer for it other than to say that it really works.

> G_admin_allready stuff: blah. Just use readyToExit for both intermission and
> warmup and change
> if( !level.intermissiontime )
> to
> if( !level.intermissiontime || level.warmupTime != -1 )
> and make the message more generic. If the logic is right, CheckTournament
> should detect that warmup has ended the next frame.

This is a matter of personal preference. The deal here is that we meld together two relatively different (functionally) commands in to one because !allready is a familiar command. I prefer the more descriptive messages that are in the patch.

> >+  // if the match is starting, join the selected team
> >+  if ( !level.warmupTime && client->sess.warmupTeam != PTE_NONE ) {
> >+    G_ChangeTeam( ent, client->sess.warmupTeam );
> >+    client->sess.warmupTeam = PTE_NONE;
> >+  }
> >+
> This should probably be in ClientBegin, not ClientConnect.
> 
> >+  if( ready )
> >+    trap_SendServerCommand( -1, va( "print \"%s^7 is ready\n\"",
> >+                                    ent->client->pers.netname ) );
> >+  else
> >+    trap_SendServerCommand( -1, va( "print \"%s^7 is NOT ready\n\"",
> >+                                    ent->client->pers.netname ) );
> UnnamedPlayer^7 is ready
> UnnamedPlayer^7 is NOT ready
> UnnamedPlayer^7 is ready
> UnnamedPlayer^7 is NOT read
> x100

Believe it or not this is what other games do. If you are worried about flooding, Tremulous still doesn't use a generic flood guard. That is a separate issue.
 
> You ignore g_doWarmup and introduce g_doWarmupReady instead. That kind of makes
> g_doWarmup meaningless.

Right now g_doWarmup 1 enables warmup mode. g_doWarmupReady will additionally require players to Ready before starting the countdown if it is set to 1. I suppose we can contract those down to the original g_doWarmup and have it set to either 0, 1, or 2 (Ready mode).

Created attachment 1390 [details]
Tournament style warmup (4)

* Votes pause the warmup countdown
* g_doWarmupReady removed in favor of "g_doWarmup 2"
* Fixed two minor issues mentioned before

Created attachment 1395 [details]
Tournament style warmup (5)

* Warmup will not activate in cheats mode
* Warmup restart will preserve the selected layout

Created attachment 2173 [details]
no-join warmup times

I don't think I'm really fond of the tournament warmups idea and since this is still sitting here I don't think the others are either. However, reviving warmup time as a period of 15 seconds or so where you cannot join a team is pretty much the standard in 1.1, so it's about time that it be put to trunk. Thanks Undeference for the original game-side patch from so long ago.

Did the later at r1820.