Update registercvar definitions in dpdefs to allow additional parameters

[MarioSMB]

The registercvar builtin optionally supports a 3rd parameter:
flags = prog->argc >= 3 ? (int)PRVM_G_FLOAT(OFS_PARM2) : 0;

This change updates the dpdefs for CSQC and SVQC (menu already included the additional parameter) to match, making registered cvar flags accessible to the other VMs.

[drjaska]

/*
=========
VM_registercvar

float	registercvar (string name, string value[, float flags])
=========
*/

@MarioSMB Why did you choose

(string name, string value, ...)

over this?

(string name, string value, float flags)

[MarioSMB]

Compatibility; the 3rd parameter is optional in non-menu VMs, so adding float flags instead of ... would cause a compilation failure in codebases updating their dpdefs.

[drjaska]

Would it make sense to add a comment note above both , ... such as

//float registercvar (string name, string value[, float flags]) //  optional flags arg for compatibility
float(string name, string value, ...) registercvar = #93;

//builtin definitions:
//float registercvar (string name, string value[, float flags]) //  optional flags arg for compatibility
float(string name, string value, ...) registercvar = #93;
//description:
//adds a new console cvar to the server console (in singleplayer this is the player's console), the cvar exists until the mod is unloaded or the game quits.

[MarioSMB]

[, float flags] is the predefined way to note an optional parameter.

[drjaska]

... leaves it completely open as for what is allowed there, include a commit that only a single float is optionally allowed.

[MarioSMB]

The documentation already states that the single float is supported, why do you want me to add a note that no other builtin has?

[drjaska]

The documentation is at the function body and not with the function prototype which is my issue. Everyone who sees that prototype will need to then go figure out what other arguments it supports because the prototype documentation is incomplete and requires the function body to demystify.

But if this is not the custom here then don't.

@hemebond bump, merge?

[drjaska]

And that there is not a comment which says why the function prototypes differ between QCVMs.

[MarioSMB]

I didn't update the menu definition because it already had the 3rd parameter, making it optional now would be a bit confusing.

The documentation and general state of builtin definitions is in a terrible state of disarray, it really deserves a separate major cleanup...

[drjaska]

The menu definition doesn't need a change and I didn't ask for one, as per my code snippets here.

[Cloudwalk9]

When in doubt, do what FTEQW does if it doesn't break ABI.

[Cloudwalk9]

I'm not sure about this. DP never allowed using the third flag in CSQC or SSQC and it's a compatibility quirk in FTEQW that relies on precise positions of values in an internal bitfield. I don't think it's a good idea to allow mods to start doing it now.

[drjaska]

When in doubt, do what FTEQC does if it doesn't break ABI.

I don't think that there is anything to compare against FTEQC unless you were going to compare which project has better documentation (properly documented overloaded arguments for function prototypes vs. undocumented function prototypes with ... in parameters for unknown reasons) or less breaking changes (having it be optional via ... vs. changing the function prototype to have a 3rd non-optional argument).

[drjaska]

DP never allowed using the third flag in CSQC or SSQC

Any idea as for why?

[Cloudwalk9]

Sorry I meant FTEQW, not "FTEQC". Also, I'm inclined to reject this PR to be on the safe side in terms of future backwards compatibility concerns and technical debt. While the engine internally allows this, dpdefs itself has not officially allowed it for the client and server and it's pretty jank to begin with. I don't think it's a good idea to officially encourage mods to use this functionality as a part of the API and ABI itself and extend the jank further. I won't close it yet but please do not merge.

@divVerent what are your thoughts?

[divVerent]

Note that there is also precedent for adding another builtin with the same number in such cases, which here could be called registercvar3. Cloudwalk9 ***@***.***> schrieb am Sa., 18. Jan. 2025, 11:20:

…

Sorry I meant FTEQW, not "FTEQC". Also, I'm inclined to reject this PR to be on the safe side in terms of future backwards compatibility concerns and technical debt. While the engine internally allows this, dpdefs itself has not officially allowed it for the client and server and it's pretty jank to begin with. I don't think it's a good idea to officially encourage mods to use this functionality as a part of the API and ABI itself and extend the jank further. I won't close it yet but please do not merge. @divVerent <https://github.com/divVerent> what are your thoughts? — Reply to this email directly, view it on GitHub <#235 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB5NMHPLEQJ5UGZ3GP6YMT2LJ5OHAVCNFSM6AAAAABUPJRYMCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKOJZG43TIOBUGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Cloudwalk9]

To elaborate a bit further, the position of the bits feels like an implementation detail of the engine and shouldn't be relied on. So my inclination to reject is to not replicate MQC's mistake just because it can do it.

[hemebond]

Does DarkPlaces just do nothing the flags parameter?

[MarioSMB]

The takeaway from some discussions is that this isn't a bad change, but it enables the use of internal bitflags that do not match between engines and the developers would prefer to see extensions to cvar registering before allowing more widespread use of those bitflags.

I am a little uneasy about this being closed on the grounds of "playing it safe" as the current method is rather limiting, but at the same time this specific change would open the door to a compatibility nightmare.
So with that said, I am going to close this with the hopes that cvar registration sees some kind of gradual improvement (i.e. a proper extension) moving forward.