[quake3-bugzilla] [Bug 4962] Protocol version 69 + legacy support

[bugzilla-daemon at icculus.org]

https://bugzilla.icculus.org/show_bug.cgi?id=4962

--- Comment #15 from Thilo Schulz <arny at ats.s.bawue.de> 2011-06-27 15:23:26 EDT ---
(In reply to comment #13)
> OK, I've had a pretty good look at it now. I have a few points and questions:

Thanks

> * There are changes in cvar.c and Com_sprintf that look unrelated to this
> patch; if that's the case they should probably just be committed anyway,
> separately.

Yes. The cvar thing was part of a hack to write a debug_protocol cvar so that
the serverbrowser would accept old versions. It's not necessary anymore though.
The snprintf change should be committed, i see no reason why it should return
void when we have the information returned by snprintf()

> * On the same theme, the fix for the q3noclient issue should probably be in a
> subsequent patch, to avoid clouding what is changing here.

Can be done.

> * There is a line of commented code sending a disconnect command using
> NET_OutOfBandPrint. If it's not necessary it should be removed completely. If
> it needs to remain commented for some reason, this reason should be explained
> in comments.

Then I will remove it. The idea is that there's no possibility a spoofed
disconnect message can disconnect arbitrary clients, because that occurance is
not connection-based.

> * The cvar names protocol and protocolver are pretty horrible and misleading. I
> see why you've named them like this, but I think a better solution would be a
> well placed hack to translate requests for the value of "protocol" coming from
> legacy qvms to "oldprotocol".

Yes, it is necessary so the server browser won't break.
How do you detect legacy QVMs though? I proposed something some time ago which
was rejected.
Besides, I don't like this kind of a hack. We'd have to add an extra if
condition to Cvar_VariableStringBuffer(), or at least in cg_ui.c where that
function is called.
I'd rather stay with the current solution of just adding a new cvar name. If
you know a better name, go ahead.

> * Actually, "oldprotocol" would be better as "legacyprotocol". This goes for
> all the preprocessor tokens etc. too.

No problem. I don't care about naming.

> * Now a slightly bigger suggestion. If we're going to have all this upheaval
> then we might as well do it right. To that end, I suggest we ditch identifying
> protocol by a simple number and switch to using strings. There are probably
> half a dozen things out there using protocol numbers in the range ~69-75, none
> of which are remotely compatible. If we use strings instead we avoid all this
> mess and also make it more difficult to (attempt) to connect to something where
> it has no business working. If strings are a problem (for whatever reason) we
> could alternatively use FOURCC codes like video codecs. This is assuming we
> have 32bits to play with.

Do you mean for each project to then call its protocol like "ioq3_1" for our
ioquake3, maybe "wq3_1" for western quake3 or "wop_1" for world of padman? Sure
we can do it, no question about it. The protocol version is already transmitted
in the infoString as a string to the server. But do we really need this? Most
standalone games seem to be separated via master server anyways, and the cvars
give full control over which protocol version number to regard as legacy, and
which as new protocol

-- 
Configure bugmail: https://bugzilla.icculus.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.