Configuration of the engine

[antsouchlos]

I noticed, that in commit a3c4a1d4284e6250d7693f3157c7a62b0f118737 a construct along the lines of

namespace preinit{
    extern bool use_legacy_renderer;
}

int main() {
    oe::preinit::use_legacy_renderer = true;

    //...
}

was added to configure which version of the renderer is used.

I must say that I personally find this construct extremely ugly for a couple of reasons, not the least of which being that it introduces global variables. Another reason is that I would like to only perform configuration at runtime when it is necessary / sensible. In my opinion in all other cases configurations options should be compile time constants.

Here are a bunch of ideas for how we could implement configuration options:

Compile-Time Configuration

Still global, but constexpr static

The user of the engine creates a config.h header file, with content along the lines of

namespace conf {
    constexpr static option1 = false;
    constexpr static option2 = false;
    constexpr static option3 = true;
}

This header file has to be included above other OEngine header files in the game source code. Configuration options can then be accessed by the engine source code by doing e.g. conf::option1. If any necessary configuration option is missing, the compiler will throw an error.

Compile-time evaluated JSON-String

Essentially the same as above, but a bit fancier :P

The user of the engine creates a config.h header file, with the configuration options written in JSON format (Don't worry about const_string, it's just a type to store a compile time string literal):

const_string conf = "{
                      'option1': true,
                      'option2': true,
                      'list_option3': {
                          'inside_opt1': true,
                          'inside_opt2': false
                      }
                     }";

This would go very well with std::embed, if and when it is accepted into the C++ standard (A long shot, I know :P)

Runtime Configuration

No general runtime-conf options

Don't provide general options to configure stuff, only have configuration options be passed as function arguments to each engine function.

Call a reconfigure function with a dedicated configuration struct as an argument

int main() {
    // ...

    oe::conf_t new_conf;
    new_conf.option1 = false;
    new_conf.option2 = false;
    new_conf.option3 = true;

    oe::reconfigure(new_conf);

    // ...
}

Internally, this could be implemented with a Singleton (A class that only has one instance, and only a reference to this instance can be obtained). While still kind of the same thing as a global variable, to me an engine-internal class that handles configuration still seems like a better option than just raw global variables:

namespace oe {
    // ...

    void configure(conf_t conf) {
        config_handler_t& handler = config_handler_t::get_instance();
        handler.set_conf(conf);
    }

    // Some engine-internal function that needs configuration values
    void some_func() {
        config_handler_t& handler = config_handler_t::get_instance();
        conf_t conf = handler.get_conf();
    }
}

While I am not sure how that would be implemented, in this case something pretty helpful would be if we could somehow prevent engine-internal software from using the set_conf() function (or otherwise writing to the configuration options), with the explicit exception of oe::configure.

[philsegeler]

This is a well written issue i must say. But i find there are a few problems with your suggestions themselves.

FYI there is already one single global variable in the OE API and that is

namespace oe{

