Page MenuHomeWildfire Games

Refactor HWDetect and rendering options setup
ClosedPublic

Authored by wraitii on May 28 2019, 11:50 AM.

Details

Reviewers
linkmauve
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP23747: Refactor HWDetect and rendering options setup.
Trac Tickets
#5538
Summary

This removes a lot of duplication in how graphics settings are set, by simply reading config variables in RenderingOptions, and letting the side-effects do their thing.

Precursor to D2293 (could have gone either way, but now it seems more natural this way).

I'm keeping User > HWDetect because I had a bad experience with Age 3 doing the opposite recently, it's rather annoying.

Test Plan

Look at the code and agree this is nicer, compile, test in-game.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

wraitii created this revision.May 28 2019, 11:50 AM
Owners added a subscriber: Restricted Owners Package.May 28 2019, 11:50 AM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/1527/display/redirect

fabio added a subscriber: fabio.Sep 2 2019, 12:56 PM

Reference to S3TC may as well mostly removed altogether, just quit with an error if not detected. See https://trac.wildfiregames.com/ticket/4803

wraitii updated this revision to Diff 9607.Sep 2 2019, 9:32 PM

Merge shadows change from old D1930 and remove s3TC stuff. It seems like the S3TC code remaining in lib/tex.cpp could also be removed now, but I need to check and also this sounds more involved.

