Page MenuHomeWildfire Games

Adds callbacks to the CConfigDB
Needs RevisionPublic

Authored by vladislavbelov on Jun 11 2018, 12:27 PM.

Details

Reviewers
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

Currently we always need to create a temporal variable to get a config value. And we don't know, was it really initialised.

I added simple callbacks for it. It's not the best what we can reach, but it's pretty limited by CppSupport. So syntax sugar can be added later.

Example of usages:

g_Config.GetValue<bool>("...", [this](const bool& ...) {
    objectPointer->SetProperty(...);
});

g_Config.GetValue<bool>("...", std::function<void(const bool&)>(
    std::bind(&Class::SetProperty, objectPointer, std::placeholders::_1)
));

It's also needed for D1571.

Test Plan
  1. Apply the patch and run the game.
  2. Make sure that all options work correctly.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 6822
Build 11217: Vulcan BuildJenkins

Event Timeline

vladislavbelov edited the summary of this revision. (Show Details)Jun 11 2018, 12:31 PM
Vulcan added a subscriber: Vulcan.Jun 11 2018, 12:37 PM

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

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

elexis added a subscriber: elexis.Nov 21 2018, 9:36 AM

And we don't know, was it really initialised.

Shouldn't there also be a callback function on failure then, used when globals are initialized by a config value?

syntax sugar can be added later.

What syntax sugar do you mean?
(There are 117 calls to CFG_GET_VAL, but I see that the callback function is only a new variant of the function, so one can keep using the old syntax.
Otherwise the repetition of 117 std::function<void(const bool&)>([this](const bool& windowed) might be considered slightly ugly.)
It would be preferable to touch these 117 at most once, if there is any 'sugar' possible at all.
If there isn't, then the question of syntax sugar doesn't even arise (and the type doesn't look so ugly, just repetitive for something that this macro might possibly avoid).

But there is also a logic change when using the callbak syntax.
Previously, variables were initialized immediately, the code following that init could rely on it.
Now the initial value of the target variable is not changed in a case of failure to read the config entry.
Perhaps that warrants a comment, we don't people to start using uninitialized variables.

(Also is it only me or is CFG_GET_VAL(...) not much nicer than g_ConfigDB.GetValue(CFG_USER, ...)?)

source/ps/ConfigDB.h
88

macro?

The type definitions must be kept in sync with those in ConfigDB.cpp. Ideally the type definitions would only be in one place. Could one implement that with some imagination or taking examples of other places (I think of *ScriptConversions.cpp)?

source/ps/VideoMode.cpp
65

This function is only called once, so it's not really relevant to use the new syntax here, is it?
Should the callback syntax be used everywhere or only where it is relevant for performance?

wraitii added inline comments.
source/network/NetSession.cpp
46 ↗(On Diff #6749)

Split and commit separately?

source/ps/VideoMode.cpp
65

Can't this be written as a lambda instead?

vladislavbelov edited the summary of this revision. (Show Details)Jan 20 2019, 2:37 PM
vladislavbelov marked an inline comment as done.Jan 20 2019, 2:42 PM
In D1573#66460, @elexis wrote:

And we don't know, was it really initialised.

Shouldn't there also be a callback function on failure then, used when globals are initialized by a config value?

Yes, it could be. If it's needed for someone.

In D1573#67421, @elexis wrote:

syntax sugar can be added later.

What syntax sugar do you mean?
(There are 117 calls to CFG_GET_VAL, but I see that the callback function is only a new variant of the function, so one can keep using the old syntax.

Yes, I'm keeping the current syntax.

Otherwise the repetition of 117 std::function<void(const bool&)>([this](const bool& windowed) might be considered slightly ugly.)

Now the callback is just [this](const bool& windowed).

(Also is it only me or is CFG_GET_VAL(...) not much nicer than g_ConfigDB.GetValue(CFG_USER, ...)?)

I agree with that, I refactored it. Now it's easier to use g_ConfigDB.GetValue.

source/ps/VideoMode.cpp
65

Can't this be written as a lambda instead?

It's already lambda, just it used an explicit conversion.

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

Link to build: https://jenkins.wildfiregames.com/job/differential/987/

Stan added a subscriber: Stan.Jan 20 2019, 3:32 PM
Stan added inline comments.
source/ps/ConfigDB.h
73

Maybe could be a trac ticket instead ? Sounds like a good candidate for a simple ticket.

vladislavbelov marked an inline comment as done.Jan 20 2019, 3:44 PM
vladislavbelov added inline comments.
source/ps/ConfigDB.h
73

Yes, it's true. But it depends on our CppSupport.

Stan added inline comments.Jan 20 2019, 3:49 PM
source/ps/ConfigDB.h
73

Sure, was just merely mentioning that it maybe easier to direct people to tickets rather than to code :)

wraitii requested changes to this revision.May 28 2019, 12:21 PM

I think it would be more interesting if we instead allowed ConfigDB to register hooks that are called when a config changes, instead of a direct callback. Then with D1929/D1930/D1931 we could also clean JSInterface_Renderer (instead of having a wrapper, we could just change the config setting an Rendering Options would call the appropriate method, propagating side effects nicely).
This would finally make it possible to add scriptable rendering options while only changing the rendering options.

This revision now requires changes to proceed.May 28 2019, 12:21 PM