The server-side demos thread, rebooted!

[GrosBedo]

@Gig: ah ok, thank's for the info (I also witnessed this "presence deletion" behaviour...).

That's unfortunate, most visitors will only read the first post since this thread is pretty long, and it would be unfortunate that they use an old and buggy version... But I can't see an alternative solution for this "presence deletion" problem.

@hairball: oh man! You've already synced lol. This is nice but please don't waste your time syncing every version I release because I will port my patch to the latest oa binary synced with ioquake3 when I'm finished, it's just a  matter of time

And about the files that shouldn't be modified, for example all the game/*.* and most of qcommon/*.* shouldn't be modified, because they overwrite standard q3 functions with deprecated/weird tremfusion functions.

[Neon_Knight]

@Gig: ah ok, thank's for the info (I also witnessed this "presence deletion" behaviour...).

That's unfortunate, most visitors will only read the first post since this thread is pretty long, and it would be unfortunate that they use an old and buggy version... But I can't see an alternative solution for this "presence deletion" problem.

Well, there's the option to move stuff to the Wiki.

[GrosBedo]

Indeed, good idea, but this is an original work, I mean it's not the standard OpenArena, so I'm not sure where it should reside in the wiki..

[GrosBedo]

New version:

- Faults tolerance mode: when sv_demoTolerant is 1, demo_play will try to be more tolerant to errors and unknown markers. This should allow future versions of this patch to add more meta data (infos about the demo) or more demo events, but still be able to replay old demos. This means that any demo recorded with a patch versions >= v0.9.4.3 should always, forever be replayable from now on with any future version of this patch.

- limits (capturelimit, timelimit, fraglimit) are now automatically set and restored (this is not necessary to replay a demo, but this enables the voice announcement of SuddenDeath).

I have now implemented all the features I wanted, and fixed pretty much everything I had on my plate. I'm going to test a little further, and then I'll finalize this patch by cleaning the code and porting to the last OA+ioquake3 engine.

[hairball]

I have now implemented all the features I wanted, and fixed pretty much everything I had on my plate. I'm going to test a little further, and then I'll finalize this patch by cleaning the code and porting to the last OA+ioquake3 engine.

Sounds good!   I deleted my copy of your server side demos to avoid needless duplication.

[GrosBedo]

Bad news: the patch crash for any other gametype that is not based on flags (CTF and Double Domination).

Here's a recap of my tests:

== Mod testing
- baseoa: works perfectly
- excessiveplus: works perfectly
- SuperHeroes Arena: works perfectly BUT suffers from the gametype bug (since there's no CTF gametype it seems?)
- Generations Arena: works ok but small problem in democlients teams management, it has to be refined

== Gametypes testing
- DM/TDM/Tourney: quit the demo when replayed after a few seconds. I don't know where this bug comes from...
- CTF: works great
- Double Domination: works great
- Overload: should work but for now it suffers from the warmup bug

So now my main concern is about the non flag based gametypes, it does not work quite good...

[GrosBedo]

Ok I've pinpointed a bit the problem, it is not related to the gametype at all, but to a problem in the democlients teams management. It's in fact a variant of an old problem I've dropped that made demos crash without any reason when bots were set to a team. The workaround was to avoid setting bots to teams, but now the problem also arise for players.

Anyway I'm pretty sure I'll be able to fix this bug now I've somewhat pinpointed it.

[GrosBedo]

Found the problem: it is not even related to the democlients team management, but to the warmup time:

in g_main.c CheckTournament():

Code:

// if the warmup time has counted down, restart
		if ( level.time > level.warmupTime ) {
			level.warmupTime += 10000;
			trap_Cvar_Set( "g_restarted", "1" );
			trap_SendConsoleCommand( EXEC_APPEND, "map_restart 0\n" );
			level.restarted = qtrue;
			return;
		}

This is what causes the demo to stop.

Now I just have to find a way to cleanly fix this problem.

Meanwhile, a simple fix is to set an absurdly high g_warmup prior to replaying a demo, but I hope to come up with a better fix.

/EDIT: just set /g_doWarmup 0 and it fixes the problem. The demo playback function now does that automatically, and I'll also implement a check to avoid g_restarted to be set to 1 when a demo is playing, this should not happen no matter the reason when a demo plays.

/EDIT2: cannot safeguard againt level.restarted = qtrue; so I'll just drop the safeguarding.

[GrosBedo]

New version available v0.9.7.4, downloadable along with win32 binaries on my github repo.

All bugs fixed. I think (hope!) this release is now pretty much stable. I didn't have the time to retest all my demos but it should hopefully work smoothly from now on.

[grey matter]

I see you did not find a workaround for the gentity_t->health issue. This means that your patch might not compatible with any other mod than the one you used for compiling.
It's unlikely that mods add fields before the health field, but if they do its offset changes and your server code will blindly overwrite memory at the wrong locations. You could make the offset configurable via a cvar, but I doubt that admins would really bother to find the correct one, especially with old closed source mods.

Since you seem to have issues with map_restart, what happens if commands like that get executed during demo playback, say due to players calling a vote? (your fix using g_doWarmup is mod specific as well. Mods might execute commands like this on other occasions).

tell, say_team, chat and tchat are also mod specific. free, red, blue, spectator , skill, timelimit, fraglimit and capturelimit also depend on the gamecode, though those are unlikely to change.
forceteam is a game command, which might not even be present in some mods.

While most of those are unlikely to be modified in mods, the size of this list suggests that your code is quite likely to break.
The patch for Tremulous modified gamecode, so it didn't need to care about mod interna. Older server-side demos had the same problems with mod support like your patch has.

I can't be of any help here, I just wanted to spell this out. I would not want to be the one maintaining such a monster, where each and everything can go wrong

[GrosBedo]

@grey matter: firstly, thank's for your input, they are very insightful.

About gentity_t->health, I DID find a workaround! And the workaround is based on the very definition of gentity_t, there's no direct access to the memory location, so even if mods change the order of the fields, the demo will still be recorded/replayed fine because it just does a cast of sharedEntity_t -> gentity_t.

About user input (such as callvote), they are not (yet) managed, but they can easily be filtered out by cancelling them when they reach the server. I will take a look at that feature. But anyway I'm not going to filter servers commands, the admin should be free to do whatever he wants to, even during a demo playback.

About chatting commands, I don't think so, every ioquake3 based game I saw used them (Smokin'Guns, Tremulous, ioquake3, OA).

