Replace `NAS2D::Utility` uses with `inline`

[DanRStevens]

The NAS2D::Utility template usage is effectively superseded by inline variables. Effectively both NAS2D::Utility and inline are ways of declaring global variables without splitting off a separate .cpp file for the definition. An inline variable may be declared and initialized simply in a header file, and then included by files that need to use it.

There may be more than one definition of an inline function or variable (since C++17) in the program as long as each definition appears in a different translation unit and (for non-static inline functions and variables (since C++17)) all definitions are identical. For example, an inline function or an inline variable (since C++17) may be defined in a header file that is #include'd in multiple source files.

Earlier discussion: https://github.com/OutpostUniverse/OPHD/issues/948#issuecomment-859939126

---

There is an interesting StackOverflow answer that compares the use of static variables in class templates for older versions of C++, and the inline keyword from C++17, and how they are implemented using the same linker feature:

https://stackoverflow.com/questions/38043442/how-do-inline-variables-work

For reference, the definition of Utility includes:

template<typename T>
class Utility
{
// ...

private:
    static T* mInstance; /**< Internal instance of type \c T. */
};

template<typename T> T* Utility<T>::mInstance = nullptr;

[Brett208]

Certainly better to use the new language feature. Does global/inline variables push us towards tighter coupling between the various modules than preferred for the long term solution?

I was thinking registering modules with an event handler might make it easier to decouple modules if needed in the future. Although we don't plan to use unit testing so don't want to use a heavier solution than needed. I always have to review event based programming when trying to use it because I find it a bit unintuitive. I've also only really used in in C# / .net.

[DanRStevens]

I would prefer using fewer globals. Though with that said, use of NAS2D::Utility effectively already defines globals. Just the syntax to access them is more complicated. Moving from NAS2D::Utility based globals to inline globals would be an improvement. If we can take things further to eliminate some use of globals, that would be even better.

On that line of thought, I think many uses of Renderer would be better if it was passed as a parameter to draw methods (which could be the const portion of the current update methods). Might also want to store a Filesystem component in a high level game class. The Filesystem doesn't need to be passed very deep to most components (or at least we could avoid such a design), so that might be another global that is relatively easy to get rid of.

But that discussion is perhaps getting a bit out of scope here. There's a lot we could do to refactor the code, and avoid using globals. Though that's more of a long term plan, and we may end up with separate issues for the various components we want to refactor the use of.

[ldicker83]

I would not want to pass Renderer as a parameter. Since it's universally used throughout the entire program, it can safely be a global. Same with Filesystem, Mixer, etc.

I know that 'global variables are evil' but sometimes they aren't. Constants, for instance. Or in the case of objects like this where there's only one throughout the life of the program. We should never have more than one active Renderer, Filesystem, Mixer, etc. And I just know that there will be the argument of "But what if...?", no. Renderer is only ever intended to be instantiated once. Filesystem is only ever intended to be instantiated once, etc. etc. etc..

These can simply be wrapped in a Utilities namespace or similar and declared/defined there. Same with other objects that we want to pass around like the StructureManager and one or two others that I think we're doing the same thing with. It really just makes it easier because these aren't things that need to be in parameters. Would just clutter the interface.

[DanRStevens]

So I understand the desire to not add clutter. However, as it stands now, the code is cluttered with NAS2D::Utility<Renderer> lines to get access to the renderer, which could have just been passed in as a parameter with less code. This is true of every draw function.

Granted switching to inline globals would reduce much of the clutter in accessing the renderer, making it more on par or slightly less code than passing it in as a parameter.

---

I don't see a need for OPHD to use more than one renderer, nor more than one filesystem. Though I can certainly imagine instances where something like that might be desirable. If two solutions are about equally difficult to write, and one allows for more modular use and multiple objects, if desired, that's probably a better way to go. Nothing stopping you from using a global in a design that supports modular use.

At any rate, this seems like more of a design decision for the future. I don't think we're really in a position yet to tackle issues such as using a global versus passing things as parameters.