    // GLOBAL VARIABLE
    extern OE_TaskManager* OE_Main;

If you have any idea how to conceal it and make it only accessible from inside the API that would be nice. This is not particularly important since all C-based libraries have this problem though. (SDL included)

Global variables also exist in the renderer api to a greater extent: (include/Renderer/NRE_GPU_API.h line 101)

//////////////////////
//  all variables here
extern void* api;
extern info_struct backend_info;
extern std::atomic<bool>  use_wireframe;

extern uint32_t x;
extern uint32_t y;
//////////////////////

The problem is that those parameters must transcend the lifetime of a specific void* api. The renderer (and the physics engine and the networking engine) should be able to be restarted on runtime at will. The renderer GPU API is already able to restart itself on runtime if needed due to parameter changes.

This does not really apply to the engine (OE_TaskManager) itself if we assume that only after oe::init the runtime configuration options can be changed, but it does for all the other APIs. If you have any suggestions for improvements, they are welcome. Could we use singletons?

Compile-Time Configuration

Still global, but constexpr static

Riiiight. There is a really important problem with this suggestion. Are you suggesting that the user HAS to provide a config.h file? I find this unnecessary complicated. It should be possible to configure and use the engine completely using a single file. I strongly suggest examples in the examples directory to be single-file for simplicity's sake. I always hate it when i am looking for examples to use some library and they always try to split the code into small .h and .cpp files instead of making it as simple as possible...

Could this be optional? Can we make a default configuration and make the oe::conf namespace optional? Maybe a oe/default_config.h that could be included before oe/api_oe.h would suffice?

What i think does not look very good:

#include <whatever_file.h>
#include <whatever_file2.h>

namespace oe { namespace conf {
    constexpr static option1 = false;
    constexpr static option2 = false;
    constexpr static option3 = true;
};

#include <OE_API.h> // yeah right i need to the new names. The new name would be <oe/api_oe.h>

int main(){

    oe::init();
    //...
    oe::destroy();
    return 0;
}

Compile-time evaluated JSON-String

Can we then do this even for compile-time constants?

#include <whatever_file.h>
#include <whatever_file2.h>
#include <oe/api_oe.h>

const_string conf = "{
                      'option1': true,
                      'option2': true,
                      'list_option3': {
                          'inside_opt1': true,
                          'inside_opt2': false
                      }
                     }";

int main(){

    oe::preconfigure(conf); // can this be constexpr (?) Can we adapt the OE_API.h for this to be possible?
    oe::init();
    //...
    oe::destroy();
    return 0;
}

If yes then i have no problem with that. I can tolerate more complexity in the OE_API.h and OE_API.cpp if this means the user-facing API looks simpler. If we cannot do this then i prefer a namespace for compile-time parameters.

The big problem with compile-time parameters is that they are set in stone. They can only be changed by the user and not the player. I can hardly think of a single option that fits this requirement for the renderer. The preinit::use_legacy_renderer definitely does not fit this. It cannot change on runtime, but it should be able to be changed by the actual player before launching the engine at least...

Runtime Configuration

No general runtime-conf options

What if the user only wants to change ONE parameter and not all at once?

Does the user also have to query and remember every single option there is? What if we load a player config and the game developer(user) does not (of course) know the parameters. Does the user have to query them by hand then?

Call a reconfigure function with a dedicated configuration struct as an argument

Again what if the user only wants to change ONE parameter and not all at once?

The logic with constant parameters cannot be applied as is for dynamic ones as i see it. We could still make this an option though. Right now my idea is to make explicit functions accepting values and functions that toggle (if it is boolean):

From OE_API.h :

void set_shading_mode(oe::renderer::shading_mode::type);
oe::renderer::shading_mode::type get_shading_mode();

void render_wireframe(bool);
void toggle_wireframe_rendering();

void render_bounding_boxes(bool);
void toggle_bounding_boxes_rendering();

void render_bounding_spheres(bool);
void toggle_bounding_spheres_rendering();

void render_HDR(bool);
void toggle_render_HDR();
// i was bored so i haven't implemented the has_wireframe_rendering() or similar etc.

If you have any objections to the naming or the pattern i use i am all ears.

We could still also implement a reconfigure function. BUT I think it would only be useful if it reads a config file and not with a struct in code.

Concerning singletons i find this a little hard to understand how it would work exactly. Consider i have OE_Main as a global variable i can imagine it would work something like this theoretically:

namespace oe {
    // ...

    void reconfigure(conf_t conf) {
         OE_Main->set_conf(conf); 
    }