About teams management, indeed that's ioquake3 specific, standalone games that modifies the team names will have to adapt a bit the code, but this way was the most effective I could find, because there's no function accessible server-side that can convert an int to a team (there is one in the gamecode, but inaccessible, I just copied it in my team management function, and added comments so that people that would port the patch would easily know what to do here).

About forceteam, it's implemented server-side in the binaries, so if my patch is ported, this command should be ported too, and it should work whatever the mod or standalone game.

That said, my primary goal is to make a patch that is compatible with OA, standard ioquake3 and any mod running on those engines, but not necessarily standalone games such as Tremulous. For these games, the patch should also work but with a few tweaks (such as the stage management for Tremulous, it was in the original Amanieu's patch but removed here because very specific to Tremulous).

For the moment, it seems it works with most mods (ExcessivePlus, Aftershock, Superheroes Arena, Generations Arena).

I will take again a look at the team management to see if I can do something more dynamical. But apart from that, I think that everything else is pretty much generic.

-----------

Anyway I'm going to try to make from scratch an alternative version of this functionnality, based on TheDoctor's patch, which should be a lot more generic and clean and short! But I'm not sure if my idea is gonna work, we'll see. At least this one patch already works.

[GrosBedo]

Ah also about team management, I forgot to mention that there's a weird beneficial side effect: in fact, whatever the team the client is set to, the gamecode will automatically affect the client to the right team.

So in fact I just can replace the command with "team o" (or in fact any string) and it should always work. So as it is, even if the team are changed, it WILL work.

/EDIT: about forceteam, you were right, it's dependent on the gamecode. But anyway, this should not hamper demo functionnalities, it is only used to force players into the spectator team prior to a demo playing, and I can't think of any other way to do it without using this command. Anyway, this was necessary before because there was a delay before the demo began, but now that's not the case anymore so it should not be necessary anymore.

/EDIT2: about the fact that we can do a "team o" with any team value and it works, it's because the gamecode will then automatically pick a team using the function PickTeam, so in fact it does NOT set to the right team, but a random team based on the availble slots. But the nice thing is that it does a CLIENTBEGIN, so that's why it shows the democlients correctly in their right team.

[GrosBedo]

Working on interoperability:

- changed forceteam into a clientcommand team, this should work on any ioquake3 based game.

- teams management isn't necessary for the patch to work, because we are recording clientcommands, so when a client change team it's completely mod-independent, the command is just replayed. Before it wasn't the case (in the original patch, these commands were not recorded), so now the team management is only useful for the initial team when the demo starts recording (because then there's no clientcommand, so the demo must still set the initial team). I'll try to move the team management from configstrings to userinfo since the userinfo string stores the full team name string, not just the int. I did that in the past but it didn't work, but maybe now it will.

But it works great even if the initial team is not set, it's just that spectators can then also be followed, which is not what we want.

Also it is to be noted that a simple CLIENT_BEGIN vmcall along with a correctly set client configstring allows the democlients to be shown in the right team, even if sessionTeam is not set. But since I want this patch to fully simulate the game state, the only way to set sessionTeam is to use the "team" clientcommand.

[GrosBedo]

BUG SPOTTED!

There is a bug in the recording of the userinfo string in the latest versions, I mistakenly changed strcpy to Q_strncpyz, which truncates the userinfo string.

I will soon post an updated version.

[grey matter]

There is a bug in the recording of the userinfo string in the latest versions, I mistakenly changed strcpy to Q_strncpyz, which truncates the userinfo string.

