Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update ConVar flags in console.inc. #344

Merged
merged 4 commits into from Jun 7, 2015
Merged

Conversation

psychonic
Copy link
Member

The existing list was from the original engine, not even being accurate for EP1.
Flags that were never used and have since been removed from headers were removed.
Flags that were removed, but were used at one time co-exist with new flags that took over their value where applicable.
Flags that only exist on some engine versions are noted as such in the comment.

The existing list was from the original engine, not even being accurate for EP1.
Flags that were never used and have since been removed from headers were removed.
Flags that were removed, but were used at one time co-exist with new flags that took over their value where applicable.
Flags that only exist on some engine versions are noted as such in the comment.
@asherkin
Copy link
Member

asherkin commented Jun 4, 2015

We can't remove these defines, there are hundreds of released plugins using FCVAR_PLUGIN, and at least a handful using FCVAR_LAUNCHER (to mean FCVAR_DEVELOPMENTONLY).

FCVAR_PLUGIN should at minimum be defined to 0 to be a no-op, and FCVAR_LAUNCHER should probably stay.
I'm assuming we can't deprecate defines, but could move these all to const ints to make that possible.

@psychonic
Copy link
Member Author

Fair enough.

In both cases, plugins would still have continued to work as-is. Plugins with FCVAR_PLUGIN are already arguably broken as they're manually adding FCVAR_SS_ADDED that exists on some engines to convars. I'm unsure of what exact effect this has, but it's not ideal. They should remove the flag. And the other is just a matter of editing the define to the one that makes more sense. It didn't seem too unreasonable if it's only after a major release.

That said, I'll add the compat shims and deprecation pragma where applicable.

@asherkin
Copy link
Member

asherkin commented Jun 4, 2015

Yeah, I can get behind removing FCVAR_LAUNCHER in master, but not FCVAR_PLUGIN - it's just in too many plugins.

On a semi-related note, I wonder if it is worth having CreateConVar strip FCVAR_REPLICATED, that's another commonly copy-pasted flag that has negative effects.

@psychonic
Copy link
Member Author

I'm assuming we can't deprecate defines, but could move these all to const ints to make that possible.

Are you sure that would make it possible? I tried converting them to const ints, with the "#pragma deprecated message" above them and usage does not seem to trigger a warning.

* Re-added FCVAR_PLUGIN (shimmed to 0) and FCVAR_LAUNCHER with deprecation notes.
* Changed defines to const ints.
* Updated comments to use our newer, single-line comment style for cleanliness.
psychonic added a commit that referenced this pull request Jun 7, 2015
@psychonic psychonic merged commit 13ecd11 into master Jun 7, 2015
@psychonic psychonic deleted the update-convar-flags branch June 7, 2015 15:18
@KyleSanderson
Copy link
Member

I've been on quite the roll lately, however shouldn't these still be variables? Even if deprecated, we can nuke them (0) internally and keep the values in-sync with the SDKs that line up with the games. The approach could be similar to entity masks. Keywords are already reserved, and we can use FCVAR_PLUGIN for instance with our own usage if need be.

@psychonic
Copy link
Member Author

I think that would probably work fine, although there isn't a pressing need to do so as historically none have ever been reordered. We also probably wouldn't need to use the deprecate feature on them again, and it would be rare for an existing one to be removed from all engines.

Amaroq7 referenced this pull request in Amaroq7/SMVIP Aug 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants