CVar.GetBool() & CVar.SetBool()

[Sarah]

We have GetInt and GetFloat, but no GetBool. Now I'm fully aware that a boolean is just an integer, which means that GetInt works fine.

Code: Select all

if (CVar.GetInt() == true)

That's a perfectly valid comparison, however it looks kind of strange when reading code. So can we get a Get/SetBool for readability?


The reason I ask is that I've set up a CVar bool to allow debug messages from ZScript-Windows (yeah ZSWin) to be toggled on an off via the options menu and there's already several of the above code snippet making my code look odd.

[Chris]

Now I'm fully aware that a boolean is just an integer, which means that GetInt works fine.

Conceptually, they're not. A boolean is an object that represents true and false, and an integer is an object that represents a whole number. You can represent a bool using an integer, but you can also represent an integer with a float... that doesn't mean they're the same. Often, programming languages convert 'true' to 1 and 'false' to 0 and interpret non-0 as true and 0 as false (consequently, 2 != true, but (bool)2 == true), but that's not universal. Command line shells for example may interpret a process exit code of 0 as 'true' (success), and non-0 as 'false' (failure).

[Sarah]

You're correct. I'm coming from a C frame of reference where bool is a typedef of an int and true and false are macros. I spent a lot of time using GDCC. Thus in my mind bool is still equivalent to an int; any non-zero value evaluates to true, there are always exceptions we could go on about for a while

Anyway, on topic, Get/SetBool() would complete the set of ZScript CVar functions in the name of readability.

[Nash]

Yes, thank you! It felt a bit weird using Get/SetInt for bool checks.

[Sarah]

Since this request hasn't gotten much attention, I thought I'd give adding these methods a shot. And it appears I've succeeded!

These need added to c_cvars.cpp:
GetBool()

Code: Select all

DEFINE_ACTION_FUNCTION(_CVar, GetBool)
{
	PARAM_SELF_STRUCT_PROLOGUE(FBaseCVar);
	auto v = self->GetGenericRep(CVAR_Bool);
	ACTION_RETURN_BOOL(v.Bool);
}

SetBool(bool)

Code: Select all

DEFINE_ACTION_FUNCTION(_CVar, SetBool)
{
	PARAM_SELF_STRUCT_PROLOGUE(FBaseCVar);
	if (!(self->GetFlags() & CVAR_MOD) && CurrentMenu == nullptr) return 0;
	PARAM_BOOL(val);
	UCVarValue v;
	v.Bool = val;
	self->SetGenericRep(v, CVAR_Bool);
	return 0;
}

Then they need added to the ZScript base.txt in the CVar struct:

Code: Select all

struct CVar native
{
	enum ECVarType
	{
		CVAR_Bool,
		CVAR_Int,
		CVAR_Float,
		CVAR_String,
		CVAR_Color,
	};

	native static CVar FindCVar(Name name);
	native static CVar GetCVar(Name name, PlayerInfo player = null);
	native bool GetBool();  // Addition
	native int GetInt();
	native double GetFloat();
	native String GetString();
	native void SetBool(bool b);  // Addition
	native void SetInt(int v);
	native void SetFloat(double v);
	native void SetString(String s);
	native int GetRealType();
	native int ResetToDefault();
}

*Disclaimer: I have tested GetBool and it looks like it works. I have not tested SetBool. Also I am terrible with Git so I'm gonna avoid making a pull request unless absolutely necessary; I tend to break things when trying to use Git.

One other thing I'm unsure of is in vm.h. The final line of GetBool calls ACTION_RETURN_BOOL (macro right?), which is just a macro call to ACTION_RETURN_INT. I'm not sure if this is now wrong or not, so I need some input there.

[Rachael]

In that case you might want to contact someone who is comfortable making pull requests and give them a copy of your modified files (and you should do that ASAP, because the longer you wait the more chance such files have to be modified and you don't want your modifications to undo them).

Or pull them into a zip/7z archive and attach them to the forum.

[Sarah]

Uploading the files seems like the fastest option

[Rachael]

I'm going to go ahead and leave it in queue for now. It would be nice to have some testing material for this - that will make the review process much faster.

But at the very least, now that it's in Git, you don't have to worry about it becoming outdated. (At least, not nearly as much...)

https://github.com/coelckers/gzdoom/pull/378/files

[_mental_]

Taking into account that VM has no separate bool type and uses int I think that the following will be enough:

Code: Select all

struct CVar native
{
	// ...
	bool GetBool() { return GetInt(); }
	void SetBool(bool b) { SetInt(b); }
	// ...
}

IMHO the idea is to avoid addition of code to the engine if it's possible to do the same via scripting without significant overhead.

[Sarah]

Thanks Rachael! I will get something put together for testing material, hopefully tonight.

@_mental_ : Either solution works for me but my opinion may change after I create some usage tests. I already have a sneaking suspicion of a possible issue that will require code addition to the engine meaning wrapping Get/SetInt isn't enough.

[Major Cooke]

It's downgraded into a boolean both ways, going mental's path. If it doesn't work, then it's a bug that needs to be reported for fixing.

[_mental_]

I already have a sneaking suspicion of a possible issue that will require code addition to the engine meaning wrapping Get/SetInt isn't enough.

Example please.

[Graf Zahl]

That might happen if some CVAR type returns different things for CVAR_Bool and CVAR_Int. No such CVAR type exists nor is there much of a chance that it ever will exist.

[Sarah]

Sorry for the week-long break, I work a fair bit of overtime every week.

Anyway, I was concerned that, because I don't know precisely what the macros are doing, that unintentional misuse of Get/SetBool would result in some odd results. I wanted to guarantee that the boolean was indeed being set to either 1 or 0 and no other values accepted. I have yet to test with only modifications to base.txt, just wrapping Get/SetInt in Get/SetBool, but my tests with the modified source does indeed show that the methods work as intended. I will update again when I've done the second round of tests, and my test pk3 is attached. Assuming both solutions are equivalent then it's not worth arguing and I leave it up to you devs to decide the best course of action. Thanks guys and gals


Edit:
So I added the methods as wrappers to base.txt in a copy of the engine that doesn't have my code additions. Now when loading the test file the engine errors out with "The function 'Cvar.Get/SetBool' has not been exported from the executable." So it does seem that the methods need added to the engine.

[_mental_]

Please read my code carefully. Or just copy/paste it.
NB: Added member functions must be defined without native keyword.

---

Code: Select all

Current value of CVar is 0
The CVar is false!
CVar was set to arbitrary positive value, the CVar value is: 1
CVar was set to an arbitary negative value, the CVar value is: 1
CVar was set to true, the value is: 1
CVar was set to false, the value is: 0