It should only do this when your target buffer is too small (hence the n in its name). Double check that your buffer is large enough, because plain strcpy might overflow it (and that's a security problem).

[GrosBedo]

That's how I did it with Q_strncpyz:

Code:

static char fuserinfodata[MAX_STRING_CHARS];
static char *fuserinfo = fuserinfodata;

Q_strncpyz(fuserinfo, userinfo, sizeof(fuserinfo));

Did I do something wrong? I'm not really a C guru

Anyway are there really any risk of overflowing if my var and the game var are both limited by the same constant (here MAX_STRING_CHAR)?

[grey matter]

You can not use sizeof(fuserinfo) here. The compiler only knows that it's a char *, it could point to any arbitrary sized struct.
You'll have to use Q_strncpyz(fuserinfodata, userinfo, sizeof(fuserinfodata)); (or directly use the MAX_STRING_CHARS size as an argument, which is not quite as good). Why the additional pointer anyways?

It does not matter whether the define *should be the same in both modules, you must always use sizeof(targetbuffer) if you want to be safe.

Thou shalt check the array bound off all strings (indeed, all arrays), for surely where thou typest "foo" someone someday shall type "supercalifragilisticexpialidocious".

[GrosBedo]

Thank's a lot, I didn't know sizeof should only point to the buffer, not the pointer to the buffer (yeah I'm a newb in C ).

I've fixed all the issues and used Q_strncpyz. I'll later convert the other ones.

Also, should I free() every malloc'ed char at the end of my functions?

[hairball]

Thank's a lot, I didn't know sizeof should only point to the buffer, not the pointer to the buffer (yeah I'm a newb in C ).

You should review the code again and read up on C.  I looked at the log for 5 seconds and saw this:

Code: ("wrong")

qboolean SV_CheckClientCommand( client_t *client, const char *cmd )
{
    char *userinfo = ""; // init with an empty string so that the compiler doesn't shout errors

    if ( !strncmp(cmd, "userinfo", 8) ) { // If that's a userinfo command, we directly handle that with a specialized function
       strncpy(userinfo, cmd+9, strlen(cmd)-9); // trimming out the "userinfo " substring (because we only need the userinfo string)

There are multiple problems with that code.

[GrosBedo]

@hairball: Ah indeed I didn't convert stncpy to Q_strncpyz, but what would be the problem here apart from that?

/EDIT: ah indeed the strlen(cmd)-9 was wrong because one shouldn't use the size of the input string. I've fixed that. Anyway I can assure you that there are similar mistakes in the ioquake3 engine since I've pretty much copied it.

[grey matter]

The problem is, that your char *userinfo is just a pointer, no buffer. Initializing it like this means the compiler will store the "" string somewhere (just large enough to store the \0, maybe even in the program's read-only section), then let userinfo point to it.
You now copy arbitrarily sized data to wherever userinfo points to, not into a properly allocated buffer. Like hairball suggested, get some book about C. Pointers can be tricky and dangerous.
You pointed one problem yourself, another problem is that you blindly use cmd+9, while the string that it points to might just be "userinfo", so you'd start to copy from some random memory that's after the string.

If there are indeed portions of ioquake3 code that do this, please file a bug report about them.

[GrosBedo]

Yes indeed I also fixed that while checking the code.

Could you please tell me if that one is ok (because it's a char[size]), or should I here too do a malloc?

Code:

static char savedFsGameVal[MAX_QPATH] = "";
static char *savedFsGame = savedFsGameVal;

Thank you for your advice but I really don't have the time to read an entire book on C, and this is the only project I have for which I must use the C language. Moreover, even if I admit that I'm not enough experienced with C as I would like, I'm a very experienced programmer in general, and I do know how the memory is managed and how pointers work generally, just not the syntax and habits specific to the C language (but I'm very willing to learn if you have a good resource about it!).

Anyway I will do more checking now as you are suggesting and follow these good habits. For example, about the userinfo, I thought about that when I did cmd+9, but I assumed that there was no possible reason that the engine would issue a userinfo without at least a string after because I can remember the engine checks that the userinfo is not empty before issuing it. Anyway I will add a strlen check, this should fix this for good.

About ioquake3, if I ever stumble accross these unfixed things in the latest version of the engine, I will report them (maybe they were fixed already).

[GrosBedo]

Code updated, fixed, and, most importantly, cleaned. I also added a readme with the changelog since about v0.5 and commands usage.

[Gig]

Hi! Where are the latest win32 binaries? I finally had some minutes to try the serverdemos thing, but I cannot find the binaries...  

[GrosBedo]

Hi! Where are the latest win32 binaries? I finally had some minutes to try the serverdemos thing, but I cannot find the binaries...  

Sorry I forgot to upload the binaries. They are now available, and this is the final version for OpenArena v0.8.8 (until I get feedback!).

You can get them here:
https://github.com/lrq3000/openarena_engine_serversidedemos/downloads