Module system needs to be refactored

I'm not really sure that the ModuleBase class is very helpful from a design perspective. Basically, from what I understand, it looks like it gives you access to the "main" ElDewrito interface objects, but it shouldn't be doing this through inheritance. Inheritance is intended to model "is-a" relationships, but we're only using it for code reuse (in fact there's nowhere in the codebase where a ModuleBase pointer is even used). You could argue that it indicates that a class "is a" module, but the term "module" is extremely vague and just covers up the fact that "modules" have way more responsibilities than they should. The problem with this, then, is that it leads to things like ModuleConsole.cpp having over 1k lines of code in it or ModuleForge having a disgusting-looking hook function inside of it. What if another piece of code wants to trigger a Forge delete operation, for example? It shouldn't have to execute a command to do it, which is clunky and requires string manipulation. We should never have to run commands internally to accomplish something which could easily be done if we had a properly-structured codebase.

In order to maximize reusability and keep our code simple, we need to clearly separate the different layers of the system instead of combining everything into "modules":

Hooks should be objects which are completely isolated from the rest of the codebase (because they're nasty) and have very few dependencies. They should simply either be event sources or provide public methods which can be used to control them. Hooks should never ever have to know about the command system being present. (Though whether they should be able to access named variables is debatable.)
The command system should just be a layer on top of everything else. (That is, you could take away the command system and have everything still work.) Each command should only be a few lines of code long and use classes/functions lower in the hierarchy (parse arguments, call function, output result). Additionally, we should be using std::function for command function callbacks so that commands can access non-global state. Commands could be created through an "ICommandRegistrar" interface of some sort which takes care of putting commands in a namespace corresponding to the plugin name.
If we want to keep the variable system the way it is currently, we need to separate the variable and command systems such that the variable system is much lower in the hierarchy. We can have something like an IVariableStore which stores variable names and values, and then the command system can access that when the user requests to get/set a variable. When a variable is updated, the store would fire an event. This would decouple variable accessors from the command system.
Any UI code (drawing the console, etc.) needs to be completely separated from everything else so that it can easily be disabled all at once in a dedicated server environment. I've thought about possibly using some sort of 3rd-party library (e.g. librocket) for stuff like the console and chat until we get further with the embedded Chromium renderer.
There needs to be a class whose sole responsibility is to provide interface objects to a plugin (and to internal code as well). We can pass a pointer to it to the InitializePlugin() function and leave it up to the plugin to use it how it likes. In its simplest form, it could just have a pure virtual CreateInterface() method, but I'm not necessarily sure I like that because then you lose type safety by having to cast a void*. I'd rather see it expose a function for each type of interface that can be retrieved.

TL;DR, frankly what I'm getting at here is that we need to stop making this so monolithic that it's impossible to reuse code outside of modules. The current system still makes things way too difficult to extend easily if we want to add business logic like variable synchronization that isn't tightly coupled to the console. Granted, it's a step in the right direction compared to the original codebase, but there's still a lot that needs to be done before I'm comfortable using it.

Any thoughts?

I agree 100% with this. Commands should not be run internally; what the commands do should be accessed directly. ModuleBase should not just be for reusing code like it is. All of these things definitely need to be looked at.

Well I always intended modules to be used for four things:

Methods that can be used in code
Registering commands so we can call those methods outside of code
Implementing hooks/patches needed by those methods, or related to the module (eg the input module having our raw input hook)
Registering variables used by the methods or hooks to change how the module works

Putting helper functions under ModuleBase and making modules inherit from that seemed the simplest way to do things, but as you said "Inheritance is intended to model "is-a" relationships, but we're only using it for code reuse". Perhaps we should refactor the ModuleBase into an ICommandRegistrar class instead, and instead of having the modules inherit from it we can just create an instance of ICommandRegistrar inside the "module".

but the term "module" is extremely vague

Each of the "modules" perform roughly the same actions that I listed above, they're essentially the same things, calling them modules seemed the best fit.

and just covers up the fact that "modules" have way more responsibilities than they should.

What do you mean by that? Each "module" sets up everything needed for the code in that "module" to work, no more and no less, I'd say they have the right amount of responsibilities.

The problem with this, then, is that it leads to things like ModuleConsole.cpp having over 1k lines of code in it

ModuleConsole is a complete hack job right now anyway, once we have a working HTML5 UI overlay we can probably remove it. I wouldn't blame this on it being a module though, it'd still be a hacky piece of code no matter where we put it.

or ModuleForge having a disgusting-looking hook function inside of it. What if another piece of code wants to trigger a Forge delete operation, for example? It shouldn't have to execute a command to do it, which is clunky and requires string manipulation. We should never have to run commands internally to accomplish something which could easily be done if we had a properly-structured codebase.

Well the code in ModuleForge was mostly just copied verbatim from the ED code. It's true what you're saying about being able to run it directly though, like I said above

Registering commands so we can call those methods outside of code

That last bit isn't being done for a lot of commands, instead the actual code is being put in the command callback instead of the callback calling a method in the module. I'll try going through the commands later to try fixing this.

The suggestion about using std::function is nice too, I didn't really know about that when doing the command system, would have made things a lot easier. Maybe we can use that to call the method directly instead of having a callback call the method for us.

I'm not really sure why we'd need to decouple the variable and command systems though, commands are just a special kind of variable that has no value, don't really see the reason to decouple them. The idea about firing an event when variables change is nice though, we could add that to the current system easily too.

Any UI code (drawing the console, etc.) needs to be completely separated from everything else so that it can easily be disabled all at once in a dedicated server environment.

Agreed with this, the current UI code is mostly a placeholder, those 1k lines in ModuleConsole could be reduced a lot if we used a proper UI library.

All the UI code is in ModuleConsole right now though, you could compile it without ModuleConsole.cpp and the few ModuleConsole refs and the UI won't show since modules are self-contained, should be trivial to stop it loading at runtime.

I'm not necessarily sure I like that because then you lose type safety by having to cast a void*. I'd rather see it expose a function for each type of interface that can be retrieved.

I'm not sure if I agree with this, the way it's done now is mostly for backward compatibility, so if we changed an interface a plugin would still get the version it expects from when it was compiled. No matter what casting would still be needed, which is why I made it use a single CreateInterface method instead (the idea was mostly borrowed from Valve, eg see the bottom of https://developer.valvesoftware.com/wiki/IGameConsole)

There needs to be a class whose sole responsibility is to provide interface objects to a plugin (and to internal code as well). We can pass a pointer to it to the InitializePlugin() function and leave it up to the plugin to use it how it likes. In its simplest form, it could just have a pure virtual CreateInterface() method

If it did only have a single method, what would be the purpose of it though? Plugins can get just get that via our exports.

frankly what I'm getting at here is that we need to stop making this so monolithic that it's impossible to reuse code outside of modules.

Besides the commands being in anonymous namespaces and the ModuleBase inheritance I don't see any other issue with modules. We can already call module methods from outside the module, it's just the command callbacks that we need to refactor so we can access them without going through the command system.

Well I always intended modules to be used for four things:

Methods that can be used in code

The problem with this is that in order to use methods from other modules, we have to include the header file for the module we're importing the code from. This means that the original module is now transitively depending on anything that the dependency module is, so if we change a header file that the dependency module needs then the original module has to be recompiled too for no reason at all. This leads to really long compile times, something which is already kind of a problem (for me at least).

Implementing hooks/patches needed by those methods, or related to the module (eg the input module having our raw input hook)

I can see why this might be nice because it keeps things contained, but at the same time I also think that we need to really focus on keeping the game interaction layer separate from everything else in order to make our code cleaner. It might scare off people who want to help with the project if they open a module to change the syntax of how a command works and see assembly code everywhere. IMO command/module code should never have to work with raw pointers or assembly code directly, because it makes things difficult to change if we ever want to add in something like support for multiple versions of the engine or pattern matching, which might require quite a bit of additional state and logic. It also just discourages code reuse in general.

I'd much rather see a system where we do something like

// ForgeDeleteHook.hpp
class ForgeDeleteHook: public IHookProvider
{
public:
    void SignalDelete();
    virtual std::vector<HookInfo> GetHooks() override; // from IHookProvider
}

// ForgeCommandProvider.hpp
class ForgeDeleteHook;
class ForgeCommandProvider: public ICommandProvider
{
public:
    explicit ForgeCommandProvider(std::shared_ptr<ForgeDeleteHook> forgeDelete);

    virtual std::vector<CommandInfo> GetCommands() override; // from ICommandProvider

private:
    void DoDelete();
    std::shared_ptr<ForgeDeleteHook> forgeDelete;
};

// ForgeCommandProvider.cpp
void ForgeCommandProvider::DoDelete()
{
    forgeDelete->SignalDelete();
}

// (some other file)
auto forgeDelete = std::make_shared<ForgeDeleteHook>();
hookInstaller->Install(forgeDelete);
// ...
auto forgeCommands = std::make_shared<ForgeCommandProvider>(forgeDelete);
commandRegistrar->Register(forgeCommands);

because now we've clearly defined two classes with distinct responsibilities - one is a hook class which can be signaled, and the other provides Forge commands which can be registered. Neither of the classes has a constructor which has side effects on other objects, and their dependencies are very well-defined and can be switched out easily. Additionally, because the delete hook is now decoupled from the command, we wouldn't need to recompile the delete hook if something like the interface for registering commands changes, and it would be extremely easy to make another command provider which also needed the delete hook without pulling in the dependencies from all of the Forge commands.

Each of the "modules" perform roughly the same actions that I listed above, they're essentially the same things, calling them modules seemed the best fit.

If I were to ask someone who's completely new to the project what a "module" does, they probably wouldn't be able to come up with a concrete answer other than "it does a bunch of stuff."

What do you mean by that? Each "module" sets up everything needed for the code in that "module" to work, no more and no less, I'd say they have the right amount of responsibilities.

https://en.wikipedia.org/wiki/Single_responsibility_principle (I understand this is a bit academic, and it's not always good to do that in practice, but it's also something that we should be mindful of.)

I'm not really sure why we'd need to decouple the variable and command systems though, commands are just a special kind of variable that has no value, don't really see the reason to decouple them. The idea about firing an event when variables change is nice though, we could add that to the current system easily too.

Well personally I view the command layer as being part of the UI and the variable layer as being part of the backend. Variables are referenced directly by backend code but commands never are (or at least they shouldn't be). It's not strictly necessary to separate the two, though.

I'm not sure if I agree with this, the way it's done now is mostly for backward compatibility

I'm not really sure we actually need a version number system. If we make a new version of an interface, old plugins will continue to work without having to do anything special as long as it implements at least as many functions as previous versions did (because polymorphism will take care of that for us).

If it did only have a single method, what would be the purpose of it though? Plugins can get just get that via our exports.

Saves us the trouble of having to export something from mtndew.dll itself. Not really necessary to do though. I just don't think that the module constructor should be responsible for registering interfaces because it creates unnecessary dependencies.

That does seem like a nice solution, what worries me is that it seems like making a class for every command might be overcomplicating it a bit, could make it harder for people to contribute.

Maybe something like that but mixed with what we have now would be better, a class that takes references to hooks and returns a batch list of commands registered using those hooks.

Though I'm not sure why we'd need a class for every hook neither, hooks are just pieces of code with a reference to the code + a pointer to where the hook is placed, we could just move the hooks to separate files and have headers to reference them from other places.

That's actually not too far from what we already have, it'd probably only need some minor changes (moving hooks to separate files, changing the command system around a bit, making Modules register commands in a method instead of the constructor..)

Also in your first post you said:

Granted, it's a step in the right direction compared to the original codebase, but there's still a lot that needs to be done before I'm comfortable using it.

Isn't DR better than the alternative right now though? Granted there's some incomplete/non-working things like VirtualKeyboard, but with more contributors these problems could be fixed pretty quickly. (The "I won't contribute to ElDewrito anymore until DewRecode is complete" mentality some people have doesn't really help when there's only a tiny amount of people helping :( )

Nice, pretty much looks like what I had in mind. I'll try and help out if I can too because 0.4.10 is pretty much done (we just need to test it).

Though I'm not sure why we'd need a class for every hook neither, hooks are just pieces of code with a reference to the code + a pointer to where the hook is placed, we could just move the hooks to separate files and have headers to reference them from other places.

We really don't I guess, in fact even the system we're using in original ED isn't that bad. I just want them separated in some way because it makes the code cleaner.

Isn't DR better than the alternative right now though? Granted there's some incomplete/non-working things like VirtualKeyboard, but with more contributors these problems could be fixed pretty quickly. (The "I won't contribute to ElDewrito anymore until DewRecode is complete" mentality some people have doesn't really help when there's only a tiny amount of people helping :( )

That was a pretty dumb thing of me to say, sorry. Yes, it was definitely better than original ED. The main reason I haven't been working on DR is because I wanted to get an update out, but now I'm done with that so I'll have some time to spend on this.

now I'm done with that so I'll have some time to spend on this.

Sweet, well I'll leave this issue open in case anybody has any other issues (the ElDorito::initClasses() is one method I'm unsure about... Hoping somebody knows a better way to implement it)

ElDewrito / DewRecode

Module system needs to be refactored #23