Vulcan added a comment.Sep 2 2019, 9:38 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/56/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/VideoMode.h
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/ps/VideoMode.h
|  23| class·CVideoMode
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCVideoMode{' is invalid C code. Use --std or --language to configure the language.

source/ps/VideoMode.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"
Executing section JS...
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/mod/hwdetect/hwdetect.js
|    |++++| /zpool0/trunk/binaries/data/mods/mod/hwdetect/hwdetect.js
| 335| 335| 
| 336| 336| global.RunHardwareDetection = function(settings)
| 337| 337| {
| 338|    |-	//print(JSON.stringify(settings, null, 1)+"\n");
|    | 338|+	// print(JSON.stringify(settings, null, 1)+"\n");
| 339| 339| 
| 340| 340| 	var output = RunDetection(settings);
| 341| 341| 
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/mod/hwdetect/hwdetect.js
|    |++++| /zpool0/trunk/binaries/data/mods/mod/hwdetect/hwdetect.js
| 339| 339| 
| 340| 340| 	var output = RunDetection(settings);
| 341| 341| 
| 342|    |-	//print(JSON.stringify(output, null, 1)+"\n");
|    | 342|+	// print(JSON.stringify(output, null, 1)+"\n");
| 343| 343| 
| 344| 344| 	for (var i = 0; i < output.warnings.length; ++i)
| 345| 345| 		warn(output.warnings[i]);

binaries/data/mods/mod/hwdetect/hwdetect.js
| 179| »   var·disable_audio·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'disable_audio' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 180| »   var·disable_s3tc·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'disable_s3tc' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 181| »   var·disable_shadows·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'disable_shadows' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 182| »   var·disable_shadowpcf·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'disable_shadowpcf' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 183| »   var·disable_allwater·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'disable_allwater' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 184| »   var·disable_fancywater·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'disable_fancywater' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 185| »   var·enable_glsl·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'enable_glsl' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 186| »   var·enable_postproc·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'enable_postproc' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 187| »   var·enable_smoothlos·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'enable_smoothlos' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 188| »   var·override_renderpath·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'override_renderpath' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 179| »   var·disable_audio·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'disable_audio' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 180| »   var·disable_s3tc·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'disable_s3tc' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 181| »   var·disable_shadows·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'disable_shadows' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 182| »   var·disable_shadowpcf·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'disable_shadowpcf' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 183| »   var·disable_allwater·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'disable_allwater' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 184| »   var·disable_fancywater·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'disable_fancywater' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 185| »   var·enable_glsl·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'enable_glsl' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 186| »   var·enable_postproc·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'enable_postproc' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 187| »   var·enable_smoothlos·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'enable_smoothlos' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 188| »   var·override_renderpath·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'override_renderpath' to 'undefined'.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/565/display/redirect

Stan added a subscriber: Stan.Sep 2 2019, 10:42 PM
Stan added inline comments.
binaries/data/mods/mod/hwdetect/hwdetect.js
357 ↗(On Diff #9607)

Should be removed if you nuke the option, shouldn't it.

source/ps/GameSetup/Config.h
40 ↗(On Diff #9607)

Oversight ? Else the option must go too.

elexis added a subscriber: elexis.Sep 4 2019, 3:28 AM

There is #5538, I was wondering whether thats the same, related, or unrelated, but I didn't get to investigate.

In D1931#93796, @elexis wrote:

There is #5538, I was wondering whether thats the same, related, or unrelated, but I didn't get to investigate.

Combined with D1930, this actually overwrites user.cfg settings too, just not command-line. As it stands, it overwrites defaults, but not user, so it is indeed #5538. The question is 'should it be easy for a user to set a setting that we detected it can't use?'

I'm guessing we could enforce in the UI that you can't set these settings, so then we would rarely have new users with incompatible user.cfg, but the option would be there. Alternatively, command line options.

wraitii updated this revision to Diff 9660.Sep 7 2019, 5:49 PM

Add a new HWDETECT configuration level, above user but below Command, and use that for hwdetect.

wraitii edited the summary of this revision. (Show Details)
Vulcan added a comment.Sep 7 2019, 5:52 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/92/display/redirect

Vulcan added a comment.Sep 7 2019, 6:04 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/VideoMode.h
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/ps/VideoMode.h
|  23| class·CVideoMode
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCVideoMode{' is invalid C code. Use --std or --language to configure the language.

source/ps/ConfigDB.h
|  54| class·CConfigDB·:·public·Singleton<CConfigDB>
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCConfigDB:' is invalid C code. Use --std or --language to configure the language.

source/ps/VideoMode.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"
Executing section JS...
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/mod/hwdetect/hwdetect.js
|    |++++| /zpool0/trunk/binaries/data/mods/mod/hwdetect/hwdetect.js
| 335| 335| 
| 336| 336| global.RunHardwareDetection = function(settings)
| 337| 337| {
| 338|    |-	//print(JSON.stringify(settings, null, 1)+"\n");
|    | 338|+	// print(JSON.stringify(settings, null, 1)+"\n");
| 339| 339| 
| 340| 340| 	var output = RunDetection(settings);
| 341| 341| 
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/mod/hwdetect/hwdetect.js
|    |++++| /zpool0/trunk/binaries/data/mods/mod/hwdetect/hwdetect.js
| 339| 339| 
| 340| 340| 	var output = RunDetection(settings);
| 341| 341| 
| 342|    |-	//print(JSON.stringify(output, null, 1)+"\n");
|    | 342|+	// print(JSON.stringify(output, null, 1)+"\n");
| 343| 343| 
| 344| 344| 	for (var i = 0; i < output.warnings.length; ++i)
| 345| 345| 		warn(output.warnings[i]);

binaries/data/mods/mod/hwdetect/hwdetect.js
| 179| »   var·disable_audio·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'disable_audio' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 180| »   var·disable_s3tc·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'disable_s3tc' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 181| »   var·disable_shadows·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'disable_shadows' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 182| »   var·disable_shadowpcf·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'disable_shadowpcf' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 183| »   var·disable_allwater·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'disable_allwater' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 184| »   var·disable_fancywater·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'disable_fancywater' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 185| »   var·enable_glsl·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'enable_glsl' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 186| »   var·enable_postproc·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'enable_postproc' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 187| »   var·enable_smoothlos·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'enable_smoothlos' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 188| »   var·override_renderpath·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'override_renderpath' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 179| »   var·disable_audio·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'disable_audio' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 180| »   var·disable_s3tc·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'disable_s3tc' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 181| »   var·disable_shadows·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'disable_shadows' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 182| »   var·disable_shadowpcf·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'disable_shadowpcf' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 183| »   var·disable_allwater·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'disable_allwater' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 184| »   var·disable_fancywater·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'disable_fancywater' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 185| »   var·enable_glsl·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'enable_glsl' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 186| »   var·enable_postproc·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'enable_postproc' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 187| »   var·enable_smoothlos·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'enable_smoothlos' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 188| »   var·override_renderpath·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'override_renderpath' to 'undefined'.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/601/display/redirect

wraitii updated this revision to Diff 12090.Jun 1 2020, 5:13 PM

Rebased, and merge the remaining changes from D1930 here.

I think I got confused at some point what this was doing maybe? Anyways, this follows #5538 specs in letting the user overwrite the HWDetect scripts, while still letting HWdetec script overwrite default config settings.

wraitii retitled this revision from Refactor HWDetect and rendering options fully to Refactor HWDetect and rendering options setup.Jun 1 2020, 5:15 PM
wraitii edited the summary of this revision. (Show Details)
wraitii updated the Trac tickets for this revision.
Vulcan added a comment.Jun 1 2020, 5:17 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1755/display/redirect

Vulcan added a comment.Jun 1 2020, 5:18 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2286/display/redirect

linkmauve accepted this revision.Jun 1 2020, 6:14 PM
linkmauve added a subscriber: linkmauve.

I haven’t tested this patch, but it’s overall a much needed clean-up, so +1 for it!

I remember wondering why configuration was intricated like that, when I was trying to bring up GLES.

This revision is now accepted and ready to land.Jun 1 2020, 6:14 PM
bb added a subscriber: bb.Jun 1 2020, 6:47 PM

For the interested reader: D86

elexis added a comment.Jun 1 2020, 7:44 PM

I haven’t tested this patch, but it’s overall a much needed clean-up, so +1 for it!
I remember wondering why configuration was intricated like that, when I was trying to bring up GLES.

Removing hardcoding of Config variables sounds good depending on implementation.
What does acceptance mean? Does it express a desire that the patch should land? Does it include taking responsibility for the code quality?
Perhaps the static code review is about certain parts of the patch but not all of the semantic changes?
I'm just wondering because if accepting patches is the same as liking the idea of a patch, then how is a formal acceptance of a patch different from a Like button? (I didn't understand that since the inception of the formal review system)

source/ps/GameSetup/Config.cpp
192 ↗(On Diff #12090)

and here

source/ps/ModIo.cpp
89 ↗(On Diff #12090)

Was this change accepted?

source/ps/scripting/JSInterface_ConfigDB.cpp
49 ↗(On Diff #12090)

and here

wraitii added inline comments.Jun 1 2020, 7:51 PM
source/ps/GameSetup/Config.cpp
192 ↗(On Diff #12090)

For the record you validated the concept On Sep 2 2019, assuming that local.cfg is deemed "purposeless".

I do think it's purposeless, though I do believe you're using it extensively.
I dislike breaking user-flow for breaking's sake, but it does not really seem worth keeping an optional config file that one person uses.

'Voice ye' concern here, heterodox config user, or forever remain silent' I guess

elexis added inline comments.Jun 1 2020, 9:22 PM
source/ps/GameSetup/Config.cpp
192 ↗(On Diff #12090)

(http://irclogs.wildfiregames.com/2019-09/2019-09-02-QuakeNet-%230ad-dev.log)

I didn't say "purposeless", but I tried to discover the reasons (use cases) why it was added, and the unreported use cases which might have ocurred and then the implications on the code if one was to consider use cases.
Yeah deleting it is an option. But I can't say with certainty without having worked with the code more closely myself, which is why I asked about that.
It seems there are use cases for multiple user configs (multiple users, multiple passwords, multiple hotkeys for instance), and there may be use cases conceived for system-wide config changes. I don't know about that. I'm not raising a concern unless there is something broken which is often discovered aposteriori when users run the administered code.

I guess most features are features that one user uses, you mean I'm the only one? I don't know, perhaps, I didn't survey. If there is a valid use case for the file then perhaps more people use it. Perhaps (actually certainly) it's advertized to the general user to use local.cfg and that's one part that should be changed if the file is removed.

(Hint ^)

Now where I look at my local.cfg and user.cfg, I remember one of the technical differences which are an unconsidered but perhaps considerable use case: code comments. local.cfg allows them, but user.cfg doesnt really support them because each time the game saves a config it overwrites the file without comments. I mentioned that one too on IRC, and I guess that 'feature' may still be dropped, if one wanted to, but one would have to drop it consciously and then under balance of pros and cons.

Another difference is that user.cfg is written in alphabetic order, local.cfg is written in the order specified by the text editor.

So it means the use case of saving per-user config values is covered by both local.cfg and user.cfg, therefore that use case does not grant reason for local.cfg's existence. And the other difference between user.cfg and local.cfg is allowing the user to store user config options with a text editor with code comments and arbitrary sorting order, which may be dropped if the author, reviewer and the users agree.

Deleting local.cfg will make it impossible to keep config options commented and semantically grouped.

Then again the part of grouping config options semantically is probably better fixed by providing categories to the options, for example renaming shadowquality to graphics.shadowquality would mean that its grouped with the other graphic options if sorted alphabetically.

So it's probably ok to delete it and probably ok to keep it and also ok to create a multi-user config system and also probably ok to support comments etc. My actual questions are actually not answered, then again asking questions is not wanted by the community because it's time consuming (unfortunately it's necessary if one doesn't want to break stuff unintentionally).

wraitii added inline comments.Jun 2 2020, 10:20 AM
source/ps/GameSetup/Config.cpp
192 ↗(On Diff #12090)

Deleting local.cfg will make it impossible to keep config options commented and semantically grouped.

Indeed, but as I said on IRC back then I doubt the usefulness of this "feature".
If you're using a development copy, it's rather easy to simply have your own version of default.cfg (using git it's as simple as a commit on top of HEAD that you regularly rebase).

I'm curious what "documentation" brings you here too, since the options are already documented.

Keeping different user-cfg for different profiles, first is a feature we don't have (yet), and second can be done by just having several files and renaming appropriately.


With that being said, I don't care much about CFG_SYSTEM, and I'm okay with keeping a CFG_LOCAL if you would prefer it so. My default stance is to delete what I see as a relic of the past. But I haven't done a survey either (and nor will I because honestly if we start doing surveys of people for these kind of changes we're never committing anything again), so I will rely on rather unique events of people saying "this breaks my flow, can you keep it in?"

So yeah basically I'm asking you (and anybody else that cares to read this) if you want me to keep CFG_LOCAL (renamed from CFG_SYSTEM) or if I should proceed with deletion.

source/ps/ModIo.cpp
89 ↗(On Diff #12090)

This isn't a change, really, it's a necessity if CFG_SYSTEM is removed.

The fact is we can already override those on the command line, so a dev working on this and wanting to test something should just use that, there's no point in Local.cfg, and for other people DEFAULT is the only thing that makes sense if we are to follow the security-concern comment above.

elexis added inline comments.Jun 2 2020, 11:23 PM
source/ps/GameSetup/Config.cpp
192 ↗(On Diff #12090)

I wonder only about which parts of the code were accepted.
I can adapt to a removed local.cfg.
It requires a survey to make a statement about the number of users of a given feature, but it doesn't require a survey to decide which features are superfluous.
The hint was about the wiki pages that recommend the player to use local.cfg.

source/ps/ModIo.cpp
89 ↗(On Diff #12090)

It is necessary to change the line if CFG_SYSTEM is removed.
It is not inherently necessary to change it to this value, it can become more or less strict.

If I was to work with this line of code, I would wonder if the code comment is actually true.

We do this so a malicious mod cannot change the base url

If a GUI context runs Engine.ConfigDB_CreateValue("default", "modio.v1.baseurl", "broken"); (try for yourself with F9 in the mod page prior to opening the mod.io dialog) it can enable the MITM attack.
Seems strange for the original author to have written such a code comment about safety precautions without actually verifying it, but perhaps it was a test for the reviewers.

I think I'll proceed with this over the WE perhaps, need to remind myself to update the wiki pages and do the few useful changes pointed out.

source/ps/GameSetup/Config.cpp
192 ↗(On Diff #12090)

It requires a survey to make a statement about the number of users of a given feature, but it doesn't require a survey to decide which features are superfluous.

It doesn't because the feature has nothing that cannot be replaced via other means, and thus its usefulness is dictated only be the number of users.
The same cannot be said of, say, wall-placement helpers.


Oh, yes, I got that hint and then forgot about it, thanks for the reminder. In both cases this comes from years back, and we now have a much better settings page which imo makes local.cfg rather redundant.


I wonder only about which parts of the code were accepted.

Not much to wonder, is there? link mauve only accepted the "idea" and the general refactoring of the code.

source/ps/ModIo.cpp
89 ↗(On Diff #12090)

Good point, thanks.
It seems you added the functionality to prevent this in rP21867, so I'll simply re-use that system going forward.

wraitii updated this revision to Diff 12161.Jun 6 2020, 8:36 AM

Actually protect the modIO keys, and I've reverted the CFG_SYSTEM deletion I'll do it in another diff.

Vulcan added a comment.Jun 6 2020, 9:01 AM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/VideoMode.h
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2018"

source/ps/VideoMode.h
|  23| class·CVideoMode
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCVideoMode{' is invalid C code. Use --std or --language to configure the language.

source/ps/scripting/JSInterface_ConfigDB.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"

source/ps/ModIo.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"

source/ps/ConfigDB.h
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"

source/ps/ConfigDB.h
|  58| class·CConfigDB·:·public·Singleton<CConfigDB>
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCConfigDB:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/mod/hwdetect/hwdetect.js
|    |++++| /zpool0/trunk/binaries/data/mods/mod/hwdetect/hwdetect.js
| 340| 340| 
| 341| 341| global.RunHardwareDetection = function(settings)
| 342| 342| {
| 343|    |-	//print(JSON.stringify(settings, null, 1)+"\n");
|    | 343|+	// print(JSON.stringify(settings, null, 1)+"\n");
| 344| 344| 
| 345| 345| 	var output = RunDetection(settings);
| 346| 346| 
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/mod/hwdetect/hwdetect.js
|    |++++| /zpool0/trunk/binaries/data/mods/mod/hwdetect/hwdetect.js
| 344| 344| 
| 345| 345| 	var output = RunDetection(settings);
| 346| 346| 
| 347|    |-	//print(JSON.stringify(output, null, 1)+"\n");
|    | 347|+	// print(JSON.stringify(output, null, 1)+"\n");
| 348| 348| 
| 349| 349| 	for (var i = 0; i < output.warnings.length; ++i)
| 350| 350| 		warn(output.warnings[i]);

binaries/data/mods/mod/hwdetect/hwdetect.js
| 179| »   var·disable_audio·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'disable_audio' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 180| »   var·disable_s3tc·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'disable_s3tc' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 181| »   var·disable_shadows·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'disable_shadows' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 182| »   var·disable_shadowpcf·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'disable_shadowpcf' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 183| »   var·disable_allwater·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'disable_allwater' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 184| »   var·disable_fancywater·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'disable_fancywater' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 185| »   var·enable_glsl·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'enable_glsl' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 186| »   var·enable_postproc·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'enable_postproc' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 187| »   var·enable_smoothlos·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'enable_smoothlos' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 188| »   var·override_renderpath·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'override_renderpath' to undefined.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 222| »   //·Enable·GLSL·on·OpenGL ES 2.0+,·which·doesn’t·support·the·fixed
|    | [NORMAL] ESLintBear (no-irregular-whitespace):
|    | Irregular whitespace not allowed.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 222| »   //·Enable·GLSL·on·OpenGL ES 2.0+,·which·doesn’t·support·the·fixed
|    | [NORMAL] ESLintBear (no-irregular-whitespace):
|    | Irregular whitespace not allowed.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 179| »   var·disable_audio·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'disable_audio' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 180| »   var·disable_s3tc·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'disable_s3tc' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 181| »   var·disable_shadows·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'disable_shadows' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 182| »   var·disable_shadowpcf·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'disable_shadowpcf' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 183| »   var·disable_allwater·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'disable_allwater' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 184| »   var·disable_fancywater·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'disable_fancywater' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 185| »   var·enable_glsl·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'enable_glsl' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 186| »   var·enable_postproc·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'enable_postproc' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 187| »   var·enable_smoothlos·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'enable_smoothlos' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 188| »   var·override_renderpath·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'override_renderpath' to 'undefined'.

binaries/data/mods/mod/hwdetect/hwdetect.js
| 222| »   //·Enable·GLSL·on·OpenGL ES 2.0+,·which·doesn’t·support·the·fixed
|    | [NORMAL] JSHintBear:
|    | This line contains non-breaking spaces: http://jshint.com/docs/options/#nonbsp
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2332/display/redirect

This revision was automatically updated to reflect the committed changes.