svc_spawnstatic and reading jdhack's mind

Discuss programming topics for the various GPL'd game engine sources.
Post Reply
Johnny Law
Posts: 22
Joined: Mon Apr 28, 2014 8:34 pm
Location: San Carlos, CA
Contact:

svc_spawnstatic and reading jdhack's mind

Post by Johnny Law »

One of the highest-priority remaining (known) bugs with reQuiem is that it fails with a bad cmd when changing levels. At least in some situations... I need to try some other changelevels but I've just been testing the zendar case.

(This is https://github.com/SpiritQuaddicted/reQuiem/issues/16 but I'll recap below.)

The short form of the situation is that it seems to be resolved by just removing a code snippet that jdhack had added. However I'd like to know why he might have added it. If you're bored and want to speculate, please do!

The function in question is PF_makestatic. In reQuiem it looks like this:

Code: Select all

void PF_makestatic (void)
{
        edict_t *ent = G_EDICT(OFS_PARM0);

        MSG_WriteSpawnstatic (&sv.signon, ent);

// JDH: if there's a local client running, send it a message
        if (svs.clients[0].spawned && svs.clients[0].netconnection && !svs.clients[0].netconnection->socket)
                MSG_WriteSpawnstatic (&svs.clients[0].message, ent);

// throw the entity away now
        ED_Free (ent);
}
The MSG_WriteSpawnstatic call that modifies sv.signon is original Quake code more or less, just modified to support an alternate protocol and factored out.

The code that writes to svs.clients[0] is jdhack's addition. Any guesses about what it is intended to do?

==========

Here's why it causes problems. The SV_SpawnServer flow during the triggered changelevel (when running in singleplayer) is:
- Unset the stored client and server protocol versions.
- Server protocol gets set again (to 10002 by default).
- Worldmodel is loaded.
- PR_LoadEdicts is called. This eventually ends up doing a lot of PF_makestatic calls.
- PF_makestatic shoves svc_spawnstatic messages both into sv.signon and also (because of jdhack's addition) into svs.clients[0].message.
- Because of the server protocol, all those svc_spawnstatic cmds use a short for modelindex instead of a byte.
- After all that, SV_SpawnServer calls SV_SendServerInfo. Among other things this generates an svc_serverinfo cmd (in svs.clients[0].message) that will tell the client which protocol version to use.

So at this point, in svs.clients[0].message we have svc_spawnstatic cmds followed later by svc_serverinfo (and a bit of other stuff).

At some point Host_PreSpawn_f is run, which copies the signon cmds to the end of svs.clients[0].message. So now it contains svc_spawnstatic cmds, then svc_serverinfo, then the same svc_spawnstatic cmds again.

When the client code tries to process this, it sees the initial set of svc_spawnstatic cmds before it sees the svc_serverinfo cmd. So it has a wrong (unset) protocol version when parsing the svc_spawnstatic cmds, it uses a byte rather than a short for modelindex, and things go downhill from there.

If I remove jdhack's change, then the message will contain svc_serverinfo before the svc_spawnstatics (from sv.signon) and things seem to work fine. But deleting someone else's code makes me nervous if I can't even guess what it is trying to do. :-)

==========

Edit: thinking back, I'm not sure that I saw Host_PreSpawn_f copying to the same initial message, as opposed to some subsequent message. Shouldn't make a difference to the situation though.
Johnny Law
Posts: 22
Joined: Mon Apr 28, 2014 8:34 pm
Location: San Carlos, CA
Contact:

Re: svc_spawnstatic and reading jdhack's mind

Post by Johnny Law »

Maybe answering my own question (which often happens once I've taken the trouble to write it up). Looking through the changelog I see this:
- create command now works with uncached models in Quoth, and for entities that are made static;
also lightmaps for spawned bmodels are set up properly
There's a new "create" command and Host_Create_f function that only works in singleplayer/listenserver, which does call PR_ExecuteProgram to spawn stuff. Proooobably this is related to the change to PF_makestatic. (Although in this case it's kind of the opposite problem? I can see wanting to send svc_spawnstatic cmds to the local client for this, but why add them to sv.signon? Maybe collecting them in case another client connects?)

I should grep around for other instances of PR_ExecuteProgram.
Spike
Posts: 2914
Joined: Fri Nov 05, 2004 3:12 am
Location: UK
Contact:

Re: svc_spawnstatic and reading jdhack's mind

Post by Spike »

presumably svs.clients[0].spawned should be false when changing levels.

sv.signon is the signon message buffer. its a reliable buffer which is sent as part of the intial connection (when you first sign on). it also contains other precaches and things (the precache_model should be first, obviously). adding to it mid-map will NOT be visible to clients currently on the server.
thus checking the spawned setting *should* allow it to only fire when the client is already on the server and will thus be unable to see the signon buffer.

there might be a race condition between setting spawned and signons. in single player you're not likely to notice it, but its possible. iirc during this window the client won't receive either.
in multiplayer, the extra code is too hacky to be useful, and may be counter productive for modders who think that it'll work in multiplayer as it does in single player (including coop). by my definition this is a bug, or at best a hack, undesirable either way.

presumably, the logic used should be the same as the lightstyle logic iirc.

(all this stuff got rewritten in QW... and again in FTE... so I might be wrong with a few things)

imho the problem comes from requiem's protocol promotion feature. if there's more than 255 models precached, just switch protocols right from the start instead of randomly mid-message, that should help solve precache index sizes.
Johnny Law
Posts: 22
Joined: Mon Apr 28, 2014 8:34 pm
Location: San Carlos, CA
Contact:

Re: svc_spawnstatic and reading jdhack's mind

Post by Johnny Law »

Ah. I'll check the spawned flag's behavior when I get back to a debugger tonight, but it looks like it's not set false until SV_SendServerinfo, which is after PR_LoadEdicts. (And then set true later, at the end of the signon process, after everything in my initial post.)

I think I've got my head around the desired behavior at this point. The "create" console command is supposed to be used only from a local client, and only when the current number of clients is 1. (And this is enforced in Host_Create_f.) Pushing svc_spawnstatic cmds into svs.clients[0].message should take care of updating the local client with the created entity. Pushing the same svc_spawnstatic cmds also to sv.signon doesn't hurt, and if a new client happens to connect it should get the created entity from there.

The kicker is that for the normal svc_spawnstatics during changelevel, we only want to put the cmds into sv.signon, and the client's spawned flag isn't the protection that is needed to avoid inappropriately dumping things into svs.clients[0].message at that time.

(I'm getting pretty curious to see what happens on changelevels other than the zendar test case.)

If I really want to make sure that jdhack's snippet is only used during the PR_ExecuteProgram for this new "create" command, there's a global (allow_postcache) I could use to guard for that. Might be all that is required. That global is currently being used to allow PF_precache_sound and PF_precache_model during that PR_ExecuteProgram.

I hate using globals this way... I wonder if there is some other client state that would express this situation more accurately than the spawned flag does. (Edit: or as an alternative maybe set the spawned flag false at an earlier point, in some circumstances? That seems iffier though since other code also looks at it.)

And this is sounding a lot like the previous issue. :-) Shared code with some behavior that should only be performed when one of its way-up-the-stack callers is invoked from a particular context.
Last edited by Johnny Law on Wed Jul 16, 2014 9:30 pm, edited 1 time in total.
Spirit
Posts: 1065
Joined: Sat Nov 20, 2004 9:00 pm
Contact:

Re: svc_spawnstatic and reading jdhack's mind

Post by Spirit »

The create function is silly fun by the way.

requiem also does something to decide which protocol to use, maybe that is somehow involved too? I have no idea how it does it so I might be just adding noise here.

edit: Oops, read over Spike's last line saying that.
Improve Quaddicted, send me a pull request: https://github.com/SpiritQuaddicted/Quaddicted-reviews
Johnny Law
Posts: 22
Joined: Mon Apr 28, 2014 8:34 pm
Location: San Carlos, CA
Contact:

Re: svc_spawnstatic and reading jdhack's mind

Post by Johnny Law »

The protocol promotion feature is gnarly enough that it could indeed have some lurking bugs, but it doesn't seem to be involved in this particular problem.
Post Reply