Page MenuHomeWildfire Games

Allow code to register hooks which get called whenever a function value changes, use this to clean rendering options further.
Needs ReviewPublic

Authored by wraitii on Sep 15 2019, 12:19 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

By registering hooks that get called anytime a config value changes, we can further simplify rendering options.

This will make it safer to do D1931 since the renderingOptions is now more equivalent to the config settings.

The system is extremely simple on purpose.

Test Plan

Test that changing renderer config works.

Event Timeline

wraitii created this revision.Sep 15 2019, 12:19 PM
Owners added a subscriber: Restricted Owners Package.Sep 15 2019, 12:19 PM

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

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

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

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

wraitii updated this revision to Diff 9811.Sep 16 2019, 7:24 PM

Replace the macro with C++ code, clarify names. I don't want to do much more for now.

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

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

vladislavbelov added inline comments.
source/ps/ConfigDB.cpp
35

It's strange to has static members in singleton.

208

Why types are different? const CStr vs CStr?

463

I'd prefer m_Hooks.emplace(name, std::move(hook)); if possible.

source/renderer/RenderingOptions.cpp
109–120

Is it guaranteed that this lifetime isn't shorter than CConfigDB?

source/renderer/RenderingOptions.h
122

Maybe empty line before description?

wraitii updated this revision to Diff 9812.Sep 16 2019, 7:53 PM
wraitii removed a subscriber: vladislavbelov.

This time with 100% more not crashing.

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

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

wraitii added inline comments.Sep 16 2019, 7:55 PM
source/ps/ConfigDB.cpp
208

Doesn't compile otherwise, the value_type of std::MultiMap is <const First Arg> (otherwise you could change the key)

source/renderer/RenderingOptions.cpp
109–120

In this instance it's a global, so it should be fine

vladislavbelov added inline comments.Sep 16 2019, 8:06 PM
source/ps/ConfigDB.cpp
208

In that definition - yes, but you just may (if you want) use const std::pair<CStr, std::function<void()>>& hook as you don't need to change the map element.

source/renderer/RenderingOptions.cpp
87

It's a bit dangerous, as it's not guaranteed that the variables is the class member.

wraitii added inline comments.Sep 16 2019, 8:08 PM
source/renderer/RenderingOptions.cpp
87

Well, the earlier Macro would have worked around that, but @elexis preferred it differently.

Your call.

(I _might_ be able to work out a template trick for that, your call)

elexis added inline comments.Sep 16 2019, 8:28 PM
source/renderer/RenderingOptions.cpp
87

Why is it dangerous and not a feature?

elexis added inline comments.Sep 16 2019, 8:43 PM
source/renderer/RenderingOptions.cpp
87

I guess it will complain if the reference doesnt exist anymore (local variable), otherwise shouldn't be wrong to be able to pass globals for example. Also the private keyword limits the use of the function. Didnt check too thoroughly, was the previous macro actually more strict? Sure we cant be as strict without resorting to a macro?

vladislavbelov added inline comments.Sep 16 2019, 8:56 PM
source/renderer/RenderingOptions.cpp
87

Why is it dangerous and not a feature?

Because it might capture a local variable or a member with a different lifetime.

it will complain

Who?

elexis added inline comments.Sep 16 2019, 9:01 PM
source/renderer/RenderingOptions.cpp
87

The program, as in runtime error.
The first version of the diff has the same possibility, no?

vladislavbelov added inline comments.Sep 16 2019, 9:37 PM
source/renderer/RenderingOptions.cpp
87

The program, as in runtime error.

It's UB, so we don't know what will happen exactly.

The first version of the diff has the same possibility, no?

I didn't see, which one?

elexis added inline comments.Sep 16 2019, 9:47 PM
source/renderer/RenderingOptions.cpp
87

You can click on the History tab -> https://code.wildfiregames.com/D2293?id=9775

elexis retitled this revision from Allow code to register hooks which get called whenever a fonction value changes, use this to clean rendering options further. to Allow code to register hooks which get called whenever a function value changes, use this to clean rendering options further..Sep 16 2019, 9:53 PM
wraitii added inline comments.Sep 17 2019, 8:42 AM
source/renderer/RenderingOptions.cpp
87

You're correct, sorry, the macro didn't improve anything in that respect.
Still I would side with elexis that this seems reasonable, it's private and we can hope people will do the right thing.

I can add a comment if necessary.

wraitii updated this revision to Diff 9843.Sep 18 2019, 8:44 PM

Return an iterator when hooking, so we can safely erase it. This code didn't need that, but other code might, and it's cleaner to handle it just in case.

Highlight that we will want to nodiscard this.

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

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

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

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