This patch allows a player to follow his/her teammates when dead. It works the same as following players when on the spectator team except only teammates can be followed.
Currently, the following does not happen automatically, players must hit mouse3 (or whatever their bind is) to start/stop following. Personally I like it better when it happens automatically (like some other games), but this was not well received in testing.
Most of the changes in this patch is just making sure that things play nice with the borrowed playerState_t you get with spectator follow. However, there is one very imporant code change that is probably not obvious:
client->pers.credit and client->pers.score are the values that gets read
in ClientSpawn(). If you need to manipulate a player's PERS_SCORE or
PERS_CREDIT in the persistant[] array, make sure to keep the values in
sync.
This patch has had a fair amount of testing on trem.tjw.org, but I its bound to contain some more bugs because of the complicated nature of playerState_t sharing involved in SPECTATOR_FOLLOW. I hope that it can get some wider testing and peer review before commit.
Here are some helpful tips for dealing with SPECTATOR_FOLLOW:
1) if client->sess.sessionTeam is TEAM_SPECTATOR, it means the player is in
spectator mode, but does not tell you what team the player is on.
2) the player's real team is determined by client->pers.teamSelection
3) NEVER trust or write to anything in client->ps when
client->sess.sessionTeam is TEAM_SPECTATOR because it may belong to
another player.
Part of attachment 1313[details] looks like a fix for bug 2970 and a lot of it looks like general code consistency stuff (bug 3089?). I don't always like to get more than what's advertised ;-)
(If also looks like your patch may fix the issue mentioned in bug 2970 comment 1.)
(In reply to comment #2)
> Part of attachment 1313[details] looks like a fix for bug 2970 and a lot of it looks
> like general code consistency stuff (bug 3089?). I don't always like to get
> more than what's advertised ;-)
>
> (If also looks like your patch may fix the issue mentioned in bug 2970 comment
> 1.)
>
Sorry for neglecting bug 2970. I've committed your patches at svn 935 for and merged this patch to it.
As for bug 3089, I prefer doing code cleanups only as actual changes touch the code. This is simply to make tracking changes more clear. I'm not trying to make policy, it's just my preference I guess.
(In reply to comment #4)
> As for bug 3089, I prefer doing code cleanups only as actual changes touch the
> code. This is simply to make tracking changes more clear. I'm not trying to
> make policy, it's just my preference I guess.
I'd generally agree in terms of commits; otherwise 3/4 of the changes might be to correct typos and indentation. The purpose of bug 3089 is to keep track of changes that will probably otherwise never touch the code.
I'll try testing attachment 1314[details] later today and see if I can come up with anything more constructive than nits about what goes in the patch.
Comment on attachment 1314[details]
svn935 spec teammates
If sessionTeam is being used for different stuff in Tremulous than in Quake (which seems to just be for g_maxGameClients, which conversely broke in Tremulous -- see bug 2969), some of the old code that uses sessionTeam should probably be cleaned up (like in g_session.c). Or maybe a different variable should be used instead.
At the moment, it's rather confusing.
+ else if( cl->sess.spectatorState == SPECTATOR_FOLLOW )
+ ping = cl->pers.ping < 999 ? cl->pers.ping : 999;
Why?
- if( ent->client->ps.stats[ STAT_HEALTH ] <= 0 )
+ if( ent->health <= 0 && !ent->client->sess.sessionTeam != TEAM_SPECTATOR )
+ {
+ trap_SendServerCommand( ent-g_entities,
+ va( "print \"You cannot use the class cmd when dead unless spec\n\"" ) );
return;
+ }
I assume that extra unary '!' a typo. (You should fix that.)
It seems sort of weird to have to stop following a teammate when you want to spawn. Perhaps this check should be moved so that I can still use my "class level0" bind without having to stop following a teammate first (e.g., I see a teammate needs help and try to rush to save him/her :). Maybe further down near:
//if we are not currently spectating, we are attempting evolution
if( ent->client->pers.classSelection != PCL_NONE )
?
+ else if( Q_stricmp( cmd, "class" ) == 0 )
...
+ // ignore all other commands when client is a spectator to prevent issues
+ // with the borrowed playerState_t when in SPECTATOR_FOLLOW
It looks like we are in agreement then.
(Though, in my opinion, ClientCommand should be cleaned up a bit. And this certainly isn't helping it.)
+ // stop any following clients
+ G_StopFromFollowing( ent );
If attachment 1251[details] (or something similar) for bug 2958 is committed, that will be done in G_LeaveTeam and will be unnecessary in G_ChangeTeam. I should correct that patch when this is committed, I just need to remember. (Note to self...)
(In reply to comment #0)
> Currently, the following does not happen automatically, players must hit mouse3
> (or whatever their bind is) to start/stop following. Personally I like it
> better when it happens automatically (like some other games), but this was not
> well received in testing.
I do too. Maybe this could be a client side option like the proposed fix for bug 2792 (perhaps using the same cvar even)?
(In reply to comment #6)
> (From update of attachment 1314[details])
> If sessionTeam is being used for different stuff in Tremulous than in Quake
> (which seems to just be for g_maxGameClients, which conversely broke in
> Tremulous -- see bug 2969), some of the old code that uses sessionTeam should
> probably be cleaned up (like in g_session.c). Or maybe a different variable
> should be used instead.
> At the moment, it's rather confusing.
Agreed, but it took me so long to finally get used to how sessionTeam is used currently in Trem I don't think I want to go back. I'm happy with just using
pers.teamSelection for determining the real team and using sess.sessionTeam
to determine spectator or not.
> - if( ent->client->ps.stats[ STAT_HEALTH ] <= 0 )
> + if( ent->health <= 0 && !ent->client->sess.sessionTeam != TEAM_SPECTATOR )
> + {
> + trap_SendServerCommand( ent-g_entities,
> + va( "print \"You cannot use the class cmd when dead unless spec\n\"" )
> );
> return;
> + }
> I assume that extra unary '!' a typo. (You should fix that.)
> It seems sort of weird to have to stop following a teammate when you want to
> spawn. Perhaps this check should be moved so that I can still use my "class
> level0" bind without having to stop following a teammate first (e.g., I see a
> teammate needs help and try to rush to save him/her :). Maybe further down
> near:
> //if we are not currently spectating, we are attempting evolution
> if( ent->client->pers.classSelection != PCL_NONE )
> ?
>
> + else if( Q_stricmp( cmd, "class" ) == 0 )
> ...
> + // ignore all other commands when client is a spectator to prevent issues
> + // with the borrowed playerState_t when in SPECTATOR_FOLLOW
> It looks like we are in agreement then.
I fixed the typo, but if i understand what you're saying, that's not going to fix your annoyance. I never thought about binds, the reason I allowed "class" while spec is because I incorrectly assumed it was being issued by the menu systems while the player was still spec. I'll do some testing with what you're suggesting and submit an updated patch later.
> (Though, in my opinion, ClientCommand should be cleaned up a bit. And this
> certainly isn't helping it.)
Meh, It's annoyingly long for sure, but it's completely understandable imo.
>
> + // stop any following clients
> + G_StopFromFollowing( ent );
> If attachment 1251[details] (or something similar) for bug 2958 is committed, that will
> be done in G_LeaveTeam and will be unnecessary in G_ChangeTeam. I should
> correct that patch when this is committed, I just need to remember. (Note to
> self...)
Sorry for neglecting that one too (bug 2958). I merged your patch for it at 939 and updated this patch to work with it.
> (In reply to comment #0)
> > Currently, the following does not happen automatically, players must hit mouse3
> > (or whatever their bind is) to start/stop following. Personally I like it
> > better when it happens automatically (like some other games), but this was not
> > well received in testing.
> I do too. Maybe this could be a client side option like the proposed fix for
> bug 2792 (perhaps using the same cvar even)?
>
Maybe, this can wait for later though.
I'll see if I can find anything else to complain about with attachment 1322[details] sometime in the next few days.
But before then, I noticed a potential bug:
G_AddCreditToClient (and perhaps other functions) carelessly modify a client's ps without checking if the client is a spectator. Your patch adds
>+ // if not a following spec, keep pers.credit in sync for next ClientSpawn()
>+ if( client->sess.sessionTeam != TEAM_SPECTATOR )
>+ client->pers.credit = client->ps.persistant[ PERS_CREDIT ];
I think the assumption there is that client->ps.persistant[ PERS_CREDIT ] was modified, so client->pers.credit needs updating. That assumption is fine if only living players, who are presumably not spectators, get credits.
Now, unless I'm mistaken (please tell me I am), here's how I could exploit that:
1. /kill
2. wait 19 seconds
3. drop grenade in alien base
4. middle click and scroll to a teammate with a lot of credits
*boom*
5. stop spectating
My teammate's ps.persistant[ PERS_CREDIT ] gets modified, but not his or her pers.credit (so the difference is discarded?). My pers.credit /does/ get modified and, because my ps values probably aren't really mine, I get my teammate's money and whatever else I earned.
I haven't actually tested that and I may be wrong about when things happen, but it should illustrate that a lot more may need to be changed before this is entirely safe.
:-(
Sorry for all the spam :-(
(In reply to comment #9)
> I fixed the typo, but if i understand what you're saying, that's not going to
> fix your annoyance. I never thought about binds, the reason I allowed "class"
> while spec is because I incorrectly assumed it was being issued by the menu
> systems while the player was still spec. I'll do some testing with what you're
> suggesting and submit an updated patch later.
>
If pers.classSelection has the correct class and Cmd_Class_f uses that, all that should be necessary is to stop spectating before being added to the spawn queue.
After that, I think this is fine. I have some ideas for potential improvement, but none should block this.
(In reply to comment #10)
> But before then, I noticed a potential bug:
> G_AddCreditToClient (and perhaps other functions) carelessly modify a client's
> ps without checking if the client is a spectator. Your patch adds
>
> >+ // if not a following spec, keep pers.credit in sync for next ClientSpawn()
> >+ if( client->sess.sessionTeam != TEAM_SPECTATOR )
> >+ client->pers.credit = client->ps.persistant[ PERS_CREDIT ];
>
> I think the assumption there is that client->ps.persistant[ PERS_CREDIT ] was
> modified, so client->pers.credit needs updating. That assumption is fine if
> only living players, who are presumably not spectators, get credits.
It looks like I added this check out of my pure paranoia about client->ps sharing. G_AddCreditToClient() should never be called for a spectating client, so this check is not necessary (and confusing). If anything, there could be a check on sessionTeam at the start of G_AddCreditToClient() so the function can return early in case it is ever called for a spectating client.
Does this seem correct?
Created attachment 1333[details]
svn939 spec teammates
* reorganized the Cmd_Class_f() function so the /class command for dead players is seperated from the /class command from live aliens
* never allow the same clientNum into the spawn queue more than once (not sure if the patch for bug 2881 fixed this completely)
* really allow the /class command for spawning when following a teammate.
* dropped unnecessary sessionTeam check in G_AddCreditToClient()
I noticed earlier that level.sortedClients (set in CalculateRanks) uses ps.persistant[ PERS_SCORE ] instead of pers.score. I think this was the cause of an amusing instance earlier today where the player with most kills was listed last on the scores at intermission.
(In reply to comment #13)
> It looks like I added this check out of my pure paranoia about client->ps
> sharing. G_AddCreditToClient() should never be called for a spectating client,
> so this check is not necessary (and confusing). If anything, there could be a
> check on sessionTeam at the start of G_AddCreditToClient() so the function can
> return early in case it is ever called for a spectating client.
>
> Does this seem correct?
>
There is the case of a player who gets kills (e.g., from a grenade) when dead. In this case, I think it is appropriate for the player to get whatever money was earned. The potential issue I noticed is valid, just not as problematic as I initially thought.
(From update of attachment 1333[details])
I have a couple nits, but other than that it looks good.
>+ client->pers.classSelection = PCL_NONE;
>+ client->ps.stats[ STAT_PCLASS ] = PCL_NONE;
> }
>+
>+ if( attack1 && client->pers.classSelection == PCL_NONE )
>+ {
>+ if( client->pers.teamSelection == PTE_NONE )
>+ G_TriggerMenu( client->ps.clientNum, MN_TEAM );
>+ else if( client->pers.teamSelection == PTE_ALIENS )
>+ G_TriggerMenu( client->ps.clientNum, MN_A_CLASS );
>+ else if( client->pers.teamSelection == PTE_HUMANS )
>+ G_TriggerMenu( client->ps.clientNum, MN_H_SPAWN );
>+ }
Does this mean that if you are in the spawn queue and you click you will be immediately prompted to enter the spawn queue? It looks to me like there should be an "else" there.
>+ //set the item to spawn with
Since you just moved it from elsewhere, I won't say it.
There are a bunch of useless uses of "va" in Cmd_Class_f.
>+ if( ent->health <= 0 )
>+ {
>+ trap_SendServerCommand( ent-g_entities,
>+ va( "print \"You cannot use the class cmd when dead unless spec\n\"" ) );
>+ return;
>+ }
The message should probably contain fewer abbreviations (maybe something like "You cannot use this command unless you are a spectator") or not be added. The no-spawn period after dying is mandatory, so I'm in favor of just returning.
There is the potential for confusion since a player in sports, for example, is not normally a spectator.
Part of my Cmd_Follow_f patch should probably also be reverted (I think I never had a valid reason for this change to begin with):
// can't follow another spectator
+ if( level.clients[ i ].pers.teamSelection == PTE_NONE )
- if( level.clients[ i ].sess.sessionTeam == TEAM_SPECTATOR )
return;
There's one little problem with your class stuff:
> // spawn from an egg
> ent->client->pers.classSelection = newClass;
> ent->client->ps.stats[ STAT_PCLASS ] = newClass;
> G_PushSpawnQueue( &level.alienSpawnQueue, clientNum );
With a bind like "class builderupg; class builder", this will set the player's class to builderupg and add them to the spawn queue. It will then change the class to builder (before the player spawns) and not add the player to the spawn queue.
It might even be possible to spawn with a bind like "class builder; class level4". I haven't tried it.
The issue mentioned in the previous comment can be fixed by checking if the player is in the spawn queue before changing their class. It's kind of silly to do it this way, but G_PushSpawnQueue could be changed to a qboolean return type and the class stuff can be handled if it returns true. The more obvious solution makes the check in G_PushSpawnQueue redundant (since "class" is the only command that will let you spawn).
The purpose of this comment was not about that though, but about something else I found that is extremely weird. If you are following someone wearing a battlesuit and you press a bind to spawn, you might have the bsuit model. I'm not sure why, but calling G_StopFollowing before adding them to the spawn queue fixes it.
Created attachment 1342[details]
svn935 spec teammates
> I noticed earlier that level.sortedClients (set in CalculateRanks) uses
> ps.persistant[ PERS_SCORE ] instead of pers.score.
It seems the issue is in SortRanks(), I changed that to use pers.score and removed the unused PERS_RANK stuff in CalculateRanks(). Renamed PERS_RANK to PERS_UNUSED for now.
> There is the case of a player who gets kills (e.g., from a grenade) when dead.
I understand now. I changed G_AddCreditToClient() so all the calculation is done on pers.score, then it's copied to PERS_SCORE if not following (instead of the other way around).
> Does this mean that if you are in the spawn queue and you click you will be
> immediately prompted to enter the spawn queue? It looks to me like there
> should be an "else" there.
Yes, it's more complicated than adding just an "else" though. My thoughts were that since leaving the spawn queue has a static position anyway, the player might as well be looking at the menu. Perhaps I'm missing something, but I'm leaving it as-is for now.
> There are a bunch of useless uses of "va" in Cmd_Class_f.
removed
> >+ if( ent->health <= 0 )
> >+ {
> >+ trap_SendServerCommand( ent-g_entities,
> >+ va( "print \"You cannot use the class cmd when dead unless spec\n\"" ) );
> >+ return;
> >+ }
> The message should probably contain fewer abbreviations (maybe something like
> "You cannot use this command unless you are a spectator") or not be added. The
> no-spawn period after dying is mandatory, so I'm in favor of just returning.
This now returns silently.
> Part of my Cmd_Follow_f patch should probably also be reverted
> + if( level.clients[ i ].pers.teamSelection == PTE_NONE )
> - if( level.clients[ i ].sess.sessionTeam == TEAM_SPECTATOR )
reverted.
> G_PushSpawnQueue could be changed to a qboolean return type
> and the class stuff can be handled if it returns true.
I took this route, it seems like the most understandable way.
> The purpose of this comment was not about that though, but about something
> else I found that is extremely weird. If you are following someone wearing a
> battlesuit and you press a bind to spawn, you might have the bsuit model. I'm
> not sure why, but calling G_StopFollowing before adding them to the spawn
> queue fixes it.
Ick. I added the G_StopFollowing() in Cmd_Class_f() if the sessionTeam is spec, but I'm not so confident this is the proper fix. The root of the problem is that ps.stats[] must not be a borrowed one with ClientSpawn() or more appropriately ClientUserinfoChanged() is called. This means the client must stop spectating and also probably run a client frame before they have their real playerState_t back. So I think there may still potential for this messing up.
Sorry, I have not tested this patch at all yet.
The bsuit thing does still happen. I'm not entirely sure what's causing it: if it's the one thing, then it's probably my fault for not using the new bsuit stuff. If it's not that, then it's conceivably possible for this to happen any time someone spawns (but that doesn't happen, so I'm going with the former).
There are a few other things that use wrong variables. I should probably make a new bug for those. This will change the solution marginally (s/savedScore/score/).
It would be nice if the mess of which variable can be trusted when could be eliminated: I noticed that the server code expects ps and persistant (why isn't that "persistent", by the way?) to be correct in a few places.
The ClientCommand stuff does cause a little inconsistency. If you use a non-existent (or existent, but invalid in the current context) command as a spectator, you will get no error message. Currently this inconsistency is only present during intermission.
I have an idea for making ClientCommand a bit better that should prevent this and help reduce some repeated code. I'll make a new bug for that later.
I had some more ideas / complaints specifically relevant to this feature, but I can't remember them. (I kept putting off mentioning them until I had enough so I wouldn't be spamming this. So I forgot them.)
(Another unrelated thought: would it make sense to move some of the client think stuff to the server frame code? It looked to me like some of it doesn't have to happen every client frame and might benefit from happening even if a few packets are dropped or late.)
I think I found what was causing the bsuit thing (I just kept overlooking it) and prevented it from happening again (I think). If I'm right, just ignore that because it's my fault (and G_StopFollowing() shouldn't be needed in Cmd_Class_f).
I made bug 3114 for my complaints about ClientCommand(), so I can't complain about that here anymore.
If you are a spectator, you can see the classes/items of the other team. Aside from that, this is good.
This patch allows a player to follow his/her teammates when dead. It works the same as following players when on the spectator team except only teammates can be followed. Currently, the following does not happen automatically, players must hit mouse3 (or whatever their bind is) to start/stop following. Personally I like it better when it happens automatically (like some other games), but this was not well received in testing. Most of the changes in this patch is just making sure that things play nice with the borrowed playerState_t you get with spectator follow. However, there is one very imporant code change that is probably not obvious: client->pers.credit and client->pers.score are the values that gets read in ClientSpawn(). If you need to manipulate a player's PERS_SCORE or PERS_CREDIT in the persistant[] array, make sure to keep the values in sync. This patch has had a fair amount of testing on trem.tjw.org, but I its bound to contain some more bugs because of the complicated nature of playerState_t sharing involved in SPECTATOR_FOLLOW. I hope that it can get some wider testing and peer review before commit. Here are some helpful tips for dealing with SPECTATOR_FOLLOW: 1) if client->sess.sessionTeam is TEAM_SPECTATOR, it means the player is in spectator mode, but does not tell you what team the player is on. 2) the player's real team is determined by client->pers.teamSelection 3) NEVER trust or write to anything in client->ps when client->sess.sessionTeam is TEAM_SPECTATOR because it may belong to another player.Created attachment 1313 [details] svn933 spec teammatesCreated attachment 1314 [details] svn935 spec teammates updated to work with svn 935Created attachment 1322 [details] svn939 spec teammates * fix typo (extra '!') * merged with svn 939 (untested)There's one little problem with your class stuff: > // spawn from an egg > ent->client->pers.classSelection = newClass; > ent->client->ps.stats[ STAT_PCLASS ] = newClass; > G_PushSpawnQueue( &level.alienSpawnQueue, clientNum ); With a bind like "class builderupg; class builder", this will set the player's class to builderupg and add them to the spawn queue. It will then change the class to builder (before the player spawns) and not add the player to the spawn queue. It might even be possible to spawn with a bind like "class builder; class level4". I haven't tried it.