SpiritQuaddicted / reQuiem

reQuiem is a custom OpenGL Quake engine for Windows and Linux. It's designed for maximum compatibility with all things Quake - past, present and future. It's fast, reliable, and easy to configure. In short: it fixes what was broken, improves what needed improving, and leaves the rest alone. It was developed by jdhack.
GNU General Public License v2.0
17 stars 2 forks source link

Loading of main map of zendar fails #16

Closed SpiritQuaddicted closed 10 years ago

SpiritQuaddicted commented 10 years ago

https://www.quaddicted.com/reviews/zendar1d.html

You launch into a skill selection area. The map then assigns a rune to the player. Walking through a teleporter does a changelevel to the same map but spawns at info_player_start2

In reQuiem this fails with:

Host_Error: CL_ParseServerMessage: Unknown server command 0 Last cmd was svc_spawnstatic (20)

SpiritQuaddicted commented 10 years ago

Same happens with masque and masque_final on the change between masquestart.bsp and masque.bsp

Loading masque.bsp on its own works fine.

neogeographica commented 10 years ago

Testing with zendar, we get into this callstack for the changelevel command:

    reQuiemd.exe!SV_SpawnServer(const char * server=0x0018f838, const char * startspot=0x00000000)  Line 2513 + 0x9 bytes   C
    reQuiemd.exe!Host_Changelevel_f(cmd_source_t src=SRC_COMMAND)  Line 486 + 0x10 bytes    C
    reQuiemd.exe!Cmd_ExecuteString(const char * text=0x0018f93c, cmd_source_t src=SRC_COMMAND)  Line 1031 + 0xc bytes   C
    reQuiemd.exe!Cbuf_Execute()  Line 264 + 0x13 bytes  C
    reQuiemd.exe!_Host_Frame(double time=3.8312179917454614e-007)  Line 793 C
    reQuiemd.exe!Host_Frame(double time=3.8312179917454614e-007)  Line 921 + 0xe bytes  C

SV_SpawnServer calls Host_ClearMemory, which among other things clears out the cl.protocol value. It also clears sv.protocol, but that soon gets set back to PROTOCOL_VERSION_BJP3.

A little further down, SV_SpawnServer calls PR_LoadEdicts. This generates quite a few messages to the client, starting with an svc_spawnstatic message generated by MSG_WriteSpawnstatic. The messages aren't yet processed by the client code though; just gathered up to prepare for sending.

MSG_WriteSpawnstatic calls MSG_WriteModelIndex. MSG_WriteModelIndex writes a short (2 bytes) rather than a byte for the first part of the svc_spawnstatic message because the server protocol is PROTOCOL_VERSION_BJP3.

Near the end of SV_SpawnServer, it calls SV_SendServerInfo. Among other things this generates an svc_serverinfo message that tells the client which protocol version to use.

OK, now the client code starts processing those messages, starting with the svc_spawnstatic message. Because it gets the svc_spawnstatic message (and others) before the svc_serverinfo message, the cl.protocol value is bogus, and it decodes the svc_spawnstatic message incorrectly.

It seems pretty clear that if we're going to nuke the cl.protocol value, we'd better send svc_serverinfo right away instead of faffing about with other messages. I'm not yet familiar enough with the protocol to know the best way to fix this (or why the problem only shows up in this particular case).

neogeographica commented 10 years ago

At first glance, Fitz code seems like it would have a similar code flow for this case. But it doesn't have the same svc_spawnstatic format difference that protocol 10002 does, so maybe it is working by accident.

Edit: ah I see the difference. PF_makestatic in the Fitz code only writes the message to sv.signon. In reQuiem it is written both to sv.signon and to the local client. Dunno why!

Just for fun I commented out that additional code from PF_makestatic and things seem to be working fine. I'd really like to know what it's there for though.

neogeographica commented 10 years ago

Got a theory; elaborating at http://forums.inside3d.com/viewtopic.php?f=3&t=5497

neogeographica commented 10 years ago

Plausible fix in the commit above.

It would be nicer if I could check some state that corresponds to "the client has been sent an svc_serverinfo command since the last time its protocol was reset" rather than checking the allow_postcache global. Just to have a more semantically precise expression of when it is kosher to do this.

Since this code is explicitly for a local-client situation, it might not be too gross to check that cl.protocol isn't zero... but the cl global struct doesn't even exist if this is a dedicated server build. I'm going to ponder this a little bit before actually merging this solution into the master branch.

(Hmm, I could just ifdef this out of the dedicated server build entirely...)