    // Some engine-internal function that needs configuration values
    void some_func() {
        OE_Main->set_conf(conf); // i dont think this can ever happen
    }
}

The problem would be to conceal `OE_Main somehow if possible, but again not something i personally worry about in particular. Maybe we could make THIS a singleton?

I do not think this is the way it would happen anyways. the reconfigure API function would just call the respective user-facing API functions for each parameter separately. :P

Final point

I definitely cannot make preinit::use_legacy_renderer a compile-time constant. I can accept a oe::preconfigure (with JSON or using your parser) plus a possibility to use a real player configuration file for non compile-time constants that cannot be changed at runtime.

[philsegeler]

SInce you also talk about runtime configuration please change the title to reflect that :P

[antsouchlos]

Runtime Configuration

Actually, making OE_Main a Singleton isn't a bad idea. As for it's concealment from the user, I would put everything the user is not supposed to directly access in an internal oe_detail namespace:

namespace oe {

    namespace oe_detail {
        // Internal OE Stuff the user is not supposed to see directly
    }

    // Public API

}

I could live with the TaskManager class handling configuration. All the conf functions used for setting the different parameters could then simply access the OE_Main instance:

void render_wireframe(bool b) {
    oe_detail::OE_Main->render_wireframe(b);
}
void render_bounding_boxes(bool b) {
    oe_detail::OE_Main->render_bounding_boxes(b);
}
void render_bounding_spheres(bool b) {
    oe_detail::OE_Main->render_bounding_spheres(b);
}

My main issue is somehow handling configuration centrally. I don't like the idea of a bunch of global variables somewhere, which can be changed and accessed from anywhere and everywhere.

Compile-time configuration

Proper compile time configuration requires either macros, constexpr static variables or templates. The first two would need to be dfined before anything else engine-related, i.e. in a user-written config file that get's included before everything else (At least I don't really see another way). The template option essentially requires a completely object oriented internal API, like in the example I proposed in issue #25 .

But we can ignore this for now. I am not sure that it would even be useful, since I can't think of any compile-time configuration options we might use.

[philsegeler]

oe_detail is a pretty good idea. I can implement that!

The really nice thing about those global variables is exactly the thing that you do not like. The only alternative i can see is to put them all inside the TaskManager, but at least some of them still would be static.

I already put all parameters that i can do inside the dummy classes in OE_DummyClasses.h. I also just noticed the use_wireframe as a global variable in the GPU API is probably not needed, but i need to investigate more.

There are also some leftover global variables in the event handler.

We still also have OE_World::objectsList etc. as static global variables, which i wanted before to put inside the taskmanager, but the renderer had a higher priority.

Would putting all possible global variables (static or not) inside the task manager and implementing config methods be enough? All those variables are still accessible through oe::oe_detail::OE_Main or TaskManager:: this way and we could make a singleton although i dont really know how.

Also should i keep preinit::use_legacy_renderer as it is for now or make it a function which would change a static task manager variable?

Should we introduce JSON with preconfigure for non compile constants in the future?

[antsouchlos]

About TaskManager

Now that I think about it, by adding e.g. config options, the TaskManager class seems to be becoming just a complete OxygenEngine handler class, not just meant for task managing. Would it be possible to restrict the functionality of the TaskManager to task managing and create a new oxygen_engine_t class, which will have a TaskManager instance as a member? The oxygen_engine_t class can then take on all functionality needed for managing the engine (config options for example).

About globally accessible variables in general

I understand the convenience of global variables (or public static members for that matter, but they basically are the same thing). I still think we should avoid them like the plague, however, and try to keep everything contained in its own "box" with as little communication between boxes as possible. That makes everything a lot easier to debug and write unit tests for.

Of course we need some global variables, since we want a C-Style API. I would limit their use as much as possible though and use them only where really necessary. In the best case there would only be a single global variable of some type oxygen_engine_t (with no static member variables, or at least no public static ones) and all functions of the API would access that in one way or another.

What to do with preinit::use_legacy_renderer

I would keep this the way it is for now.

But we should implement a proper config management solution in the not so distant future and handle all config tasks that way.

[philsegeler]

About TaskManager

This would be more complicated. The oxygen_engine_t would already have to pass the right configuration options to the taskmanager which would then pass them to the respective subsystems.

On the other hand, this would make the task manager simpler to an extent. I will open an issue once i have an updated design proposal ready.

EDIT: FYI the C-style API already provides the abstraction we need to handle such configuration. We do not need a separate class. Although it could make the API and OE_API.cpp simpler.

About globally accessible variables in general

I tried to remove them as far as I could, but there are many complications. I need for example both the window system and the renderer to be able to get access to the window dimensions or backend_info. The obvious solution would be to let the window system handle renderer initialisation and execution. However, this makes the window system more complicated and more importantly we cannot independently decouple the renderer from the window system. It would make the window system do half the job of task managing if that were to be the case.

Also the renderer (and the physics engine and csl as well) need direct access to the actual objectList etc. I cannot easily make them non-static members of taskmanager or oxygen_engine_t, since i would need to always pass around relevant pointers...

I may need to think about a partial redesign here as well. I already have something in mind using return values and arguments for the renderer and window system.

Thoughts/ideas welcome especially regarding the objectsList and similar.

What to do with preinit::use_legacy_renderer

Ok.

I like the idea of oe::preconfigure. We could use JSON or your parser together with it. For file configs we could use oe::preload_config or something similar. It should still be possible to only change parameters individually and not all at once.