MantisBT - Zandronum
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0002633Zandronum[All Projects] Suggestionpublic2016-02-13 14:252018-09-30 21:40
ReporterDusk 
Assigned ToDusk 
PrioritynormalSeverityminorReproducibilityhave not tried
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version 
Target Version2.2Fixed in Version2.2 
Summary0002633: Remove sv_limitcommands from release builds
DescriptionI think sv_limitcommands needs the same treatment as sv_useticbuffer. People are disabling it in public servers (e.g. Best Ever) when they really shouldn't be doing that.
Steps To Reproduce
Additional Information
TagsNo tags attached.
Relationships
Attached Filesdiff sv_limitcommands.diff (2,608) 2016-02-13 15:59
/tracker/file_download.php?file_id=1738&type=bug
diff sv_limitcommands-2.diff (3,663) 2016-02-13 21:13
/tracker/file_download.php?file_id=1740&type=bug
Issue History
Date ModifiedUsernameFieldChange
2016-02-13 14:25DuskNew Issue
2016-02-13 14:33DuskStatusnew => needs review
2016-02-13 14:33DuskFile Added: sv_limitcommands.diff
2016-02-13 15:58DuskFile Deleted: sv_limitcommands.diff
2016-02-13 15:59DuskFile Added: sv_limitcommands.diff
2016-02-13 15:59DuskNote Added: 0014429
2016-02-13 16:34Torr SamahoNote Added: 0014430
2016-02-13 16:34Torr SamahoStatusneeds review => feedback
2016-02-13 19:24DuskNote Added: 0014431
2016-02-13 19:24DuskStatusfeedback => new
2016-02-13 21:13DuskFile Added: sv_limitcommands-2.diff
2016-02-13 21:13DuskNote Added: 0014432
2016-02-13 21:13DuskAssigned To => Dusk
2016-02-13 21:13DuskStatusnew => needs review
2016-02-14 15:45Torr SamahoNote Added: 0014438
2016-02-14 15:45Torr SamahoStatusneeds review => feedback
2016-02-23 22:10DuskNote Added: 0014484
2016-02-23 22:10DuskStatusfeedback => assigned
2016-02-23 22:24DuskNote Added: 0014485
2016-02-23 22:24DuskStatusassigned => needs review
2016-02-23 22:24DuskView Statusprivate => public
2016-02-28 20:24cobaltStatusneeds review => needs testing
2016-02-28 20:24cobaltNote Added: 0014515
2016-03-01 22:05DuskNote Added: 0014528
2016-03-01 22:05DuskStatusneeds testing => resolved
2016-03-01 22:05DuskFixed in Version => 2.2
2016-03-01 22:05DuskResolutionopen => fixed
2018-09-30 21:40Blzut3Statusresolved => closed

Notes
(0014429)
Dusk   
2016-02-13 15:59   
For this to work, version.h needs to be included from sv_main.h. Is this a problem?
(0014430)
Torr Samaho   
2016-02-13 16:34   
Did you introduce the define HAVE_SV_LIMITCOMMANDS so that people could compile a release version with sv_limitcommands (which would be incompatible with the normal release version)?

I'm not sure if it's worth making sv_main.h depend on version.h. This would mean pretty much everything depends on version.h. It's probably better to move the definition of sv_limitcommands to some other header.
(0014431)
Dusk   
2016-02-13 19:24   
I only had it so as not to copy-paste the BUILD_ID check around, but it could be removed just as well.

Now that I think of it, we could probably use a CVAR_ flag to make it constant and invisible in the console, such as CVAR_CONSTANT. This way we don't have to alter any headers or code at all, only the CVAR declaration. We could then also have CVAR_DEBUGONLY which evaluates to CVAR_CONSTANT in release builds and 0 otherwise.
(0014432)
Dusk   
2016-02-13 21:13   
New diff attached.
(0014438)
Torr Samaho   
2016-02-14 15:45   
I don't think that CVAR_CONSTANT itself is a useful flag. Off the top of my head, I can't think of any example where you would want a CVAR that is always invisible and unchangeable.

CVARs that are only available in debug mode make a lot of sense though, so IMHO CVAR_DEBUGONLY would be a useful flag. In your implementation, it's no flag though, although it name implies that it is a flag.

In addition to adding CVAR_NOSET in release mode, how about also removing CVAR_ARCHIVE? Or does your current patch already make sure that the cvar is removed from the ini?

BTW: Is it safe to call EnableNoSet in the FBaseCVar constructor? Since this is a static function, doesn't it have a global effect?
(0014484)
Dusk   
2016-02-23 22:10   
I did not realize that m_DoNoSet was a static member. Aagh, what's with that prefix?

As for the cvar naming, I simply didn't realize that I could simply define CVAR_DEBUGONLY as a regular CVAR flag and test whether it should have any effect in FBaseCVar's constructor. I was too convinced that it had to be a macro.
(0014485)
Dusk   
2016-02-23 22:24   
Also realized that there isn't much reason to keep this private either.

'https://bitbucket.org/Torr_Samaho/zandronum-stable/pull-requests/28 [^]'
(0014515)
cobalt   
2016-02-28 20:24   
Issue addressed by commit 5fc7cd2b35ea: Restrict sv_limitcommands to debug builds, addresses 2633
Committed by Teemu Piippo [Dusk] on Wednesday 31 December 1969 23:59:57

Changes in files:

 docs/zandronum-history.txt | 2 +-
 src/c_cvars.cpp | 10 ++++++++++
 src/c_cvars.h | 3 +++
 src/sv_main.cpp | 8 ++------
 4 files changed, 16 insertions(+), 7 deletions(-)
(0014528)
Dusk   
2016-03-01 22:05   
I ran extensive tests of this before the patch was pulled. In a release-identifying build it was impossible to change sv_limitcommands or sv_useticbuffer from the console or from the ini.