First step is switching to inline globals.

[cugone]

Just my two cents: I recently switch my engine from global pointers (downstream games use these) and dependency injection (Engine-side only; i.e. Renderer parameter arguments everywhere, especially in constructors) to global pointers and a Service Locator pattern, which is what NAS2D::Utility<> emulates.

It's a little more work to get right because of how it's implemented, but it cut down on the Constructor-clutter on the engine side from up to eight arguments for each subsystem to zero. Wherein each individual function requests the service it wants when it wants on-the-fly and the class didn't need to hold a reference to the subsystem the entire time. I much prefer a service locator than dependency injection.

[ldicker83]

@cugone has the general idea of what I was going for here. I heavily dislike dependency injection. The argument that using the Utlity object clutters code at the location is used seems like grasping at straws. For me, it makes perfect sense -- you get a reference to a service/utility object where and when you need it. Having to modify functions to take pointers/references to service classes is painful. It was the original way I did it back before NAS2D was even a thing and it sucked. So I opted for this method which is technically a global pointer just wrapped inside an interface. But, it made the interface a lot easier to read.

Grabbing a reference or pointer to a global class through a standardized interface just seems very intuitive to me. It's why I preferred it. Of course this was before inlining was a thing -- though I'm not sure using inline would really improve the code here. It might improve overall performance but I suspect the improvement would be negligible at best. Plus it still creates the problem of "Where the hell did this come from??" which is what the Utility interface was designed to solve.

[DanRStevens]

To be clear, I was never suggesting dependency injection. That would involving passing extra parameters to the constructor, and then storing local references. The suggestion I had earlier was to pass a parameter to a draw method, and not store any local references.

Either way though, you need access to a Renderer, so it needs to come from somewhere. That could be dependency injection, a service locator, a global variable (inline), or by passing a parameter. The choice between using a service locator, a global, or passing a parameter is pretty small in terms of impact on the code.

There should be no performance change between an inline global and using Utility. They are effectively the same under the hood. It's the same linker feature that implements both. The main difference is perhaps that inline globals would require less source code, since that linker feature is now more directly accessible.

[DanRStevens]

One additional thought. It would be easier to write unit tests for draw code if we passed the Renderer in as a parameter. That would be considerably more awkward to do with a global Renderer object, which is implied by an inline global, or a simply written service locator.

[ldicker83]

So I know it's been forever since I weighed in here but I'm starting to lean toward dependency injection as @DanRStevens has suggested with the ultimate removal of the NAS2D::Utility object. I've been implementing this sort of interface more and more prototype code I've been noodling around with and I'm finding it a lot cleaner to know where these objects are coming from. Plus it really shows what's needed where.

[DanRStevens]

I should probably point out that my suggestion here was to use inline globals defined in header files, rather than NAS2D::Utility. Both features make use of the same linker feature to expose a global.

I also made a side comment about how we should probably pass in a Renderer object reference from the root draw call, which passes it down further to sub-object draw calls. Though that's a bit of a side discussion, and maybe should get it's own issue.

I wasn't the person who brought up dependency injection. Though technically, we already meet that definition:

In software engineering, dependency injection is a programming technique in which an object or function receives other objects or functions that it requires, as opposed to creating them internally.

Whether we use NAS2D::Utility, or inline header variables, or service locator patterns, the code using those objects doesn't need to be concerned with how they were constructed, nor are they constructed internally.

With that said, the typical implementation of dependency injection is to received references to such objects in a constructor, and stored those references internally. I explicitly do not recommend this approach (for draw and Renderer). It adds a lot of clutter to constructors, and requires extra fields on objects the store the references. In the case of the Renderer object, it's only ever really used in draw functions, so it makes more sense to receive the object as a parameter to draw, rather than a constructor parameter which then needs to be stored.

Pretty sure the constructor parameter approach wasn't what you meant. Just wanted to be explicit here so there's no confusion in case someone else takes up this case.