Remove references to globals

[elexis]

Globals should be avoided in C++, since they have to be checked for initialization in most cases. Instead a static GetFoo function could be used that does that check and returns the instance.

One example would be g_Game being used in various places of source/simulation/.

[elexis]

The ticket was created following #3991

See 2016-09-12-QuakeNet-#0ad-dev.log:

07:52 < elexis> Itms: so Game should become a member of SimContext?
07:58 < elexis> (or how should the ref to a global be removed if we dont have a playerID member var)
08:00 <@Itms> No, Game should be accessed with a static method instead of having g_Game
08:01 <@Itms> but that's a lot of work and it's not really an issue right now
08:03 < elexis> so just GetGame() instead of g_Game? The advantage is that we dont need check whether game is initialized when refering to  that function?
08:07 <@Itms> yes, the checks and possible initializations would happen inside GetGame()

[Silier]

WIP

[Itms]

There is no consensus on whether changing globals to static functions is needed/wanted.

The diff ​Phab:D1670 was abandoned for this reason.

[Freagarach]

[11:39:29] <elexis1> #4211 this one seriously lacks the information on how the globals should be removed. its not a choice between using a global g_Game and using a global static GetGame. the game reference should be stored in a LOCAL variable m_Game and either be passed in the constructor or using a setter if necessary
[11:40:18] <elexis1> vladislav knows fully well that it should be done like that and Im glad he had this ticket reopen, but Itms and Angen/Silier dont understand, so probably future readers wont understand either, and Vladislav didnt post on the ticket how it should be done
[11:40:53] <elexis1> (I didnt understand at the time of writing the ticket either)
[11:41:44] <elexis1> (23:08:28) vladislav: Kind of dependency injection.
[11:41:44] <elexis1> (23:08:28) elexis: https://trac.wildfiregames.com/browser/ps/trunk/binaries/data/mods/public/gui/session/session.js#L287
[11:41:44] <elexis1> (23:09:29) vladislav: Yeah

[Vladislav Belov]

Generally we have at least two basic ways to pass some dependencies:

1. We might store it locally:

class CCamera
{
public:
    CCamera(CTerrain& terrain, ...) : m_Terrain(terrain) {}
    void Update()
    {
        const float height = m_Terrain.GetHeight(...);
    }
private:
    // Might a pointer if it can be nullptr or might be a copy
    // if its size isn't critical in this context.
    CTerrain& m_Terrain;
};

2. We might pass it via a function argument:

class CCamera
{
public:
    // The terrain argument might be a pointer if it can be nullptr.
    void Update(CTerrain& terrain)
    {
        const float height = terrain->GetHeight(...);
    }
};

Depending on a) a context, on b) a dependency lifetime and c) how often a dependency might change we might choose one or another. For example, if the lifetime is guaranteed and we need to use the dependency in mostly every method then storing it as a member is a default choice.

[phosit]

Should this ticket be closed as wontfix or repurposed to "C++ Reduce globals"?

Most Globals are used because it's not trivial to bind arguments to JS-interface functions (JS_GetContextPrivate should be used). Another reason is that it's not possible to bind arguments to an InHandler (std::function might be used).

About the lifetime issue when a reference is stored in a class. A bit saver way would be to not take the parameter by reference but take a wrapper to "force" the caller to think about it: Type obj{StoresReference{param}}; when a reference is stored and Type obj{param}; if the parameter is only used in the constructor. I never saw that convention in other codebases though.

[Vladislav Belov]

Replying to phosit:

Should this ticket be closed as wontfix

It can't be closed until it's fixed.

or repurposed to "C++ Reduce globals"?

We need to remove globals not only in C++ but also in JS. Not sure if it's worth to make a different ticket. Because the current ticket is more about design.

[phosit]

Replying to phosit:

Most Globals are used because it's not trivial to bind arguments to JS-interface functions (JS_GetContextPrivate should be used).

The ScriptInterface is not typesafe: To set callback data we have to convert a pointer to the data to void* and call SetCallbackData. To get the data back ObjectFromCBData<T> is called. If T is not the type previously set there is no warning. There is no way to check if the internal void* actually points to a T.

I propose to make ScriptInterface a template. SetCallbackData (I don't think this is nessesary the callback data should be set when constructing the ScriptInterface) and ObjectFromCBData would only work with the template type: The signature would look like this
template<typename T> T& ScriptInterface<T>::ObjectFromCBData().

Another issue is when the object where the void* is pointing at is destructed (e.g. goes out of scope). That can be solved by placing that object inside the ScriptInterface.

[phosit]

Ignore the last comment, there is a type save API in SM91.

[wraitii]

There are a few things to look at on this topic:

• Some globals, like the C++ g_Game global, are 'dependencies of everything'.
• Some globals, like the Simulation, are dependencies of most Sim object.
• Some globals might just be dependencies of specific classes / files.
• Some globals are basically 'always on', like Config, some aren't, like Game. It's worse in JS.

There are a few different approaches to improving the situation IMO:

• In C++, we can differentiate read/write access. That's already an improvement IMO.
• In some cases, we can use the approach we use for Sim components: they store a SimContext object, which is itself pointing to a number of 'global' objects. This is useful, as we can run several instances of the simulation in parallel, and it provides a neat place to do dependency injection. This approach could be extended to a GameContext, though most likely it would still be kinda useful to have a global for this for now.
• Of course in some tight coupling cases storing it as a member variable in C++ makes more sense.
• Passing as arguments in some cases also makes more sense.

For JS code, I think we should just switch to modules. They essentially fix the problem.

[phosit]

In 27883:

Remove Engine.IsGameStarted completely

Revert rP22900

refs: #4211, rP23114

Accepted By: elexis

Differential Revision: ​https://code.wildfiregames.com/D5152

[phosit]

In 27908:

Make the RLInterface local

refs: #4211

Comments By: @vladislavbelov

Differential Revision: ​https://code.wildfiregames.com/D5103